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

Index migration #505

Merged
merged 9 commits into from
Feb 21, 2020
12 changes: 12 additions & 0 deletions cmd/krew/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"sigs.k8s.io/krew/cmd/krew/cmd/internal"
"sigs.k8s.io/krew/internal/environment"
"sigs.k8s.io/krew/internal/gitutil"
"sigs.k8s.io/krew/internal/indexmigration"
"sigs.k8s.io/krew/internal/installation"
"sigs.k8s.io/krew/internal/installation/receipt"
"sigs.k8s.io/krew/internal/installation/semver"
Expand Down Expand Up @@ -138,6 +139,17 @@ func preRun(cmd *cobra.Command, _ []string) error {
return errors.New("krew home outdated")
}

if _, ok := os.LookupEnv("X_KREW_ENABLE_MULTI_INDEX"); ok {
isMigrated, err = indexmigration.Done(paths)
if err != nil {
return errors.Wrap(err, "error getting file info")
}
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 installation.IsWindows() {
klog.V(4).Infof("detected windows, will check for old krew installations to clean up")
err := cleanupStaleKrewInstallations()
Expand Down
20 changes: 20 additions & 0 deletions cmd/krew/cmd/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,11 @@
package cmd

import (
"os"

"github.com/spf13/cobra"

"sigs.k8s.io/krew/internal/indexmigration"
"sigs.k8s.io/krew/internal/receiptsmigration"
)

Expand Down Expand Up @@ -50,7 +53,24 @@ This command will be removed without further notice from future versions of krew
PreRunE: ensureIndexUpdated,
}

var indexUpgradeCmd = &cobra.Command{
Use: "index-upgrade",
Short: "Perform a migration of the krew index",
Long: `Krew became more awesome! To use the new features, you need to run this
one-time migration, which will enable installing plugins from custom indexes.

This command will be removed without further notice from future versions of krew.
`,
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
return indexmigration.Migrate(paths)
},
}

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@chriskim06 chriskim06 Feb 21, 2020

Choose a reason for hiding this comment

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

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

systemCmd.AddCommand(indexUpgradeCmd)
}
systemCmd.AddCommand(receiptsUpgradeCmd)
rootCmd.AddCommand(systemCmd)
}
62 changes: 62 additions & 0 deletions internal/indexmigration/migration.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Copyright 2019 The Kubernetes Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package indexmigration

import (
"os"
"path/filepath"

"github.com/pkg/errors"
"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) {
return true, nil
}
return false, err
}

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

Choose a reason for hiding this comment

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

@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 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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
}

err = os.RemoveAll(paths.IndexPath())
if err != nil {
return errors.Wrapf(err, "could not remove index directory %q", paths.IndexPath())
}
err = gitutil.EnsureCloned(constants.IndexURI, filepath.Join(paths.IndexPath(), "default"))
if err != nil {
return errors.Wrap(err, "failed to clone index")
}
klog.Infof("Migration completed successfully.")
return nil
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
}
67 changes: 67 additions & 0 deletions internal/indexmigration/migration_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright 2019 The Kubernetes Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package indexmigration

import (
"os"
"testing"

"sigs.k8s.io/krew/internal/environment"
"sigs.k8s.io/krew/internal/testutil"
)

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
expected bool
}{
{
name: "Already migrated",
dirPath: "index/default/.git",
expected: true,
},
{
name: "Not migrated",
dirPath: "index/.git",
expected: false,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
tmpDir, cleanup := testutil.NewTempDir(t)
defer cleanup()

err := os.MkdirAll(tmpDir.Path(test.dirPath), os.ModePerm)
if err != nil {
t.Fatal(err)
}

newPaths := environment.NewPaths(tmpDir.Root())
actual, err := Done(newPaths)
if err != nil {
t.Fatal(err)
}
if actual != test.expected {
t.Errorf("Expected %v but found %v", test.expected, actual)
}
})
}
}