Skip to content

Skip upgrades for plugins installed via manifest - #626

Merged
k8s-ci-robot merged 2 commits into
kubernetes-sigs:masterfrom
chriskim06:detached-upgrade
Jul 28, 2020
Merged

k8s-ci-robot merged 2 commits into
kubernetes-sigs:masterfrom
chriskim06:detached-upgrade

Conversation

@chriskim06

Copy link
Copy Markdown
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
Comment thread cmd/krew/cmd/upgrade.go Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@ahmetb ahmetb left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

Details 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

ahmetb commented Jul 28, 2020

Copy link
Copy Markdown
Member

/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