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.

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

Merged
merged 2 commits into from
Jul 28, 2020

Conversation

chriskim06
Copy link
Member

Fixes #625

/area multi-index
/cc @ahmetb
/cc @corneliusweig

@k8s-ci-robot k8s-ci-robot added area/multi-index cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 28, 2020
@@ -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)
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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).

Copy link
Member Author

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

Copy link
Member

@ahmetb ahmetb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2020
@ahmetb
Copy link
Member

ahmetb commented Jul 28, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2020
@k8s-ci-robot k8s-ci-robot merged commit f90ce36 into kubernetes-sigs:master Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/multi-index cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not friendly "upgrade" message for detached plugins
3 participants