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

Remove multi index flag #624

Merged

Conversation

chriskim06
Copy link
Member

Changes:

  • remove the multi index flag
  • update the install help message with an example for installing from a custom index
  • update integration test function WithDefaultIndex to create an index in the migrated layout

Fixes #597
Related issue: #566

/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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 24, 2020
@chriskim06 chriskim06 mentioned this pull request Jul 24, 2020
22 tasks
Comment on lines 57 to 58
To install one or multiple plugins from a custom index, run:
kubectl krew install INDEX/NAME [INDEX/NAME...]
Copy link
Member

Choose a reason for hiding this comment

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

let's move this after the next example

Copy link
Member

@ahmetb ahmetb left a comment

Choose a reason for hiding this comment

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

Looks good.

@ahmetb
Copy link
Member

ahmetb commented Jul 24, 2020

/lgtm
/approve

image

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2020
@k8s-ci-robot
Copy link
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

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 24, 2020
@k8s-ci-robot k8s-ci-robot merged commit 7ba6715 into kubernetes-sigs:master Jul 24, 2020
@chriskim06 chriskim06 deleted the remove-multi-index-flag branch July 24, 2020 18:38
@ahmetb
Copy link
Member

ahmetb commented Jul 24, 2020

So far this went fine for me on master.

Something I noticed is we print the migration messages as Info (user-visible).
Was that our intention? It doesn't bug me and it's also one-time, so maybe it's ok.

krew git:(master) $ tree -d ~/.krew/index
/Users/ahmetb/.krew/index
└── plugins
1 directory
krew git:(master) $ hack/make-binary.sh
Building binaries for: darwin/amd64
(Stamping with git tag=v0.3.4-78-g7ba6715 rev=7ba6715)

Number of parallel builds: 15
-->    darwin/amd64: sigs.k8s.io/krew/cmd/krew
krew git:(master) $ out/bin/krew-darwin_amd64 --help
krew is the kubectl plugin manager.
You can invoke krew through kubectl: "kubectl krew [command]..."

Usage:
  kubectl krew [command]

Available Commands:
  help        Help about any command
  index       Manage custom plugin indexes
  info        Show information about an available plugin
  install     Install kubectl plugins
  list        List installed kubectl plugins
  search      Discover kubectl plugins
  uninstall   Uninstall plugins
  update      Update the local copy of the plugin index
  upgrade     Upgrade installed plugins to newer versions
  version     Show krew version and diagnostics

Flags:
  -h, --help      help for krew
  -v, --v Level   number for the log level verbosity

Use "kubectl krew [command] --help" for more information about a command.
krew git:(master) $ out/bin/krew-darwin_amd64 list
I0724 12:01:24.987470   63847 migration.go:41] Migrating krew index layout.
I0724 12:01:24.987938   63847 migration.go:58] Migration completed successfully.
PLUGIN                 VERSION
access-matrix          v0.4.4
ctx                    v0.9.1
detached/cert-manager  v0.16.0-alpha.0
detached/chonk         v0.1.1
detached/example       v1.0.0
[...]
krew git:(master) $ tree -d ~/.krew/index
/Users/ahmetb/.krew/index
└── default
    └── plugins

2 directories

@ahmetb
Copy link
Member

ahmetb commented Jul 24, 2020

Actually I think I found a bug.

After migration, I ran a out/bin/krew-darwin_amd64 upgrade and got errors so I immediately terminated:

Updated the local copy of plugin index.
Upgrading plugin: access-matrix
Skipping plugin access-matrix, it is already on the newest version
WARNING: failed to upgrade plugin "detached/", skipping (error: open /Users/ahmetb/.krew/index/detached/plugins/cert-manager.yaml: no such file or directory)
WARNING: failed to upgrade plugin "detached/", skipping (error: open /Users/ahmetb/.krew/index/detached/plugins/chonk.yaml: no such file or directory)
Upgrading plugin: ctx
Skipping plugin ctx, it is already on the newest version
WARNING: failed to upgrade plugin "detached/", skipping (error: open /Users/ahmetb/.krew/index/detached/plugins/example.yaml: no such file or directory)
Upgrading plugin: fleet
Upgraded plugin: fleet
WARNING: You installed plugin "fleet" from the krew-index plugin repository.
   These plugins are not audited for security by the Krew maintainers.
   Run them at your own risk.
Upgrading plugin: get-all
Skipping plugin get-all, it is already on the newest version
Upgrading plugin: gs
^C

@chriskim06
Copy link
Member Author

Ya that one is because when you install via manifest it gets the detached index. Then when upgrade with no args happens this line causes it to try to upgrade from a non existent index.

@chriskim06
Copy link
Member Author

chriskim06 commented Jul 24, 2020

I thought we agreed on that when we added the detached index

I was looking for something about that but couldn't find anything. What should the behavior for that be? I don't think we can assume that plugins installed from manifest are necessarily upgraded from krew-index.

@ahmetb
Copy link
Member

ahmetb commented Jul 24, 2020

I don't think we can assume that plugins installed from manifest are necessarily upgraded from krew-index.

We can.

Where else would they be installed from at the time of the migration from a time where we only allowed 1 index? (They could indeed be installed manually with --archive|--manifest?)

We definitely should not suddenly orphan all plugins. Most users have >0 plugins installed. And likely <1% of those are actually "detached". We definitely should not assume all is "detached", that would break ~everyone for no reason.

@ahmetb
Copy link
Member

ahmetb commented Jul 24, 2020

Actually, in my case those plugins chonk, cert-manager and example were definitely "detached" at the time of installation.

But the message is confusing:

WARNING: failed to upgrade plugin "detached/", skipping (error: open /Users/ahmetb/.krew/index/detached/plugins/chonk.yaml: no such file or directory)

  • It tells me plugin name is detached/
  • It tells me there is supposed to be a dir /Users/ahmetb/.krew/index/detached

that’ll be confusing (to a very small % of the users who install detached plugins)

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/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.

Update help messages to reflect custom index syntax
3 participants