skip/fail upgrade if no longer matches a platform #427
Conversation
k8s-ci-robot
added
cncf-cla: yes
It turns out when upgrade process looks to a plugin manifest that exists but does no longer have a matching Platform spec, we were returning nil error, which was surfacing as "upgraded" (see kubernetes-sigs#423). This addresses the problem by returning an error in this case. However, the 'upgrade' command fails with this error if a plugin list is given to the cmd. If 'krew upgrade' is called without args (i.e. upgrade all plugins), ensuring this doesn't fail the cmd, but prints warnings (just like current 'Skipping' behavior this cmd has). I think this 'skipping with a warning' behavior is justified because it can make upgrade command entirely malfunctional when a plugin manifest is updated breaking the upgrades for that plugin. (There are still some cases that make 'upgrade' command fatally fail, such as an installed plugin gone from manifest, which we should address separately.) Adding integration tests to capture both cases. Signed-off-by: Ahmet Alp Balkan <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #427 +/- ##
==========================================
- Coverage 56.41% 56.35% -0.07%
==========================================
Files 19 19
Lines 927 928 +1
==========================================
Hits 523 523
- Misses 349 350 +1
Partials 55 55
Continue to review full report at Codecov.
|
|
/lgtm and LGTM 😄 |
|
@ferhatelmas: changing LGTM is restricted to collaborators In response to this:
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/test-infra repository. |
| test.WithEnv("KREW_OS", "somethingelse") | ||
|
|
||
| // if upgrading 'all' plugins, must succeed | ||
| out := string(test.Krew("upgrade", "--no-update-index").RunOrFailOutput()) |
There was a problem hiding this comment.
👍 for using the --no-update-index option. Should we add this to other integration tests to speed them up?
There was a problem hiding this comment.
Yeah not a bad idea, though worth seeing how much it speeds up.
My motive here was to not to cause an index update b/c I was gonna modify the manifest file, but then i realized I can just override env.
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, corneliusweig 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 |
It turns out when upgrade process looks to a plugin manifest that exists
but does no longer have a matching Platform spec, we were returning nil error,
which was surfacing as "upgraded" (see #423).
This addresses the problem by returning an error in this case. However, the
'upgrade' command fails with this error if a plugin list is given to the cmd.
If 'krew upgrade' is called without args (i.e. upgrade all plugins), ensuring
this doesn't fail the cmd, but prints warnings (just like current 'Skipping'
behavior this cmd has).
I think this 'skipping with a warning' behavior is justified because it can
make upgrade command entirely malfunctional when a plugin manifest is updated
breaking the upgrades for that plugin.
(There are still some cases that make 'upgrade' command fatally fail, such as
an installed plugin gone from manifest, which we should address separately.)
Adding integration tests to capture both cases.
Fixes #423.