Index migration #505

chriskim06 · 2020-02-21T04:08:09Z

This only adds the migration code to change the directory structure that will be needed to support custom indexes. The other changes will be in subsequent PRs.

I'm using the existence of the ~~index/default/~~index/.git directory to indicate whether or not a migration needs to be performed.

Related issue: #483

This only changes the directory structure that will be needed to support custom indexes. The other changes will be in subsequent PRs.

codecov-io · 2020-02-21T04:15:46Z

Codecov Report

Merging #505 into master will decrease coverage by 1.29%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master     #505     +/-   ##
=========================================
- Coverage   58.51%   57.21%   -1.3%     
=========================================
  Files          22       23      +1     
  Lines         969      991     +22     
=========================================
  Hits          567      567             
- Misses        346      368     +22     
  Partials       56       56

Impacted Files	Coverage Δ
internal/indexmigration/migration.go	`0% <0%> (ø)`

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 fb5343a...4a0c3e0. Read the comment docs.

internal/indexmigration/migration.go

ahmetb · 2020-02-21T04:27:15Z

internal/indexmigration/migration_test.go

+			tmpDir, cleanup := testutil.NewTempDir(t)
+			defer cleanup()
+
+			_ = os.MkdirAll(tmpDir.Path(test.dirPath), os.ModePerm)


handle err :)

ahmetb · 2020-02-21T04:28:06Z

internal/indexmigration/migration.go

+		return err
+	}
+	if isMigrated {
+		klog.Infoln("Already migrated")


please use leveled logs (e.g. klog.v(2) ) for stuff users won't see.

also "already migrated" is not a good log line by itself.

Sounds good. I've seen that throughout the code base, is there some guide for what levels should be used in certain scenarios? I briefly looked through the klog repo but didn't see anything immediately.

Non-leveled logs: always visible to users
Leveled logs: never visible to users, unless -v LEVEL is specified.

We frequently use -v 2 or -v 10 etc for debugging.
So use judgement from the rest of the repo. :) If it's too off, we'll comment.

Actually wouldn't we want to display to the user that the migration has already been performed?

🤔 you’re right but this actually sounds like an error.
But both are fine since this is just a one off command.

Ok I've already changed it to use leveled logging, so I'll just leave it that way if you think either way is cool.

ahmetb · 2020-02-21T04:28:29Z

internal/indexmigration/migration.go

+	err = gitutil.EnsureCloned(constants.IndexURI, paths.DefaultIndexPath())
+	if err != nil {
+		return err
+	}
+	return nil


this simply can be returned as "errors.Wrap"

ahmetb · 2020-02-21T04:28:58Z

internal/indexmigration/migration.go

+func Migrate(paths environment.Paths) error {
+	isMigrated, err := Done(paths)
+	if err != nil {
+		return err


we rarely do return err in this codebase.
try errors.Wrap(err, "failed to check if index migration is complete")

ahmetb · 2020-02-21T04:31:05Z

internal/environment/environment.go

+// DefaultIndexPath returns the base directory where the default plugin index repository is cloned.
+func (p Paths) DefaultIndexPath() string { return filepath.Join(p.base, "index", "default") }


I feel like we don't need a method like this. Something like we eventually need to refactor this to IndexPath(name string).

The method above (IndexPath()) ideally should not be necessary (except for some tests, e.g. below).

Ya that makes sense, I'll remove it since this will check for the .git directory.

ahmetb · 2020-02-21T18:00:30Z

cmd/krew/cmd/system.go

 func init() {
+	if _, ok := os.LookupEnv("X_KREW_ENABLE_MULTI_INDEX"); ok {


Maybe add the migration check behind this feature gate to the PreRunE of root as well?

Sure I can add that, I didn't include the check in root to prevent other krew operations when the user hasn't migrated but it makes sense to include it in this PR

internal/indexmigration/migration.go

ahmetb · 2020-02-21T20:56:59Z

/lgtm
just one nit.

ahmetb · 2020-02-21T21:13:45Z

/lgtm
/approve
Thanks.

k8s-ci-robot · 2020-02-21T21:13:55Z

[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

corneliusweig · 2020-02-21T23:48:05Z

internal/indexmigration/migration.go

+}
+
+// Migrate removes the index directory and then clones krew-index to the new default index path.
+func Migrate(paths environment.Paths) error {


@chriskim06 This is currently untested. I think this is mostly because the migration code has external dependencies. What about renaming the directory index -> index/default? Then you could also test it :)

I'm not sure I totally understand what you mean about renaming the directory. If you could elaborate a little more I'd love to add a unit test for this. I was thinking that this would be covered more in integration tests after looking at the receiptmigration test.

Since this has the call to gitutil.EnsureCloned I didn't want that to be called during the unit test. Normally I would do something like using an interface for that function and a different implementation in my test that doesn't actually use git to mock that behavior, but I thought that might overcomplicate this PR.

So what I was trying to say is that the function is hard to test, because it has an external dependency due to the hidden git clone. After the current function exits, the result is the same as if $ mv index index/default was called (this is just pseudo-mv, it doesn't work like that :) ).

So if your implementation would rely on mv instead of git-clone, we could write a unit test for it, because file system operations are fine in unit tests. I think I'll give it a try to see if it works.

Whoops, I meant to reply to this yesterday. But ya, that makes sense, I didn't think to do it that way. It does seem more testable in your PR.

chriskim06 added 2 commits February 20, 2020 19:59

Add index migration code behind an environment variable

e12915d

This only changes the directory structure that will be needed to support custom indexes. The other changes will be in subsequent PRs.

Check to see if the migration has already happened

31cdb6f

k8s-ci-robot added 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 Feb 21, 2020

k8s-ci-robot requested review from ahmetb and corneliusweig February 21, 2020 04:08

Fix error and linting issue

317df5c

ahmetb reviewed Feb 21, 2020

View reviewed changes

internal/indexmigration/migration.go Outdated Show resolved Hide resolved

ahmetb reviewed Feb 21, 2020

View reviewed changes

Code review changes

aeeb8a2

chriskim06 mentioned this pull request Feb 21, 2020

add helper for inferring index name from input #506

Merged

Make migrated test case a little more explicit

dfa0d05

ahmetb reviewed Feb 21, 2020

View reviewed changes

chriskim06 added 2 commits February 21, 2020 10:38

Add migration check to root behind env var

10f0502

Update wording around out of date index message

521f840

ahmetb reviewed Feb 21, 2020

View reviewed changes

internal/indexmigration/migration.go Show resolved Hide resolved

k8s-ci-robot assigned ahmetb Feb 21, 2020

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

Add success message after migrate

9d0c164

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2020

Fix typo

4a0c3e0

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

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

k8s-ci-robot merged commit df44611 into kubernetes-sigs:master Feb 21, 2020

chriskim06 deleted the index-migration branch February 21, 2020 21:16

corneliusweig reviewed Feb 21, 2020

View reviewed changes

corneliusweig mentioned this pull request Feb 22, 2020

Simpler migration without external dependency #511

Merged

ahmetb added the area/multi-index label Mar 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Index migration #505

Index migration #505

chriskim06 commented Feb 21, 2020 •

edited

Loading

codecov-io commented Feb 21, 2020 •

edited

Loading

ahmetb Feb 21, 2020

ahmetb Feb 21, 2020

chriskim06 Feb 21, 2020

ahmetb Feb 21, 2020

chriskim06 Feb 21, 2020

ahmetb Feb 21, 2020

chriskim06 Feb 21, 2020

ahmetb Feb 21, 2020

ahmetb Feb 21, 2020 •

edited

Loading

ahmetb Feb 21, 2020

chriskim06 Feb 21, 2020

ahmetb Feb 21, 2020

ahmetb Feb 21, 2020

chriskim06 Feb 21, 2020 •

edited

Loading

ahmetb commented Feb 21, 2020

ahmetb commented Feb 21, 2020

k8s-ci-robot commented Feb 21, 2020

corneliusweig Feb 21, 2020

chriskim06 Feb 22, 2020

corneliusweig Feb 22, 2020

chriskim06 Feb 23, 2020

		// DefaultIndexPath returns the base directory where the default plugin index repository is cloned.
		func (p Paths) DefaultIndexPath() string { return filepath.Join(p.base, "index", "default") }

		func init() {
		if _, ok := os.LookupEnv("X_KREW_ENABLE_MULTI_INDEX"); ok {

Index migration #505

Index migration #505

Conversation

chriskim06 commented Feb 21, 2020 • edited Loading

codecov-io commented Feb 21, 2020 • edited Loading

Codecov Report

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

ahmetb Feb 21, 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

Choose a reason for hiding this comment

chriskim06 Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

ahmetb commented Feb 21, 2020

ahmetb commented Feb 21, 2020

k8s-ci-robot commented Feb 21, 2020

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 commented Feb 21, 2020 •

edited

Loading

codecov-io commented Feb 21, 2020 •

edited

Loading

ahmetb Feb 21, 2020 •

edited

Loading

chriskim06 Feb 21, 2020 •

edited

Loading