Return more detailed errors when fixup fails #113
Conversation
remotes/fixup.go
Outdated
| } | ||
| if ok { | ||
| return info, pushed, nil | ||
| } | ||
| } | ||
|
|
||
| return imageFixupInfo{}, false, fmt.Errorf("failed to resolve or push image for service %q", name) | ||
| return imageFixupInfo{}, false, fmt.Errorf("failed to resolve or push image %s for service %q", baseImage.Image, name) |
There was a problem hiding this comment.
This is the one error message that I'm not clear on how we actually get into this situation and what else we could include in the message to help someone solve the problem.
When cnab-to-oci fixes up a bundle after publishing it, if any error occurs we only get a single error message "failed to fixup the image for service InvocationImage" This bumps our cnab-to-oci to use the patch in cnabio/cnab-to-oci#113 so that we get a more detailed error message that indicates the image that failed, and which fixup step. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
When cnab-to-oci fixes up a bundle after publishing it, if any error occurs we only get a single error message "failed to fixup the image for service InvocationImage" This bumps our cnab-to-oci to use the patch in cnabio/cnab-to-oci#113 so that we get a more detailed error message that indicates the image that failed, and which fixup step. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
remotes/fixup.go
Outdated
| @@ -220,59 +220,69 @@ func fixupBaseImage(ctx context.Context, name string, baseImage *bundle.BaseImag | |||
| info, pushed, ok, err := f(ctx, targetRepoOnly, baseImage, cfg) | |||
| if err != nil { | |||
| log.G(ctx).Debug(err) | |||
| return imageFixupInfo{}, false, fmt.Errorf("failed to fixup the image %s for service %q: %v", baseImage.Image, name, err) | |||
There was a problem hiding this comment.
If I read this correctly, this returns early if there is an error pushing any image. Is this the intended behavior? Do we want to keep pushing and then return an error?
There was a problem hiding this comment.
Honestly it's super hard to follow the logic for how the fixups should be applied. Can you help clarify so that I can better match the desired behavior here?
It looks like as soon as one fixup returns "ok=true" (not sure what ok indicates though), then we stop applying fixups. Is it expected that a fixup will fail with an error and we should keep trying other fixups?
I could instead keep a multierror and only return it when none of the fixups return ok (i.e. in that final return statement with the ambiguous error). What do you think?
There was a problem hiding this comment.
@radu-matei Any thoughts on my question above?
It's not possible from the current error to tell what went wrong since the real error is only sent to the debug logger, and isn't included in the returned error message. I have updated the errors returned from each fixup function so that you can tell where the fixup failed from the error message. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
When applying fixups to an image, accumulate any errors and only return an error if the image ultimately was unable to be pushed. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
carolynvs
force-pushed
the
return-copy-error
branch
from
01953d7
to
b9df3a4
Compare
It's not possible from the current error to tell what went wrong since the real error is only sent to the debug logger, and isn't included in the returned error message.
I have updated the errors returned from each fixup function so that you can tell where the fixup failed from the error message.
❓ There is still one message that I didn't know how to improve. If all the fixups complete, but nothing was pushed, then when we finish iterating over the fixups, a generic error is returned. I'm not sure what would cause that situation so I kept the error message as-is, but when you run into that error, it's too vague to help narrow down the problem.