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

Implement custom plugin indexes #566

Closed
22 tasks done
chriskim06 opened this issue Mar 29, 2020 · 13 comments
Closed
22 tasks done

Implement custom plugin indexes #566

chriskim06 opened this issue Mar 29, 2020 · 13 comments

Comments

@chriskim06
Copy link
Member

chriskim06 commented Mar 29, 2020

Here's a list of some of the remaining work regarding support for multiple indexes:

Proposal issue: #483
Proposal doc: link

/area multi-index

@ahmetb
Copy link
Member

ahmetb commented Mar 30, 2020

I'm scratching off these two for now as I see no work necessary there.

update krew install to work with multi-index (no work needed?)
update krew remove to work with multi-index (no work needed?)

"install" already works (correct me if I'm wrong), and "remove" has no notion of indexes (as it does not need to).

@chriskim06
Copy link
Member Author

There was a small change required for install that was taken care of here. As for uninstall, that might still be pending. If we want to support krew uninstall foo/plugin that'll require a small change.

@corneliusweig
Copy link
Contributor

I think what @chriskim06 is referring to is that plugins installed via --manifest now set a detached index name. I don't think this is worth mentioning though, because it only affects plugin developers and the change is primarily cosmetic.

On the other hand, the uninstall question is still pending so let's not tick it off :)

@chriskim06
Copy link
Member Author

Sorry I should have been more explicit (also when I click the link I added it doesn't take me to the line that I linked, I have to scroll up a bit for some reason), I was referring more to how a plugin is loaded when its being installed: indexscanner.LoadPluginByName(paths.IndexPluginsPath(indexName), pluginName). We used to have indexscanner.LoadPluginByName(paths.IndexPluginsPath(constants.DefaultIndexName), pluginName) so things could only be installed from the default index until the arg passed to IndexPluginsPath was changed to use the index name passed by the user.

But this is pretty minor, tbh I don't even remember adding those 2 to this list 😅 so I think its totally fine to not include the install one on the list.

@ahmetb
Copy link
Member

ahmetb commented Apr 1, 2020

I don't even remember adding those 2 to this list 😅

I'm editing the list above as we go. :)

@ahmetb
Copy link
Member

ahmetb commented Apr 29, 2020

@chriskim06 I've checked off a few recently completed items above. Please take a look if we're up to date.

@chriskim06
Copy link
Member Author

chriskim06 commented Apr 29, 2020

So I linked your most recent PR to this bullet:

rethink how the "local plugin index is not initialized" message should read, or how we suggest users to add krew-index if there are no indexes? (#595)

Does that seem reasonable to check off since that was merged or did you have something else in mind?

Besides that the list looks up to date to me!

@chriskim06
Copy link
Member Author

chriskim06 commented Jul 15, 2020

@ahmetb for this bullet:

write integration tests (like receiptsmigration) to ensure index migration happens v0.3.4 -> x

did you mean something like the verify-receipts-upgrade-migration.sh step of the ci workflow or just like standard cmd line integration tests?

@ahmetb
Copy link
Member

ahmetb commented Jul 15, 2020

Yeah similar to that script. We'd install old krew, install a couple of plugins, and then run the binary compiled as part of the build. Upon doing so, we verify (1) .krew/index/default/ exists (2) "krew index list" cmd works.

Don't spend too much effort, as these are temporary. (Btw if we removed receipsmigration, we should remove that test, too)

@chriskim06
Copy link
Member Author

Sounds good. I'll try to get to that soon. And ya I removed that script and the step in the ci

@ahmetb
Copy link
Member

ahmetb commented Aug 24, 2020

/close
N I C E

@k8s-ci-robot
Copy link
Contributor

@ahmetb: Closing this issue.

In response to this:

/close
N I C E

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@corneliusweig
Copy link
Contributor

Great work @chriskim06! Thanks for pushing krew forward. 🍾

corneliusweig added a commit to corneliusweig/org that referenced this issue Oct 3, 2020
Chris has been taking on a pretty active role in Krew over the past 6 months. He was the primary driver of implementing decentralized plugin repositories in krew ([meta-issue](kubernetes-sigs/krew#566), [issues & PRs](https://github.com/kubernetes-sigs/krew/issues?q=label%3Aarea%2Fmulti-index+author%3Achriskim06+)).
Since the successful launch of this feature he continued his work on krew through PR reviews and [fixes](https://github.com/kubernetes-sigs/krew/issues?q=author%3Achriskim06+created%3A%3E%3D2020-08-19+).

Chris should therefore have maintainer rights for krew, so that he can continue his work more efficiently.

@chriskim06
@ahmetb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants