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

cmd: Add 'index remove' command #552

Merged
merged 7 commits into from
Mar 19, 2020

Conversation

ahmetb
Copy link
Member

@ahmetb ahmetb commented Mar 17, 2020

Fixes #550
Related issue: #545, #483

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2020
@ahmetb
Copy link
Member Author

ahmetb commented Mar 17, 2020

/assign @corneliusweig
cc: @chriskim06 to review as well.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 17, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2020
@ahmetb ahmetb force-pushed the index-delete branch 6 times, most recently from 042f6e4 to 054331f Compare March 17, 2020 05:12
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
cmd/krew/cmd/index.go Outdated Show resolved Hide resolved
cmd/krew/cmd/index.go Outdated Show resolved Hide resolved
cmd/krew/cmd/index.go Outdated Show resolved Hide resolved
cmd/krew/cmd/index.go Outdated Show resolved Hide resolved
cmd/krew/cmd/index.go Outdated Show resolved Hide resolved
cmd/krew/cmd/index.go Show resolved Hide resolved
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
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.

Overall this looks quite good. There are some smaller nits.

And I'd like to discuss how we should treat the --force option. Its very existence is proof that this is supported behavior. If anything, I would prefer talking about "not recommended". Anyway, we should think hard if we really want that. My opinion is -- either support it -- or don't. Right now, we are going both ways which will create problems, because users will make use of --force get lost.
So either we hide --force for the time being, or we make sure that everything else works just fine, if additional plugins are installed. (This will also help us plugin reviewers, just saying :) )

cmd/krew/cmd/index.go Outdated Show resolved Hide resolved
cmd/krew/cmd/index.go Outdated Show resolved Hide resolved
cmd/krew/cmd/index.go Outdated Show resolved Hide resolved
cmd/krew/cmd/index.go Outdated Show resolved Hide resolved
internal/index/indexoperations/index.go Outdated Show resolved Hide resolved
internal/index/indexoperations/index.go Show resolved Hide resolved
integration_test/index_test.go Outdated Show resolved Hide resolved
integration_test/index_test.go Outdated Show resolved Hide resolved
@ahmetb
Copy link
Member Author

ahmetb commented Mar 18, 2020

Its very existence is proof that this is supported behavior. If anything, I would prefer talking about "not recommended". Anyway, we should think hard if we really want that. My opinion is -- either support it -- or don't.

First of all we expect >95% users won't use krew index cmds. So anything we do here is "advanced" land. Furthermore --force says "not recommended" –which means this is even more advanced.

Here's my primary rationale for having --force: Assume you just added Jim’s index –but Jim messed up one of his manifest files, its parsing fails. So, naturally most our primary cmds (install, update, search) start failing. But you have plugins from Jim, so you can just go and "nuke" that index with the consequences.

--force puts you in an "unsupported" land because you might install plugin foo from indexA , then remove --force indexA, install indexB which also has a foo.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@corneliusweig
Copy link
Contributor

Yeah, that all makes sense. I'm also ok with having --force, but even if we don't recommend using it, we should make sure that the core workflow continues to work without hiccups (install, upgrade, update, list, uninstall). I'm mostly concerned about krew upgrade which bails out when finding an unknown plugin at the moment.

So in a way we do support the core workflow after rm --force even though we claim to not support it. Or is that just dumb?

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb
Copy link
Member Author

ahmetb commented Mar 19, 2020

I'm mostly concerned about krew upgrade which bails out when finding an unknown plugin at the moment.

I think there's no reason for us to support all those weird corner cases at the moment. We'll make krew update and krew upgrade tolerant to broken/unreachable indexes –but anything beyond that to make this bulletproof adds SO much complexity and very little in return.

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.

/lgtm
/approve
Nice work!


It is only safe to remove indexes without installed plugins. Removing an index
while there are plugins installed will result in an error, unless the --force
option is used ( not recommended).`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
option is used ( not recommended).`,
option is used (not recommended).`,

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, 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 e918c28 into kubernetes-sigs:master Mar 19, 2020
@chriskim06 chriskim06 mentioned this pull request Mar 29, 2020
22 tasks
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. 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
Development

Successfully merging this pull request may close these issues.

indexoperations methods should take environment.Paths
4 participants