Add subcommand for migrating the krew home #249

corneliusweig · 2019-07-09T20:13:00Z

This PR adds a new subcommand krew system following the logic outlined in #232 (comment).

The basic steps are:

add a check to detect if the krew home needs to be migrated. This check runs before every command.
add migration logic
- build list of plugins by looking at the folders in $KREW_ROOT/store and perform some validation on them
- remove the link in $KREW_ROOT/bin and the folder in $KREW_ROOT/store/<plugin>.
- call kubectl krew install <plugin> if the previous steps were successful
manually copy over the krew.yaml manifest, because that cannot be re-installed (and we know that this is the correct manifest).

Notes:

I'm not 100% happy with the method how we detect an outdated krew home. One major drawback is that new users will have to run the migration as the very first thing.
So far, I didn't implement as many component tests as I normally would. However this is throw-away code and covered by integration tests. So I think this is acceptable.
Because we want to make the migration code independent from the main logic, I duplicated a lot of code (mainly from pkg/installation/install.go). This was intentional.

Close #232

Krew cannot re-install itself if it is uninstalled, so it must be skipped in the standard code path.

ahmetb · 2019-07-10T22:05:53Z

I'll try to find time to review this.

I'm not 100% happy with the method how we detect an outdated krew home. One major drawback is that new users will have to run the migration as the very first thing.

this is ok

So far, I didn't implement as many component tests as I normally would. However this is throw-away code and covered by integration tests. So I think this is acceptable.

we don't need tests for this, but we have to do quite a bit of manual verification.

add migration logic

build list of plugins by looking at the folders in $KREW_ROOT/store and perform some validation on them

remove the link in $KREW_ROOT/bin and the folder in $KREW_ROOT/store/<plugin>.

call kubectl krew install <plugin> if the previous steps were successful

this SGTM. I was assuming we can do krew uninstall but we can’t. Makes sense.

manually copy over the krew.yaml manifest, because that cannot be re-installed (and we know that this is the correct manifest).

hmm this feels hacky. but might be OK.

ahmetb · 2019-07-10T22:07:47Z

cmd/krew/cmd/root.go

@@ -69,7 +79,9 @@ func init() {
 		paths.DownloadPath(),
 		paths.InstallPath(),
 		paths.BinPath(),
-		paths.InstallReceiptPath()); err != nil {
+	// TODO(corneliusweig): re-enable when migration code is removed:
+	//  paths.InstallReceiptPath(),


I think you can still enable it.
We just need to find a different way to detect. An example could be: :the number of directories in store/ are non-zero but *.yamls in receipts/ are zero" is cleaner and doesn't require you to do this.

Yes, perfect. That'll do.

cmd/krew/cmd/system.go

ahmetb · 2019-07-10T22:08:25Z

cmd/krew/cmd/system.go

+
+// systemCmd represents the system command
+var systemCmd = &cobra.Command{
+	Use:   "system receipts-upgrade",


can't we nest subcommands? this seems a bit hacky. we should let cobra handle non-existing subcmds etc for us.

Yes, good idea.

don't forget to change the string :P

🤦‍♂️

cmd/krew/cmd/system.go

ahmetb · 2019-07-10T22:11:11Z

integration_test/system_test.go

+	test.Krew("system", "receipts-upgrade").RunOrFailOutput()
+	test.AssertExecutableInPATH("kubectl-" + validPlugin)
+
+	assertReceiptExistsFor(test, validPlugin)


nit: should this be part of ITest? this way you can do:

test.assertReceiptExistsFor(test, validPlugin) just like test.AssertExecutableInPATH("kubectl-" + validPlugin)

also note that it's inconsistent one is exported, the other one isn't.

That depends on whether we want to keep that assertion method. Also, I would consider receipts more of an implementation detail. It's necessary to assert on them in the current test-case, but should we encourage doing so in other integration tests?

in that case we should add a TODO to all these tests + helpers to remove them after the migration code is scrubbed.

pkg/migration/migration.go

ahmetb · 2019-07-10T22:18:26Z

pkg/migration/migration.go

+		return nil
+	}
+
+	oldPaths := oldenvironment.MustGetKrewPaths()


why do we need oldenvironment package?

Can't we just get KREW_ROOT from the current environment pkg and do filepath.Join(root, "store") etc? This oldenvironment adds a lot of code, I recommend just going for straight brain surgery on version==0.2.x assumption here and "assume" paths.

My thinking was

it eases review, because one can do file-diff.

I don't have to bother about correctness wrt KREW_ROOT or so, because it's all tried and tested.

it raises the abstraction level of the migration code.

Granted, it adds more code than necessary, but it's going to be deleted anyways.

I'm OK with it. Don't forget to add TODOs to the methods and packages we must delete. Granted, we'll probably delete this code in v0.4.

My hope was that we simply revert the whole commit. I find todos on every method a little excessive (especially since I'm not a big fan of todos). Can we take a middle ground where there's only one todo for a whole package, when the package should be deleted, and one todo for a function outside of such packages?

It no longer fails in fresh installs.

codecov-io · 2019-07-11T08:22:41Z

Codecov Report

Merging #249 into master will increase coverage by 0.16%.
The diff coverage is 54.68%.

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
+ Coverage    53.6%   53.76%   +0.16%     
==========================================
  Files          16       18       +2     
  Lines         735      863     +128     
==========================================
+ Hits          394      464      +70     
- Misses        289      342      +53     
- Partials       52       57       +5

Impacted Files	Coverage Δ
pkg/receiptsmigration/migration.go	`48.14% <48.14%> (ø)`
...kg/receiptsmigration/oldenvironment/environment.go	`90% <90%> (ø)`

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 7b2c944...f73ba27. Read the comment docs.

pkg/testutil/tempdir.go

pkg/receiptsmigration/migration.go

ahmetb · 2019-07-11T14:41:35Z

I think at this point we should probably just do manual testing.

- add todos to mark packages and functions for removal - move `Touch` function out of tempdir.go - fix `Use` field in system command - remove obsolete consistency check before uninstall/reinstall - no need to create the receipts directory during migration as this is created by cmd/krew/cmd/root.go:init()

ahmetb · 2019-07-12T23:13:58Z

/approve
I added #255 that addresses the need for a script that ensures as we refactor more parts, the migration works. PTAL.

ahmetb · 2019-07-14T04:09:14Z

/lgtm
/approve

corneliusweig added 8 commits July 9, 2019 21:37

Add migration logic to upgrade the krew home to a newer version

c8410cf

Check if the krew home needs to be upgraded

b365407

Add todo about bad UX after fresh installs

a06f9a6

Add new cobra command krew system

44a8690

Add test to verify the list of renewable plugins

99189da

Add integration tests for system receipts-upgrade

bb4646f

Manually copy the krew manifest

80e560d

Krew cannot re-install itself if it is uninstalled, so it must be skipped in the standard code path.

Change message for migration check

46a3ca2

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 9, 2019

k8s-ci-robot requested review from ahmetb and juanvallejo July 9, 2019 20:13

k8s-ci-robot added 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. labels Jul 9, 2019

ahmetb mentioned this pull request Jul 10, 2019

Migrate krew home to add install receipts for installed plugins #232

Closed

ahmetb reviewed Jul 10, 2019

View reviewed changes

cmd/krew/cmd/system.go Outdated Show resolved Hide resolved

ahmetb reviewed Jul 10, 2019

View reviewed changes

corneliusweig added 4 commits July 11, 2019 09:56

Make migration check smarter

60d2ded

It no longer fails in fresh installs.

Improve package and function naming

588055d

Fix linter issues

38f1b69

Ensure consistent use of nested cobra commands

d89aacc

ahmetb reviewed Jul 11, 2019

View reviewed changes

pkg/testutil/tempdir.go Outdated Show resolved Hide resolved

ahmetb reviewed Jul 11, 2019

View reviewed changes

pkg/receiptsmigration/migration.go Outdated Show resolved Hide resolved

ahmetb reviewed Jul 11, 2019

View reviewed changes

pkg/receiptsmigration/migration.go Outdated Show resolved Hide resolved

corneliusweig force-pushed the w/repo-upgrade branch from bced34e to f73ba27 Compare July 11, 2019 20:16

corneliusweig changed the title ~~[WIP] Add subcommand for migrating the krew home~~ Add subcommand for migrating the krew home Jul 11, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2019

ahmetb mentioned this pull request Jul 12, 2019

hack: add script to verify receipts migration #255

Merged

k8s-ci-robot assigned corneliusweig Jul 12, 2019

k8s-ci-robot assigned ahmetb Jul 14, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2019

This comment has been minimized.

Sign in to view

k8s-ci-robot merged commit 5042800 into kubernetes-sigs:master Jul 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add subcommand for migrating the krew home #249

Add subcommand for migrating the krew home #249

corneliusweig commented Jul 9, 2019

ahmetb commented Jul 10, 2019

ahmetb Jul 10, 2019

corneliusweig Jul 11, 2019

ahmetb Jul 10, 2019

corneliusweig Jul 11, 2019

ahmetb Jul 11, 2019

corneliusweig Jul 11, 2019

ahmetb Jul 10, 2019

corneliusweig Jul 11, 2019

ahmetb Jul 11, 2019

ahmetb Jul 10, 2019

corneliusweig Jul 11, 2019

ahmetb Jul 11, 2019

corneliusweig Jul 11, 2019

codecov-io commented Jul 11, 2019 •

edited

Loading

ahmetb commented Jul 11, 2019

ahmetb commented Jul 12, 2019

ahmetb commented Jul 14, 2019

This comment has been minimized.

Add subcommand for migrating the krew home #249

Add subcommand for migrating the krew home #249

Conversation

corneliusweig commented Jul 9, 2019

ahmetb commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 11, 2019 • edited Loading

Codecov Report

ahmetb commented Jul 11, 2019

ahmetb commented Jul 12, 2019

ahmetb commented Jul 14, 2019

This comment has been minimized.

codecov-io commented Jul 11, 2019 •

edited

Loading