serve the status of image acquiring #82

enoodle · 2017-11-22T09:47:51Z

When image-inspector fails to acquire an image and is in the serve mode it fails to serve this status and just exists. I consider this a bug.
To fix I added two fields to the metadata status: ImageAcquireSuccess, ImageAcquireError to report that status.

bazulay · 2017-11-22T10:24:23Z

@simon3z PTAL

simon3z · 2017-11-23T08:50:08Z

@pweil- @ilackarms can you review this?

enoodle · 2017-12-05T13:49:32Z

@ilackarms @pweil- ping

simon3z · 2018-01-04T13:06:42Z

@enoodle anything that we can test here?

enoodle · 2018-01-04T13:44:58Z

@simon3z This is moving a bulk of code into the acquireImage function and then depending on the error either continue or serve the result.
To test this I wanted a good way to fail acquireImage and this is one of the reasons that made me create #87.
I can try to move those tests form there to here, but if this is the only problem with this PR then I think we should first go with #87 and then get those tests .

simon3z · 2018-01-05T09:53:29Z

IIUC you want to add the ability to expose a more granular image-inspector status through the rest-api, and you want to do that before a new release.

Now the question is, do you feel more confident that either this PR or #87 are going to solve more issues than creating new ones? If so how can we be sure about that?

Do you prefer to go with #87 with many more changes and partly tested (and you can add more), or do you want to work more on this one with less changes (smaller impact overall) and trying to add some tests?

enoodle · 2018-01-05T12:54:38Z

I feel that the code is getting too much complicated to keep in one package and that #87 simplifies things. most of the code was just moved and not changed so it not such a big change in my opinion.
I think postponing the change will only make it harder later and that fixing problems and getting new releases is not a difficulty worth not making changes for. So I would go with #87 first.

enoodle · 2018-01-14T16:03:54Z

@simon3z I added tests here. Some even helped identify a bug with decodeDockerResponse that I fixed.

simon3z · 2018-01-14T16:44:13Z

pkg/inspector/image-inspector.go

+
+// acquireImage returns error, the docker image that was acquired, the image ID,
+// the container ID that the image was acquired from, and iiapi.FilesFilter for this image.
+func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, docker.Image, string, string, iiapi.FilesFilter) {


@enoodle when you return such a large number of items you may want to consider an object approach, e.g. either you make those attributes of the relevant defaultImageInspector class (if it makes sense), or you return an object of a new class.

simon3z · 2018-01-14T16:45:37Z

pkg/api/types.go

@@ -93,6 +93,9 @@ var (

 // InspectorMetadata is the metadata type with information about image-inspector's operation
 type InspectorMetadata struct {
+	ImageAcquireSuccess bool   // status of aquiring the image


@enoodle do you need to make this an attribute or could it be just a method that that relies on whether ImageAcquireError is nil or not?

I can do that, It is mostly effecting on the user side.

simon3z · 2018-01-14T16:46:25Z

pkg/api/types.go

@@ -93,6 +93,9 @@ var (

 // InspectorMetadata is the metadata type with information about image-inspector's operation
 type InspectorMetadata struct {
+	ImageAcquireSuccess bool   // status of aquiring the image
+	ImageAcquireError   string // error message from aquiring the image


@enoodle I think this should be an error that you stringfy only on when some output requires it

@simon3z This struct is served as a JSON to the user [1], so I can't have error here.

[1] https://github.com/openshift/image-inspector/blob/master/pkg/imageserver/webdav.go#L96

never mind that, it can json marshal error, I will make this change.

Sorry, apparently it is not possible to meaningfully do it yet
golang/go#5161

simon3z · 2018-01-14T16:49:33Z

pkg/inspector/image-inspector.go

 		}
 	}

 	if i.imageServer != nil {
-		return i.imageServer.ServeImage(&i.meta, i.opts.DstPath, scanResults, scanReport, htmlScanReport)
+		if i.meta.ImageAcquireSuccess {


@enoodle it's not easy to understand why you need this check now. In general I would try to encapsulate any further logic in ServeImage.

Will move to the ServeImage function.

enoodle · 2018-01-14T17:45:23Z

pkg/inspector/image-inspector_test.go

-	return []iiapi.Result{}, nil, nil
-}
-
-func TestScanImage(t *testing.T) {


This test was meaningless , I used to test something but over the time it was refactored not to test anything and the function it used to test was removed.

Factoring out the whole image acquiring to a function and abstracting docker client with an interface to ease testing

also checking return value of getContainerChanges

enoodle · 2018-01-15T08:15:31Z

@simon3z PTAL

simon3z · 2018-01-16T10:23:02Z

pkg/inspector/image-inspector.go

-		}
-		i.meta.Image = *imageMetadata
-		scanResults.ImageID = i.meta.Image.ID
+		i.meta.ImageAcquireError = err.Error()


@enoodle please keep the logic of this (already too long) method simple and return on error.

If you need to serve even on error please use a wrapper method,

enoodle · 2018-01-23T10:46:21Z

ping @simon3z Can you take another look?

simon3z · 2018-01-23T10:57:14Z

pkg/inspector/image-inspector.go

+// acquireImage returns error and iiapi.FilesFilter for this image.
+func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, iiapi.FilesFilter) {
+	var err error
+	if len(i.opts.Container) == 0 {


@enoodle this method calls for a refactoring, anything that looks like

func ... { if (...) { ... } else { ... } }

should be better designed.

Anyway it's not for this PR.

@enoodle to be clear you can just factor out those large bodies to other functions, etc. what I am saying is that a whole long function with two in-line code paths should be refactored.

enoodle mentioned this pull request Nov 22, 2017

read image acquiring status ManageIQ/manageiq-providers-kubernetes#174

Merged

simon3z requested review from mfojtik, legionus and pweil- November 23, 2017 08:49

simon3z reviewed Jan 14, 2018

View reviewed changes

enoodle force-pushed the serve_image_aquire_status branch from dba2c89 to f17e056 Compare January 14, 2018 17:43

enoodle commented Jan 14, 2018

View reviewed changes

Erez Freiberger added 3 commits January 15, 2018 10:09

serve the status of image aquiring

94aea42

Factoring out the whole image acquiring to a function and abstracting docker client with an interface to ease testing

add finish channel to decodeDockerResponse to ensure it finishes

c842dd0

adding test for acquireImage and pullImage

d7cfc31

also checking return value of getContainerChanges

enoodle force-pushed the serve_image_aquire_status branch from f17e056 to d7cfc31 Compare January 15, 2018 08:10

removing unnecessary variables from acquireImage function

18075e4

simon3z reviewed Jan 16, 2018

View reviewed changes

factoring out the serving from Inspect function

710bead

enoodle force-pushed the serve_image_aquire_status branch from 16f7b3d to 710bead Compare January 16, 2018 11:44

simon3z reviewed Jan 23, 2018

View reviewed changes

simon3z merged commit ba9122f into openshift:master Jan 23, 2018

enoodle mentioned this pull request Jan 23, 2018

only use the ImageAcquireError field ManageIQ/manageiq-providers-kubernetes#222

Merged

enoodle mentioned this pull request Feb 13, 2018

Create ImageAcquirer interface and package #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

serve the status of image acquiring #82

serve the status of image acquiring #82

enoodle commented Nov 22, 2017

bazulay commented Nov 22, 2017

simon3z commented Nov 23, 2017

enoodle commented Dec 5, 2017

simon3z commented Jan 4, 2018

enoodle commented Jan 4, 2018

simon3z commented Jan 5, 2018

enoodle commented Jan 5, 2018

enoodle commented Jan 14, 2018

simon3z Jan 14, 2018

simon3z Jan 14, 2018

enoodle Jan 14, 2018

simon3z Jan 14, 2018

enoodle Jan 14, 2018

enoodle Jan 14, 2018

enoodle Jan 14, 2018

simon3z Jan 14, 2018

enoodle Jan 14, 2018

enoodle Jan 14, 2018

enoodle commented Jan 15, 2018

simon3z Jan 16, 2018

enoodle commented Jan 23, 2018

simon3z Jan 23, 2018

simon3z Jan 23, 2018

serve the status of image acquiring #82

serve the status of image acquiring #82

Conversation

enoodle commented Nov 22, 2017

bazulay commented Nov 22, 2017

simon3z commented Nov 23, 2017

enoodle commented Dec 5, 2017

simon3z commented Jan 4, 2018

enoodle commented Jan 4, 2018

simon3z commented Jan 5, 2018

enoodle commented Jan 5, 2018

enoodle commented Jan 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Jan 15, 2018

Choose a reason for hiding this comment

enoodle commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment