-
Notifications
You must be signed in to change notification settings - Fork 373
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.
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
Skip upgrades for plugins installed via manifest #626
Skip upgrades for plugins installed via manifest #626
Conversation
cmd/krew/cmd/upgrade.go
Outdated
@@ -78,6 +78,11 @@ kubectl krew upgrade foo bar"`, | |||
var nErrors int | |||
for _, name := range pluginNames { | |||
indexName, pluginName := pathutil.CanonicalPluginName(name) | |||
if indexName == "detached" { | |||
fmt.Fprintf(os.Stderr, "Skipping upgrade for %q because it was installed via manifest\n", pluginName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
klog.Warnf might be preferable for our output style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it should be behind some log level then and use Infof
(doesn't seem like Warningf
is available when using levels)? If not the output looks a little strange IMO
Updated the local copy of plugin index.
Updated the local copy of plugin index "local".
Upgrading plugin: local/fields
Skipping plugin local/fields, it is already on the newest version
W0727 17:55:55.211941 25668 upgrade.go:82] Skipping upgrade for "grep" because it was installed via manifest
Upgrading plugin: whoami
Skipping plugin whoami, it is already on the newest version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 yeah we don't have a pretty way of printing warnings.
maybe search the source tree about how other warnings are printed.
we def use klog.warnf in several places and these are mostly visible to plugin developers (exceptions exist).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya that's a good point. This also happens in install
so its probably not that big of a deal, I'll switch it to Warningf
$ kubectl krew install who-can whoami
Updated the local copy of plugin index.
Updated the local copy of plugin index "local".
Installing plugin: who-can
Installed plugin: who-can
\
| Use this plugin:
| kubectl who-can
| Documentation:
| https://github.com/aquasecurity/kubectl-who-can
| Caveats:
| \
| | The plugin requires the rights to list (Cluster)Role and (Cluster)RoleBindings.
| /
/
WARNING: You installed plugin "who-can" from the krew-index plugin repository.
These plugins are not audited for security by the Krew maintainers.
Run them at your own risk.
Installing plugin: whoami
W0727 19:34:59.417519 11450 install.go:160] Skipping plugin "whoami", it is already installed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, chriskim06 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 |
/lgtm |
Fixes #625
/area multi-index
/cc @ahmetb
/cc @corneliusweig