-
Notifications
You must be signed in to change notification settings - Fork 29
Conversation
@simon3z @pweil- @ilackarms please take a look |
@enoodle looks good to me |
cc @aweiteka |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it 👍 Some minor questions
type OpenSCAPMetadata struct { | ||
Status OpenSCAPStatus // Status of the OpenSCAP scan report | ||
ErrorMessage string // Error message from the openscap | ||
ContentTimeStamp string // Timestamp for this data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a string? Would there be a benefit to leaving it as Time
? If it is because of issues with json marshalling should we take the Kube approach and provide a wrapper (see unversioned.Time
in Kube) so we retain the option to perform time based functions on this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH it is string because it was string before [1], I didn't go into this that much when I moved these pieces of code around. I agree that it will be nice to have it as Time but I think that changing this should be done in a different PR because the issue is different than the one this PR is about.
[1]https://github.com/openshift/image-inspector/pull/39/files#diff-f5d24f0dc00fe1c3eeda8d22b2e523ebL20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, ok. Been a while, sorry. I agree that we should leave it for this refactor
@@ -143,7 +151,7 @@ func TestGetAuthConfigs(t *testing.T) { | |||
} | |||
|
|||
for k, v := range tests { | |||
ii := &defaultImageInspector{*v.opts, InspectorMetadata{}} | |||
ii := &defaultImageInspector{*v.opts, iiapi.InspectorMetadata{}, &MockImageServer{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you just pass nil here, why the mock? I guess that means the next question is should we test the nil and non nil use case? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests pass with nil
, in [1] we check to see if the image server is defined. I will remove this redundant code for the mock server. It is easy to make again but I don't think it will be needed here anyway.
[1]https://github.com/openshift/image-inspector/pull/39/files#diff-cb14c05f76ccda787aced81d2a624e69R138
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
resubmitting of simon3z#11