WIP: cli: return distinct error codes depending on copy failure #1062
Conversation
The `oc-mirror` return code now reflects the category of the copy failure: * 1: generic error * 2: release image error * 4: operator image error * 8: helm image error * 16: additional image error So a return code of `12` means there were operator and helm image copy errors (4 + 8). Because release image errors are fatal, when one happens the return code will always be `2`.
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: r4f4 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@r4f4: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| @@ -20,3 +20,16 @@ func (e *NormalStorageInterruptError) Is(err error) bool { | |||
| _, ok := err.(*NormalStorageInterruptError) | |||
| return ok | |||
| } | |||
|
|
|||
| type ExecutorSchemaError struct { | |||
There was a problem hiding this comment.
@r4f4 - Nice 👍 for wrapping the exitCode in a struct
|
@r4f4 - Could you create an OCPBUGS issue so that we can backport to 4.18 ? @kasturinarra Could you look creating an automated test matrix to cover the various error exit scenarios's, also could you include Nidan ? |
| genericErrorCode = 1 << 0 | ||
| releaseImageErrorCode = 1 << 1 | ||
| operatorErrorCode = 1 << 2 | ||
| helmImageErrorCode = 1 << 3 | ||
| additionalImageErrorCode = 1 << 4 |
There was a problem hiding this comment.
+1 for the Bitwise operation.
This approach will allows oc-mirror to combine multiple error codes.
Big win here.
|
|
||
| defer o.closeAll() | ||
| return nil | ||
| return err |
There was a problem hiding this comment.
+1 here, since error zero value is nil :)
| if !o.Opts.IsDryRun { | ||
| err = o.RebuildCatalogs(cmd.Context(), collectorSchema) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| var copiedSchema v2alpha1.CollectorSchema | ||
| // call the batch worker | ||
| if cs, err := o.Batch.Worker(cmd.Context(), collectorSchema, *o.Opts); err != nil { | ||
| if copiedSchema, errorredSchema, err = o.Batch.Worker(cmd.Context(), collectorSchema, *o.Opts); err != nil { | ||
| if _, ok := err.(batch.UnsafeError); ok { |
There was a problem hiding this comment.
Since the way oc-mirror is going to handle errors is different now, maybe this batch.UnsafeError/batch.SafeError is not needed anymore.
There was a problem hiding this comment.
Yeah, I'll see if I can simplify this part. I tried to keep the existing error because of the possible errors when creating the errors file.
| @@ -188,7 +188,7 @@ func (o DeleteImages) DeleteRegistryImages(deleteImageList v2alpha1.DeleteImageL | |||
|
|
|||
| o.Opts.Stdout = io.Discard | |||
| if !o.Opts.Global.DeleteGenerate && len(o.Opts.Global.DeleteDestination) > 0 { | |||
There was a problem hiding this comment.
Is the executorErrorFromBatchError needed to be called here? Or is it going to be called by the executor.go also for the delete command?
There was a problem hiding this comment.
Do we also want to differentiate errors for the delete command? Or is delete just a simple "either we deleted all or we failed"?
There was a problem hiding this comment.
@r4f4 - IMHO delete is a sub-command and if it fails we know delete fails, so it's a simple pass/fail and no need to differentiate errors
There was a problem hiding this comment.
So based on @lmzuccarelli answer only having a genericErrorCode is enough for the delete
Description
The
oc-mirrorreturn code now reflects the category of the copy failure:So a return code of
12means there were operator and helm image copy errors (4 + 8).Because release image errors are fatal, when one happens the return code will always be
2.Github / Jira issue:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Expected Outcome
Exit codes other than
1.