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

cmd: modify upgrade to work with multiple indexes #594

Merged
merged 6 commits into from
Apr 29, 2020
Merged

cmd: modify upgrade to work with multiple indexes #594

merged 6 commits into from
Apr 29, 2020

Conversation

chriskim06
Copy link
Member

@chriskim06 chriskim06 commented Apr 16, 2020 •
edited

Upgrade was already refactored to support adding indexes to receipts so this was a pretty simple change + integration test. Upgrade works with:

  • kubectl krew upgrade
  • kubectl krew upgrade plugin ... // plugin can be default/plugin or foo/plugin

Also added warning text to uninstall when user tries to uninstall by canonical name.

Related issue: #566
/area multi-index

@k8s-ci-robot k8s-ci-robot added area/multi-index cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 16, 2020
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 16, 2020
@chriskim06 chriskim06 mentioned this pull request Apr 16, 2020
Closed
22 tasks
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 17, 2020
@ahmetb
Copy link
Member

ahmetb commented Apr 17, 2020

This is kinda like #579 (should we do krew uninstall foo or krew uninstall myindex/foo). In this case, only 1 plugin with a particular name can be installed at any time. So is kubectl krew upgrade foo/plugin syntax actually the right call here? (Note, this creates a bit asymmetry with uninstall command.)

@chriskim06
Copy link
Member Author

Oh right, I forgot that we decided on that in that issue. So just to be clear, we're going to do krew upgrade plugin to refer to either default/plugin or foo/plugin right?

@ahmetb
Copy link
Member

ahmetb commented Apr 17, 2020

Oh right, I forgot that we decided on that in that issue. So just to be clear, we're going to do krew upgrade plugin to refer to either default/plugin or foo/plugin right?

Yeah, whatever’s installed. Upgrade machinery should be primarily looking at the install receipts to figure out what that name resolves to.

Also, I was hoping to add a check to uninstall to remind people not to use / in the specified name. You can do it here as well.

@chriskim06
Copy link
Member Author

chriskim06 commented Apr 17, 2020 •
edited

I kinda misread what you meant about uninstall having that warning. For some reason I thought you meant you wanted me to add that in this PR but I realize you meant to add the same type of warning to upgrade. Let me know if you want me to remove that from this PR.

@ahmetb
Copy link
Member

ahmetb commented Apr 18, 2020

I think it’s fine to update. But I think we should return it as err not warning. That way we can also test it.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 18, 2020
corneliusweig
corneliusweig approved these changes Apr 18, 2020
View reviewed changes
Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

/approve
Just minor nits from here. Nice work!

cmd/krew/cmd/upgrade.go Outdated
Comment on lines 65 to 67
if isCanonicalName(arg) {
return errors.New("upgrade command does not support INDEX/PLUGIN syntax; just specify PLUGIN")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move that right before the enclosing if

cmd/krew/cmd/upgrade.go Outdated
}
r, err := receipt.Load(paths.PluginInstallReceiptPath(arg))
if err != nil {
return errors.Wrapf(err, "receipt not found for %q", arg)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we don't know what the error is

Suggested change
return errors.Wrapf(err, "receipt not found for %q", arg)
return errors.Wrapf(err, "read receipt %q", arg)

cmd/krew/cmd/uninstall.go Outdated
Comment on lines 43 to 45
if isCanonicalName(name) {
return errors.New("uninstall command does not support INDEX/PLUGIN syntax; just specify PLUGIN")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move in right before the enclosing if.

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

Any final reviews would be much appreciated when you guys get some spare time 😄

ahmetb
ahmetb reviewed Apr 23, 2020
View reviewed changes
cmd/krew/cmd/upgrade.go Outdated
Comment on lines 66 to 67
}
if !validation.IsSafePluginName(arg) {
Copy link
Member

Choose a reason for hiding this comment

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

can do else if here

@corneliusweig
Copy link
Contributor

/approve
from my side. Sorry for being so sluggish to respond. Please do @ me, if you want quicker feedback 🙏

@ahmetb
Copy link
Member

ahmetb commented Apr 29, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, chriskim06, 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:
  • OWNERS [ahmetb,corneliusweig]

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 merged commit 6419102 into kubernetes-sigs:master Apr 29, 2020
@chriskim06 chriskim06 deleted the upgrade-multi-index branch June 11, 2020 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahmetb ahmetb ahmetb left review comments

@corneliusweig corneliusweig corneliusweig approved these changes

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@ahmetb ahmetb

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chriskim06 @ahmetb @corneliusweig @k8s-ci-robot