Add index add command #545

chriskim06 · 2020-03-10T22:47:35Z

Relatively small change. The majority of it is the test I added. I wasn't sure how to test the case where an index is added since it relies on git to clone the repo, so the only test cases I have are negative ones where the index with a name has already been configured and an invalid url is provided for the clone operation. I was thinking of using constants.IndexURI in a test case but didn't like that the unit test would need to be able to reach github so I didn't add it.

Related issue: #483

ahmetb · 2020-03-10T23:12:27Z

cmd/krew/cmd/index.go

+Example:
+  kubectl krew index add index-name index-url`,
+	RunE: func(cmd *cobra.Command, args []string) error {
+		if len(args) != 2 {


Cobra supports this natively. You can set minargs 2

ahmetb · 2020-03-10T23:12:44Z

cmd/krew/cmd/index.go

+
+		err := indexoperations.AddIndex(paths.IndexBase(), args[0], args[1])
+		if err != nil {
+			return errors.Wrapf(err, "failed to add index %s: %s", args[0], args[1])


No need to print args back again

cmd/krew/cmd/index.go

ahmetb · 2020-03-10T23:14:44Z

cmd/krew/cmd/index.go

+var indexAddCmd = &cobra.Command{
+	Use:   "add",
+	Short: "Add a new index",
+	Long: `Configure a new index to install plugins from.


See if you can name the positional args.
Like NAME GIT_URL.
Cobra probably has facilities to do that.

internal/index/indexoperations/index.go

internal/index/indexoperations/index_test.go

ahmetb · 2020-03-10T23:17:46Z

Please add CLI integration tests to ./integration_test.

corneliusweig

It's really great that you take small steps. This makes reviewing so much easier 👍!

internal/index/indexoperations/index_test.go

internal/index/indexoperations/index.go

ahmetb · 2020-03-11T22:28:47Z

integration_test/index_test.go

+	}
+
+	indexName := "foo"
+	test.Krew("index", "add", indexName, constants.IndexURI).RunOrFail()


Add/list should have separate tests ideally.

But for this you can merge them but make sure test name reflects that. Refer to other integration tests for an idea on how to develop. We test cli scenario and behavior here.

For example you don’t test 0 args fails case here :)

integration_test/index_test.go

ahmetb · 2020-03-11T22:30:27Z

integration_test/testutil_test.go

@@ -171,6 +173,13 @@ func (it *ITest) WithIndex() *ITest {
 	return it
 }

+// WithMigratedIndex initializes the index and performs the index migration.


Add TODO for deletion, ditto migrateIndex.

Why don’t we use the env variable for behavior change in initializeIndex instead? Simple if can solve this but you added 2 methods to be removed for it. :)

That's a good idea, I'll change it

ahmetb · 2020-03-11T23:23:27Z

@chriskim06 does the code handle this case?

krew index add ../../../usr/bin [REPO]

we need sanity checks around names to ensure they don't serve path escapes.

chriskim06 · 2020-03-11T23:33:18Z

I didn't think about that, I'll add those checks.

ahmetb · 2020-03-12T20:17:12Z

cmd/krew/cmd/index.go

+		err := indexoperations.AddIndex(paths.IndexBase(), args[0], args[1])
+		if err != nil {
+			return errors.Wrap(err, "failed to add index")
+		}
+		klog.Infoln("Successfully added index")
+		return nil


honestly this entire func can be 1 line :)
we'd know the success from the command execution anyway, so no need to log that.

I thought that it seemed a little nicer with some explicit validation (similar to index/receipt migration) but am fine either way. I'll remove it.

integration_test/index_test.go

ahmetb · 2020-03-12T20:19:10Z

internal/index/indexoperations/index.go

+
+// AddIndex initializes a new index to install plugins from.
+func AddIndex(path, name, url string) error {
+	if strings.ContainsAny(name, "./") {


as you might imagine this won't work on Windows

please use os.PathSeparator

consider extracting this to IsValidIndexName method and check for further things, like spaces, \ /, . ... + add unit tests + reuse everywhere (add, delete, ...)

Even easier, we could simply restrict index names to k8s resource names (see https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names). Hopefully, there is a k8s tool that we can just use?

Like IsValidPathSegmentName? Although that doesn't check \

Let's start restrictive, we can relax as we need.
Honestly, something like [A-Za-z0-9_-] is good enough and safest for now. Let's make it a method regardless as I'm intending to reuse in other places.

Ah, finally found it: https://github.com/kubernetes/kubernetes/blob/fdb2cb4c8832da1499069bda918c014762d8ac05/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L203

If we can, let's use it. Then we get all the goodness with names starting with alpha characters.

Are you 100% sure that string supports all repo names on github. If so, let's use it. If not, let's stick to something more relevant.

Looks like GitHub also allows dots in repo names, i.e. [A-Za-z0-9_-.]+. And it does not care whether a repo name starts with an alpha character or _-.. So if we want to support everything that GitHub does, the current pattern is better.

internal/index/indexoperations/index.go

integration_test/index_test.go

integration_test/testutil_test.go

corneliusweig · 2020-03-12T22:48:16Z

internal/index/indexoperations/index.go

+
+// AddIndex initializes a new index to install plugins from.
+func AddIndex(path, name, url string) error {
+	if strings.ContainsAny(name, "./") {


Even easier, we could simply restrict index names to k8s resource names (see https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names). Hopefully, there is a k8s tool that we can just use?

internal/index/indexoperations/index.go

corneliusweig · 2020-03-12T22:54:10Z

internal/index/indexoperations/index_test.go

+
+	if err := AddIndex(tmpDir.Path("index"), indexName, localRepo); err != nil {
+		t.Errorf("error adding index: %v", err)
+	}


IMHO, you should also test that AddIndex has actually done something.

To do that, you can have a combined Add+List scenario.

That way you can also move away from doing manual mkdir/clone in the test code.

@chriskim06 What's your opinion about using the ListIndex machinery for the assertion?

Sure I can change it to use that.

ahmetb · 2020-03-13T20:19:17Z

cmd/krew/cmd/index.go

+		if len(indexes) == 0 {
+			klog.Info(indexoperations.NoIndexMessage)
+			return nil
+		}


let's not do this here.

We can do that when user tries to query a plugin (krew install, krew update, krew upgrade etc) but there's no plugins installed.

Furthermore klog isn't good for this. we probably should use fmt.Fprintf or something like that with color pkg.

Also I think this command should still print an "empty table".

We need to print the output properly to stderr so it doesn't get mixed up with other stuff.

Do you mean it should still print:

INDEX URL

Yeah. That’s expected. Like krew list.

integration_test/index_test.go

ahmetb · 2020-03-13T20:23:59Z

integration_test/index_test.go

+	if !strings.Contains(string(out), indexoperations.NoIndexMessage) {
 		t.Fatalf("expected index list to be empty:\n%s", string(out))
 	}


testing with messages isn't good practice, let's not do that.
the previous test was good.

Ok I'll change it back

ahmetb · 2020-03-13T20:25:34Z

internal/index/indexoperations/index.go

+)
+
+var (
+	NoIndexMessage   = "No index configured, run kubectl krew index add default " + constants.IndexURI + " to get started"


Let's not do this here. We should ideally implement this in cmd/krew pkg as a func PrintDefaultIndexSuggestion().

Index commands are for pro-users, so most users should be realizing they miss an index while using main user cmds (install, search, update etc). "index" cmds are mgmt cmds, so they ideally shouldn't print these.

I'll just remove this for now then since I'm not covering the main user cmds in this PR.

Yeah sgtm. We’ll handle the scenario later.

ahmetb · 2020-03-13T20:29:06Z

internal/index/indexoperations/index_test.go

+	if IsValidIndexName(" ") {
+		t.Error("name cannot contain spaces")
+	}
+	if IsValidIndexName("foo/bar") {
+		t.Error("name cannot contain forward slashes")
+	}
+	if IsValidIndexName("foo\\bar") {
+		t.Error("name cannot contain back slashes")
+	}
+	if IsValidIndexName("foo.bar") {
+		t.Error("name cannot contain forward periods")
+	}
+	if !IsValidIndexName("foo") {
+		t.Error("expected valid index name: foo")
+	}


this actually would be the perfect use case for tabular tests :)

corneliusweig · 2020-03-13T20:32:11Z

internal/index/indexoperations/index.go

+	if os.IsNotExist(err) {
+		return nil, nil


nit: now that we ensure that index/ is present on startup, this error case no longer needs to be treated in a special way.

corneliusweig · 2020-03-15T21:43:04Z

internal/index/indexoperations/index.go

+// AddIndex initializes a new index to install plugins from.
+func AddIndex(path, name, url string) error {
+	if !IsValidIndexName(name) {
+		return errors.New("index name cannot contain path characters")


That error is no longer accurate.

What about something like "index name can only contain alphanumeric characters, -, and _"

This should just say “invalid index name”.
In a separate PR you can return an error from the validation func and wrap it here :)

corneliusweig · 2020-03-15T21:47:36Z

internal/index/indexoperations/index_test.go

+
+	if err := AddIndex(tmpDir.Path("index"), indexName, localRepo); err != nil {
+		t.Errorf("error adding index: %v", err)
+	}


@chriskim06 What's your opinion about using the ListIndex machinery for the assertion?

corneliusweig · 2020-03-15T22:01:29Z

internal/index/indexoperations/index.go

+
+// AddIndex initializes a new index to install plugins from.
+func AddIndex(path, name, url string) error {
+	if strings.ContainsAny(name, "./") {


Looks like GitHub also allows dots in repo names, i.e. [A-Za-z0-9_-.]+. And it does not care whether a repo name starts with an alpha character or _-.. So if we want to support everything that GitHub does, the current pattern is better.

corneliusweig

Nice, I think we are almost there. Just smaller final nits from my side. 👍

Add success case to unit test and add initial integration test for index command

Remove test table to make it easier to read and split into separate tests for failure and success cases

Print empty index list and convert valid index name to use test tables

Co-Authored-By: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Use ListIndexes in add test and fix error message

ahmetb · 2020-03-16T23:52:38Z

/lgtm
/approve

Thanks @chriskim06 for pushing it through.
Thanks @corneliusweig for thorough reviews!

I'll be making several followup PRs to clean up a few things!
I also have a patch ready for krew index delete.

k8s-ci-robot · 2020-03-16T23:52:49Z

[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 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 Mar 10, 2020

k8s-ci-robot requested review from corneliusweig and soltysh March 10, 2020 22:48

ahmetb reviewed Mar 10, 2020

View reviewed changes

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

ahmetb reviewed Mar 10, 2020

View reviewed changes

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

ahmetb reviewed Mar 10, 2020

View reviewed changes

internal/index/indexoperations/index.go Show resolved Hide resolved

ahmetb reviewed Mar 10, 2020

View reviewed changes

internal/index/indexoperations/index.go Show resolved Hide resolved

ahmetb reviewed Mar 10, 2020

View reviewed changes

internal/index/indexoperations/index_test.go Show resolved Hide resolved

ahmetb reviewed Mar 10, 2020

View reviewed changes

internal/index/indexoperations/index_test.go Show resolved Hide resolved

ahmetb reviewed Mar 10, 2020

View reviewed changes

internal/index/indexoperations/index_test.go Outdated Show resolved Hide resolved

ahmetb reviewed Mar 10, 2020

View reviewed changes

internal/index/indexoperations/index_test.go Show resolved Hide resolved

corneliusweig reviewed Mar 11, 2020

View reviewed changes

internal/index/indexoperations/index_test.go Outdated Show resolved Hide resolved

internal/index/indexoperations/index.go Show resolved Hide resolved

ahmetb reviewed Mar 11, 2020

View reviewed changes

integration_test/index_test.go Show resolved Hide resolved

ahmetb reviewed Mar 11, 2020

View reviewed changes

ahmetb reviewed Mar 12, 2020

View reviewed changes

integration_test/index_test.go Show resolved Hide resolved

ahmetb reviewed Mar 12, 2020

View reviewed changes

integration_test/index_test.go Outdated Show resolved Hide resolved

ahmetb reviewed Mar 12, 2020

View reviewed changes

internal/index/indexoperations/index.go Outdated Show resolved Hide resolved

corneliusweig reviewed Mar 12, 2020

View reviewed changes

ahmetb reviewed Mar 13, 2020

View reviewed changes

integration_test/index_test.go Show resolved Hide resolved

ahmetb reviewed Mar 13, 2020

View reviewed changes

corneliusweig reviewed Mar 13, 2020

View reviewed changes

corneliusweig reviewed Mar 15, 2020

View reviewed changes

chriskim06 and others added 12 commits March 15, 2020 15:50

Add index add command

55fb742

Add validation around number of args

dc3bb06

Prevent adding index if name is already used

d2fb96f

Most code review changes

4542c57

Add success case to unit test and add initial integration test for index command

Update unit test

f3f13e5

Remove test table to make it easier to read and split into separate tests for failure and success cases

Code review changes

c67e6d6

Check migration error

e09231a

Code review changes

7dddf2b

Code review changes

623af95

Print empty index list and convert valid index name to use test tables

Update integration_test/index_test.go

eb6e374

Co-Authored-By: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

Check number of lines in index integration test

fe3fe3f

Code review changes

d39f5fa

Use ListIndexes in add test and fix error message

chriskim06 force-pushed the index-add branch from 7959e23 to d39f5fa Compare March 15, 2020 22:52

k8s-ci-robot assigned ahmetb Mar 16, 2020

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

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

k8s-ci-robot merged commit 1201796 into kubernetes-sigs:master Mar 16, 2020

ahmetb mentioned this pull request Mar 17, 2020

cmd: Add 'index remove' command #552

Merged

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

This was referenced Mar 28, 2020

REQUEST: New membership for chriskim06 kubernetes/org#1757

Closed

Implement custom plugin indexes #566

Closed

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

Add index add command #545

Add index add command #545

Conversation

chriskim06 commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb commented Mar 10, 2020

corneliusweig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb commented Mar 11, 2020

chriskim06 commented Mar 11, 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

Choose a reason for hiding this comment

chriskim06 Mar 12, 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

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

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

corneliusweig left a comment

Choose a reason for hiding this comment

ahmetb commented Mar 16, 2020

k8s-ci-robot commented Mar 16, 2020

ahmetb Mar 11, 2020 •

edited

Loading

chriskim06 Mar 12, 2020 •

edited

Loading