retryImageAction: strip error wrappings for MultiError members #68
Conversation
openshift-ci-robot
added
the
size/S
|
Doh. Forgot to mention the bug ID in the commit log message. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
openshift-ci-robot
added
size/M
|
/retest |
|
/test e2e-aws-builds |
|
/retest |
|
@adambkaplan Can we get this merged, seems to have gotten lost in the weeds. |
|
/approve |
| } | ||
| } | ||
| if unwrap { | ||
| err = errors.Cause(err) |
There was a problem hiding this comment.
isn't this stomping the original group of errors? so now you'll only log a single (unwrapped)error from the original group?
There was a problem hiding this comment.
The scoping here is probably confusing - the errors.Cause() call on line 74 is removing additional context (that errors.Wrapf() would have added) from the err defined on line 52, when we detect that there's an authorization error in there somewhere. If the result that first went into errors.Wrapf() is ultimately an errcode.Errors, that list is what err will be set to on line 74. I'll change the name of the one we create on line 67.
There was a problem hiding this comment.
sorry i'm still a little confused:
if errs, ok := errors.Cause(err).(errcode.Errors); ok
if that's true, then err = errors.Cause(err) is going to set err== something that is an array of errors (errcode.Errors)
then you're going to pass array of errors (interface errcode.Errors) down to unwrapUnauthorizedError
which then calls cause := errors.Cause(err). What does errors.Cause(err) return when err is an errcode.Errors?
I would have expected logic that iterates the array and unwraps each err within the array that needs unwrapping.
There was a problem hiding this comment.
errors.Cause() looks at the error that err wraps, repeating until it reaches an error that isn't a wrapper (i.e., an object that doesn't provide a Cause() method), and errcode.Errors does not provide such a method, so if it's an errcode.Errors, and one of the errors in its list is an "unauthorized" error that we recognize, we're removing the layers of wrapping around the errors list.
There was a problem hiding this comment.
ok.....so if action() returned an errors list or a wrapped errors list, you will scan the errors list, see if it contains an "unauthorized error", and then attempt to unwrap the list (leaving the list itself).
if that's the intent (as compared w/ unwrapping the unauthorized error specifically), then this lgtm
More from bug #1703262: if the error we get back from attempting to push or pull an image is a multi-error, process each component error as we would a single error. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, nalind, rhatdan 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 |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
If the error we get back from attempting to push or pull an image is a multi-error, extend our change from #66 to process each of its component errors as we would have a single error.