Repository: kubernetes-sigs/krew
Pull request #511 (2020-02-22 22:32:56)

Simpler migration without external dependency
This migrates the krew home to the new layout without an external dependency.
Related issue: #505

Also see https://github.com/kubernetes-sigs/krew/pull/505#discussion_r382856984
<!-- For proposed features, make sure there's an issue it's discussed first -->

cc @chriskim06 

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/511#" title="Author self-approved">corneliusweig</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fkrew).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/krew/blob/master/OWNERS)~~ [corneliusweig]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov-io: # [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/511?src=pr&el=h1) Report
> Merging [#511](https://codecov.io/gh/kubernetes-sigs/krew/pull/511?src=pr&el=desc) into [master](https://codecov.io/gh/kubernetes-sigs/krew/commit/e662ea441206a51e156f6d6901a033e087b32521?src=pr&el=desc) will **increase** coverage by `1.44%`.
> The diff coverage is `40%`.

[![Impacted file tree graph](https://codecov.io/gh/kubernetes-sigs/krew/pull/511/graphs/tree.svg?width=650&token=J3nFtARI2m&height=150&src=pr)](https://codecov.io/gh/kubernetes-sigs/krew/pull/511?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #511      +/-   ##
==========================================
+ Coverage   57.42%   58.87%   +1.44%     
==========================================
  Files          23       23              
  Lines         996      997       +1     
==========================================
+ Hits          572      587      +15     
+ Misses        368      350      -18     
- Partials       56       60       +4
```


| [Impacted Files](https://codecov.io/gh/kubernetes-sigs/krew/pull/511?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/indexmigration/migration.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/511/diff?src=pr&el=tree#diff-aW50ZXJuYWwvaW5kZXhtaWdyYXRpb24vbWlncmF0aW9uLmdv) | `65.21% <40%> (+65.21%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/511?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/511?src=pr&el=footer). Last update [e662ea4...7c00b39](https://codecov.io/gh/kubernetes-sigs/krew/pull/511?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ahmetb: /lgtm
/approve
/hold 
just a question.
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/511#issuecomment-590527147" title="Approved">ahmetb</a>*, *<a href="https://github.com/kubernetes-sigs/krew/pull/511#" title="Author self-approved">corneliusweig</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fkrew).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/krew/blob/master/OWNERS)~~ [ahmetb,corneliusweig]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
corneliusweig: /hold cancel

Patch:
diff --git a/internal/indexmigration/migration.go b/internal/indexmigration/migration.go
index d8685c5..342bc80 100644
--- a/internal/indexmigration/migration.go
+++ b/internal/indexmigration/migration.go
@@ -22,17 +22,13 @@ import (
 	"k8s.io/klog"
 
 	"sigs.k8s.io/krew/internal/environment"
-	"sigs.k8s.io/krew/internal/gitutil"
-	"sigs.k8s.io/krew/pkg/constants"
 )
 
 // Done checks if the krew installation requires a migration to support multiple indexes.
 // A migration is necessary when the index directory contains a ".git" directory.
 func Done(paths environment.Paths) (bool, error) {
 	_, err := os.Stat(filepath.Join(paths.IndexPath(), ".git"))
-	if err == nil {
-		return false, nil
-	} else if os.IsNotExist(err) {
+	if err != nil && os.IsNotExist(err) {
 		return true, nil
 	}
 	return false, err
@@ -48,15 +44,22 @@ func Migrate(paths environment.Paths) error {
 		klog.V(2).Infoln("Already migrated")
 		return nil
 	}
+	indexPath := paths.IndexPath()
+	tmpPath := filepath.Join(paths.BasePath(), "tmp_index_migration")
+	newPath := filepath.Join(paths.IndexPath(), "default")
 
-	err = os.RemoveAll(paths.IndexPath())
-	if err != nil {
-		return errors.Wrapf(err, "could not remove index directory %q", paths.IndexPath())
+	if err := os.Rename(indexPath, tmpPath); err != nil {
+		return errors.Wrapf(err, "could not move index directory %q to temporary location %q", indexPath, tmpPath)
 	}
-	err = gitutil.EnsureCloned(constants.IndexURI, filepath.Join(paths.IndexPath(), "default"))
-	if err != nil {
-		return errors.Wrap(err, "failed to clone index")
+
+	if err := os.Mkdir(indexPath, os.ModePerm); err != nil {
+		return errors.Wrapf(err, "could create index directory %q", indexPath)
 	}
+
+	if err := os.Rename(tmpPath, newPath); err != nil {
+		return errors.Wrapf(err, "could not move temporary index directory %q to new location %q", tmpPath, newPath)
+	}
+
 	klog.Infof("Migration completed successfully.")
 	return nil
 }


Test patch:
diff --git a/internal/indexmigration/migration_test.go b/internal/indexmigration/migration_test.go
index eaaf5ff..a3155d3 100644
--- a/internal/indexmigration/migration_test.go
+++ b/internal/indexmigration/migration_test.go
@@ -23,10 +23,6 @@ import (
 )
 
 func TestIsMigrated(t *testing.T) {
-	if _, ok := os.LookupEnv("X_KREW_ENABLE_MULTI_INDEX"); !ok {
-		t.Skip("Set X_KREW_ENABLE_MULTI_INDEX variable to run this test")
-	}
-
 	tests := []struct {
 		name     string
 		dirPath  string
@@ -65,3 +61,32 @@ func TestIsMigrated(t *testing.T) {
 		})
 	}
 }
+
+func TestMigrate(t *testing.T) {
+	var tests = []struct {
+		name   string
+		gitDir string
+	}{
+		{name: "migration is necessary", gitDir: "index/.git"},
+		{name: "no migration is necessary", gitDir: "index/default/.git"},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			tmpDir, cleanup := testutil.NewTempDir(t)
+			defer cleanup()
+
+			tmpDir.Write(tt.gitDir, nil)
+
+			newPaths := environment.NewPaths(tmpDir.Root())
+			err := Migrate(newPaths)
+			if err != nil {
+				t.Fatal(err)
+			}
+			done, err := Done(newPaths)
+			if err != nil || !done {
+				t.Errorf("expected migration to be done: %s", err)
+			}
+		})
+	}
+}