Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

False upgrade success message if an installed plugin no longer matches a platform spec #423

Closed
ahmetb opened this issue Dec 3, 2019 · 1 comment · Fixed by #427
Closed

False upgrade success message if an installed plugin no longer matches a platform spec #423

ahmetb opened this issue Dec 3, 2019 · 1 comment · Fixed by #427
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/P2 P2 issues or PRs

Comments

@ahmetb
Copy link
Member

ahmetb commented Dec 3, 2019 •
edited
Loading

Discovered through #421

In case of:

  • A user installed a plugin, and its platform is removed from the corresponding manifest.

For example, I removed darwin platform of ctx plugin and ran:

$ kubectl krew upgrade ctx --no-update-index  -v=10
I1203 15:16:51.666127   66174 upgrade.go:81] --no-update-index specified, skipping updating local copy of plugin index
I1203 15:16:51.666209   66174 scanner.go:82] Reading plugin "ctx"
I1203 15:16:51.667079   66174 upgrade.go:65] Upgrading plugin: ctx
I1203 15:16:51.667286   66174 platform.go:43] Matching platform for labels(arch=amd64,os=darwin)
Upgraded plugin: ctx

It definitely didn't do an upgrade, but prints a message saying so.
/kind bug

I think we should skip upgrade (no fail) with reason like (plugin doesn't have distribution for platform $GOOS/$GOARCH)
@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 3, 2019
@corneliusweig corneliusweig added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Dec 4, 2019
@ahmetb ahmetb added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Dec 4, 2019
@ahmetb
Copy link
Member Author

ahmetb commented Dec 4, 2019

/priority P2
/kind bug
/good-first-issue

@k8s-ci-robot k8s-ci-robot added the priority/P2 P2 issues or PRs label Dec 4, 2019
ahmetb added a commit to ahmetb/krew that referenced this issue Dec 5, 2019
@ahmetb
skip/fail upgrade if no longer matches a platform
42954b1 
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]>
@ahmetb ahmetb mentioned this issue Dec 5, 2019
Merged
ahmetb added a commit to ahmetb/krew that referenced this issue Dec 5, 2019
@ahmetb
skip/fail upgrade if no longer matches a platform
3499656 
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]>
ahmetb added a commit to ahmetb/krew that referenced this issue Dec 5, 2019
@ahmetb
skip/fail upgrade if no longer matches a platform
0ee10ab 
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]>
k8s-ci-robot pushed a commit that referenced this issue Dec 7, 2019
@ahmetb @k8s-ci-robot
skip/fail upgrade if no longer matches a platform (#427)
a6ac816 
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.

Signed-off-by: Ahmet Alp Balkan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/P2 P2 issues or PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

skip/fail upgrade if no longer matches a platform ahmetb/krew
3 participants
@ahmetb @k8s-ci-robot @corneliusweig