IR-260: Add CLI flag to set ImportMode when importing a tag #1289
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
dorzel
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
It would be nice to validate the user input against the known values for the import mode (before we hit the server). Something like what we do here for the ReferencePolicy.
WDYT?
I think that makes sense to do, added a commit |
|
/lgtm |
| ImportMode: imagev1.ImportModeLegacy, | ||
| }, | ||
| }}, | ||
| }, |
There was a problem hiding this comment.
Would you mind adding a test case also for valid ImportMode values are %s or %s error?
There was a problem hiding this comment.
I've added the test case in a new commit. I realized the test cases weren't running the validation step, so I've also added that step along with related error checking.
| switch o.ImportMode { | ||
| case string(imagev1.ImportModeLegacy): | ||
| case string(imagev1.ImportModePreserveOriginal): | ||
| case "": |
There was a problem hiding this comment.
Due to this assignment, there is no problem. But would it be better to set ImportMode to Legacy in this initialization
oc/pkg/cli/importimage/importimage.go
Line 97 in d27e7c9
ReferencePolicy?
There was a problem hiding this comment.
That would seem to be the better place, but this is also done for ReferencePolicy just above. I'm wondering if this is needed for either of these.
There was a problem hiding this comment.
Ok, it seems this assignment is needed here regardless in the specific case that the user specifies --reference-policy="" or --import-mode="".
| if err := o.parseImageReference(); err != nil { | ||
| t.Errorf("unexpected error: %v", err) | ||
| continue | ||
| } | ||
|
|
||
| if err := o.Validate(); err != nil { |
There was a problem hiding this comment.
Thanks for handling this missing bit. Would it be better to handle the case where validate returns no error but test expects(also a trivial modification by discarding continue statements).
I mean something like this;
if err := o.Validate(); err != nil {
if len(test.err) == 0 || !strings.Contains(err.Error(), test.err) {
t.Errorf("unexpected error %v", err)
}
} else {
if len(test.error) > 0 {
t.Errorf("error expected but not get")
}
}There was a problem hiding this comment.
This makes sense. I've tweaked it a bit as retaining the behavior of continue is needed if there is a validation error so that the import object doesn't actually get created in the test. This makes it clearer what actually failed and emulates the actual behavior better. The general "unexpected error" case is already handled below.
| // set default for referencePolicy | ||
| if len(test.referencePolicy) == 0 { | ||
| for i := range test.expectedImages { | ||
| test.expectedImages[i].ReferencePolicy.Type = imagev1.SourceTagReferencePolicy |
There was a problem hiding this comment.
I think, these expected fields should be immutable. Wouldn't it be better to set them in their initializations instead in here?. Is there any reason for doing these?
There was a problem hiding this comment.
I think the main reason for doing this here was to emulate the behavior of the user not needing to specify the default for it to be set, as it does when working with oc. These needed to be set because the Validate() step ends up mutating the ImportOptions object. But it would be nicer to not have to change the expected fields ever.
|
/retest |
1 similar comment
|
/retest |
|
/approve |
openshift-ci
bot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, dorzel, flavianmissi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
issues.redhat.com/browse/IR-260