-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Import from repository doesn't panic #8599
Import from repository doesn't panic #8599
Conversation
if err == nil { | ||
tag = ref.Tag | ||
if len(status.Tag) > 0 { | ||
tag = status.Tag |
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.
This assignment doesn't use the ref
. Why this if
inside err == nil
?
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.
Good catch, this block needs to be moved outside of if err == nil
.
Please add a test that exercises all the failure paths here |
dde538f
to
8862811
Compare
Comments addressed. |
- Check the import status before a use of image object. - Also to set tag names on import status objects in order to produce nice output (containing tag names that failed to import). Signed-off-by: Michal Minar <[email protected]>
8862811
to
24519b0
Compare
LGTM thanks [merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3286/) (Image: devenv-rhel7_4039) |
Evaluated for origin merge up to 24519b0 |
[Test]ing while waiting on the merge queue |
Evaluated for origin test up to 24519b0 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3286/) |
nice output (containing tag names that failed to import).
Resolves #8593
cc @smarterclayton, @legionus