Return err when there is a failure upgrading #1056
Conversation
manuelbuil
requested review from
ereslibre,
innobead and
vadorovsky
and removed request for
ereslibre
|
This changes the behavior. Previously even if an addon failed to apply, the remaining addons would still be applied. With this change, it stops at the first failed addon. I don't know if this would be a problem but perhaps it should be checked and mentioned? |
|
ping @ereslibre |
I agree this is a change in behavior. Perhaps what we can do is to enrichen the type returned by This way I think we could still keep the same behavior by iterating over all addons, and still give meaningful feedback to the user. WDYT? |
|
This PR is important to have, at least I encounter this issue that partial addons install failed but CLI still bumps out bootstrapped successfully. |
|
BTW I think it's fine to fail at the first failure. It's not perfect, but it's fine. I think this needs to pass testing though ;) |
|
@manuelbuil Could you please rebase to the latest master branch in order to let CI pass, thx. |
manuelbuil
dismissed stale reviews from c3y1huang, innobead, and vadorovsky
via
9fa2912
Currently we are logging the error but not returning error. As a consequence, the upgrade might fail but we still get the successful message. For example: E0422 08:25:36.316862 19710 addons.go:334] failed to run kubectl delete: error: the path "/tmp/skuba-addon-cilium478546698/base/cilium-preflight.yaml" does not exist E0422 08:25:36.317170 19710 addons.go:170] failed to apply "cilium" addon (exit status 1) [apply] Successfully upgraded addons Signed-off-by: Manuel Buil <mbuil@suse.com>
done. Is it ok to merge it? |
Currently we are logging the error but not returning error. As a consequence, the upgrade might fail but we still get the successful message. For example: E0422 08:25:36.316862 19710 addons.go:334] failed to run kubectl delete: error: the path "/tmp/skuba-addon-cilium478546698/base/cilium-preflight.yaml" does not exist E0422 08:25:36.317170 19710 addons.go:170] failed to apply "cilium" addon (exit status 1) [apply] Successfully upgraded addons Signed-off-by: Manuel Buil <mbuil@suse.com>
Currently we are logging the error but not returning error. As a
consequence, the upgrade might fail but we still get the successful
message. For example:
E0422 08:25:36.316862 19710 addons.go:334] failed to run kubectl delete: error: the path "/tmp/skuba-addon-cilium478546698/base/cilium-preflight.yaml" does not exist
E0422 08:25:36.317170 19710 addons.go:170] failed to apply "cilium" addon (exit status 1)
[apply] Successfully upgraded addons
Signed-off-by: Manuel Buil mbuil@suse.com
Why is this PR needed?
Fixing a bug.
Currently we are logging the error but not returning error. As a
consequence, the upgrade might fail but we still get the successful
message. For example:
E0422 08:25:36.316862 19710 addons.go:334] failed to run kubectl delete: error: the path "/tmp/skuba-addon-cilium478546698/base/cilium-preflight.yaml" does not exist
E0422 08:25:36.317170 19710 addons.go:170] failed to apply "cilium" addon (exit status 1)
[apply] Successfully upgraded addons
What does this PR do?
Adds a return err. Currently we were only logging it
Anything else a reviewer needs to know?
Special test cases, manual steps, links to resources or anything else that could be helpful to the reviewer.
Status BEFORE applying the patch
Run an upgrade that fails and you'll see the success message
Status AFTER applying the patch
If the upgrade fails, we should see:
"[apply] Failed to deploy addons"
Merge restrictions
(Please do not edit this)
We are in v4-maintenance phase, so we will restrict what can be merged to prevent unexpected surprises: