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

Add support for custom indices #478

Closed
wants to merge 18 commits into from

Conversation

chriskim06
Copy link
Member

@chriskim06 chriskim06 commented Jan 28, 2020

I'm in the process of adding tests currently. I wanted to open the PR to write down the approach and maybe get some feedback. So here's a summary of the changes:

  • added a new index subcommand
    • kubectl krew index: lists the configured custom indices
    • kubectl krew index add custom-index https://github.com/custom/index.git: adds a custom index with the name custom-index
    • kubectl krew index remove custom-index: removes the custom index with the name custom-index
  • a file is stored at $KREW_ROOT/indexconfig.yaml that contains an object whose key/values are a user defined name and URI of the index being added
  • custom indices are cloned to $KREW_ROOT/custom/{index name}
  • plugins from custom indices are installed in $KREW_ROOT/store/krew-indices/{index name}/{plugin}
  • plugin receipts for plugins installed from a custom index are stored $KREW_ROOT/receipts/{index name}/{plugin}.yaml
  • plugins from custom indices need to be explicitly installed: kubectl krew install custom-index/plugin (this is similar to how its handled in homebrew; a follow up issue could be to handle plugin name collisions, but for this I only implemented allowing one or the other)
  • list/search add plugins from custom indices to the end of the list:
$ kubectl krew list
PLUGIN           VERSION
ctx              v0.7.1
grep             v1.2.2
kudo             v0.10.0
ns               v0.7.1
whoami           v0.0.29
private/warp     v0.0.2

$ kubectl krew search
NAME                                    DESCRIPTION                                         INSTALLED
...
who-can                                 Shows who has RBAC permissions to access Kubern...  no
whoami                                  Show the subject that's currently authenticated...  yes
private/access-matrix                   Show an RBAC access matrix for server resources     no
private/auth-proxy                      Authentication proxy to a pod or service            no
private/warp                            Sync and execute local files in Pod                 yes
...
  • update/upgrade have been modified to look in the correct directory for plugins
    • plugins from custom indices must be explicit when upgrading them specifically: kubectl krew upgrade custom-index/plugin
    • no arg upgrade will look in the correct directory for plugins that were installed from custom indices

I think that covers most things at a high level. I took a lot of ideas from brew tap in working on this feature. Its definitely a rough first draft and I would love to get some feedback. I chose to do things in certain ways to try to make it as backwards compatible as possible. This is a really important feature in order for me to use this at work (which I'd love to do).

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 28, 2020
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 28, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chriskim06
To complete the pull request process, please assign ahmetb
You can assign the PR to them by writing /assign @ahmetb in a comment when ready.

The full list of commands accepted by this bot can be found 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

@codecov-io
Copy link

Codecov Report

Merging #478 into master will decrease coverage by 1.62%.
The diff coverage is 15.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #478      +/-   ##
==========================================
- Coverage   56.47%   54.85%   -1.63%     
==========================================
  Files          19       19              
  Lines         926      957      +31     
==========================================
+ Hits          523      525       +2     
- Misses        350      377      +27     
- Partials       53       55       +2
Impacted Files Coverage Δ
internal/installation/upgrade.go 0% <0%> (ø) ⬆️
internal/installation/install.go 37.08% <0%> (-5.02%) ⬇️
internal/environment/environment.go 67.39% <42.1%> (-20.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21fa51e...7616c66. Read the comment docs.

@ahmetb
Copy link
Member

ahmetb commented Jan 28, 2020

@chriskim06 While I totally appreciate you writing code, it's most likely that this has been the BIGGEST item on our list since the inception of Krew.

We didn't discuss this at all and it's been on our list to design this first.

Code is not important (yet). So I will close this PR, and ask you to open an issue with a Design proposal first.

We care about:

  • user experience
  • developer experience
  • migration path

more than the code. Let's discuss a design first, then we can revisit your patch.

In Krew project, it's very unlikely we'll accept non-trivial patches of this size without discussing how it changes the user experience first.

I'm hoping you're not surprised about this, you didn't let the maintainers know you're taking on this challenge.

/close

@k8s-ci-robot
Copy link
Contributor

@ahmetb: Closed this PR.

In response to this:

@chriskim06 While I totally appreciate you writing code, it's most likely that this is the BIGGEST work item since the inception of Krew.

We didn't discuss this at all and it's been on our list to design this first. Code is not important (yet). So I will close this PR, and ask you to open an issue with a Design proposal first.

We care about:

  • user experience
  • developer experience
  • migration path

more than the code. Let's discuss a design first, then we can revisit your patch.

In Krew project, it's very unlikely we'll accept non-trivial patches of this size without discussing how it changes the user experience first.

/close

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.

@chriskim06
Copy link
Member Author

@ahmetb that makes sense, I'll open up a new issue with a more in depth design proposal. Sorry for doing these things out of order 😅

@chriskim06
Copy link
Member Author

@ahmetb I'm not at all surprised, definitely knew it was possible for this to get closed when I opened the PR. I'm really looking forward to being able to use krew at work but custom indices is a must before that can happen (I'm sure that's the case for a lot of people).

I'm still new to open source so sorry again if I'm going about things out of order/in a way that isn't conventional. Lesson learned though, in the future I will try to communicate these things through issues to you and the other maintainers!

@ahmetb
Copy link
Member

ahmetb commented Jan 28, 2020

That's not at all a problem.
Please open a new issue with your proposal and refer to #23. We haven't collected enough use cases about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants