Automatic index migration #607

chriskim06 · 2020-06-05T16:24:41Z

replace the part in root.go that previously informed the user that they needed to migrate with the actual indexmigration.Migrate function
add integration tests (1 for checking auto migration happens and another for the only scenario I can think of where it doesn't happen)

Fixes #600
Related issue: #566

ahmetb · 2020-06-05T21:50:46Z

cmd/krew/cmd/root.go

-		if !isMigrated && cmd.Use != "index-upgrade" {
-			fmt.Fprintln(os.Stderr, "You need to perform a migration to continue using krew.\nPlease run `kubectl krew system index-upgrade`")
-			return errors.New("krew index outdated")
+		if !isMigrated {


it seems indexmigration.Migrate already has the check and return:

krew/internal/indexmigration/migration.go

Lines 37 to 46 in ea01855

// Migrate removes the index directory and then clones krew-index to the new default index path.

func Migrate(paths environment.Paths) error {

isMigrated, err := Done(paths)

if err != nil {

return errors.Wrap(err, "failed to check if index migration is complete")

}

if isMigrated {

klog.V(2).Infoln("Already migrated")

return nil

}

so we're duplicating indexmigration.Done call here.

also try adding as many log statements around this path as you can ("detected migration needed, starting..." etc).

Ah good catch!

Should this logging be leveled or always displayed to the user? The confirmation at the end was displayed to the user back when this was a manual process through system index-upgrade but now that its automatic should this happen at some log level and be silent normally?

I think we can show the one-time operations like these to the user (after all they would be shown only "once"):

migrating krew index layout

migration completed

chriskim06 · 2020-06-05T23:22:25Z

/area multi-index

corneliusweig

Just minor nits. This is getting serious now 😄 !

corneliusweig · 2020-06-07T15:16:58Z

internal/indexmigration/migration.go

 	if err != nil {
 		return errors.Wrap(err, "failed to check if index migration is complete")
 	}
 	if isMigrated {
-		klog.V(2).Infoln("Already migrated")
+		klog.V(2).Infoln("already migrated")


IMO, log messages can be capitalized and end with a dot. Errors on the other hand should not, because they can be wrapped and then it looks weird.

Ah ok I can change it, I misremembered reading that log messages shouldn't be capitalized in addition to error messages.

corneliusweig · 2020-06-07T15:22:01Z

cmd/krew/cmd/root.go

-			fmt.Fprintln(os.Stderr, "You need to perform a migration to continue using krew.\nPlease run `kubectl krew system index-upgrade`")
-			return errors.New("krew index outdated")
+		if err := indexmigration.Migrate(paths); err != nil {
+			return errors.Wrap(err, "failed to automatically migrate index")


This error is no longer accurate. The index could already have been migrated, but the check failed. In this particular situation, I think it's best not to wrap at all, because the errors from Migrate are already descriptive. If you feel strongly about wrapping, I'd suggest something like

Suggested change

return errors.Wrap(err, "failed to automatically migrate index")

return errors.Wrap(err, "index initialization")

corneliusweig · 2020-06-07T15:23:58Z

integration_test/migration_test.go

+	// any command here should cause the index migration to occur
+	test.Krew("index", "list").RunOrFail()
+	if !isIndexMigrated(test) {
+		t.Error("output should include the default index after migration")


Suggested change

t.Error("output should include the default index after migration")

t.Error("index should have been auto-migrated")

corneliusweig · 2020-06-07T15:25:55Z

integration_test/migration_test.go

+	test.Krew().RunOrFail()
+	if isIndexMigrated(test) {


Is that behaviour something we really require or just a coincidence of the implementation? If the latter is the case, consider removing the test, because it doesn't test a relevant behaviour.

I think just a coincidence of the implementation (cobra doesn't seem to execute the prerun in the case of no subcommand or krew -h). It was something I noticed when I was testing the migration locally and wasn't really sure if I should add a test for or not. I can remove this if it seems unnecessary.

ahmetb · 2020-06-07T19:13:22Z

cmd/krew/cmd/root.go

-			fmt.Fprintln(os.Stderr, "You need to perform a migration to continue using krew.\nPlease run `kubectl krew system index-upgrade`")
-			return errors.New("krew index outdated")
+		if err := indexmigration.Migrate(paths); err != nil {
+			return err


this err needs to be wrapped. I don't see any indication of what was being done for err messages inside Migrate() (saying "migration failed"). So it's good to add that here.

I don't think we should say "migration failed", because the index could already have been migrated, but the check failed. Wrapping with a message which communicates this ambiguity would be fine though.

Maybe the function is also not named well. It checks and maybe migrates instead of just migrate.

What do you guys think about moving the migration needed check out of Migrate then? I think that would solve these issues (make it easier to wrap the migration failed/check failed errors and help the name make more sense).

Yes, let's separate the check from the migration. That will make it easier to understand what is actually happening. 👍

ahmetb · 2020-06-07T19:13:58Z

integration_test/migration_test.go

+	return err == nil
+}
+
+func prepareOldIndexLayout(it *ITest) {


is this something we should eventually delete?
if so please add a TODO (e.g. // TODO remove when testing indexmigration is no longer necessary).

ahmetb · 2020-06-07T19:15:11Z

internal/indexmigration/migration.go

 	if err != nil {
 		return errors.Wrap(err, "failed to check if index migration is complete")
 	}
 	if isMigrated {
-		klog.V(2).Infoln("Already migrated")
+		klog.V(2).Infoln("Already migrated.")


so imagine every time krew runs in -v≥2, this will show up in the output.
but it's missing context:

"Already migrated" → what’s migrated

when is it started (there's no "checking index migration" msg)

ahmetb · 2020-06-07T19:15:27Z

/approve
minor issues here, too.

Add integration test for auto migration

Just call migrate in root and add logging around migration. Also make migration check function private

ahmetb · 2020-06-10T22:37:17Z

/lgtm
/approve

thanks for this!

k8s-ci-robot · 2020-06-10T22:37:26Z

[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:

~~OWNERS~~ [ahmetb]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 5, 2020

k8s-ci-robot requested review from ahmetb and soltysh June 5, 2020 16:24

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 5, 2020

ahmetb reviewed Jun 5, 2020

View reviewed changes

k8s-ci-robot added the area/multi-index label Jun 5, 2020

corneliusweig approved these changes Jun 7, 2020

View reviewed changes

ahmetb reviewed Jun 7, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2020

chriskim06 added 8 commits June 8, 2020 15:05

Make migration automatic and remove system cmd

f5c19b0

Add integration test for auto migration

Fix typo

016c8f8

Add integration test for no migration scenario

b3825b6

Check for existence of default dir in test

e2c3b0a

Fix linting error

bacad93

Code review changes

5cbc832

Just call migrate in root and add logging around migration. Also make migration check function private

Code review changes

424493c

Code review changes

1521b2b

chriskim06 force-pushed the automatic-index-migration branch from 601a4cd to 1521b2b Compare June 8, 2020 22:06

k8s-ci-robot assigned ahmetb Jun 10, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2020

k8s-ci-robot merged commit 4e194df into kubernetes-sigs:master Jun 10, 2020

chriskim06 deleted the automatic-index-migration branch June 11, 2020 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatic index migration #607

Automatic index migration #607

chriskim06 commented Jun 5, 2020

ahmetb Jun 5, 2020

chriskim06 Jun 5, 2020

chriskim06 Jun 5, 2020 •

edited

Loading

ahmetb Jun 5, 2020

chriskim06 commented Jun 5, 2020

corneliusweig left a comment

corneliusweig Jun 7, 2020

chriskim06 Jun 7, 2020

corneliusweig Jun 7, 2020

corneliusweig Jun 7, 2020

corneliusweig Jun 7, 2020

chriskim06 Jun 7, 2020

ahmetb Jun 7, 2020

corneliusweig Jun 7, 2020

chriskim06 Jun 7, 2020 •

edited

Loading

corneliusweig Jun 7, 2020

ahmetb Jun 7, 2020

ahmetb Jun 7, 2020

ahmetb commented Jun 7, 2020

ahmetb commented Jun 10, 2020

k8s-ci-robot commented Jun 10, 2020

	// Migrate removes the index directory and then clones krew-index to the new default index path.
	func Migrate(paths environment.Paths) error {
	isMigrated, err := Done(paths)
	if err != nil {
	return errors.Wrap(err, "failed to check if index migration is complete")
	}
	if isMigrated {
	klog.V(2).Infoln("Already migrated")
	return nil
	}

	return errors.Wrap(err, "failed to automatically migrate index")
	return errors.Wrap(err, "index initialization")

	t.Error("output should include the default index after migration")
	t.Error("index should have been auto-migrated")

Automatic index migration #607

Automatic index migration #607

Conversation

chriskim06 commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskim06 Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskim06 commented Jun 5, 2020

corneliusweig left a 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

chriskim06 Jun 7, 2020 • edited Loading

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

ahmetb commented Jun 7, 2020

ahmetb commented Jun 10, 2020

k8s-ci-robot commented Jun 10, 2020

chriskim06 Jun 5, 2020 •

edited

Loading

chriskim06 Jun 7, 2020 •

edited

Loading