cmd: Update each index instead of just default #588

chriskim06 · 2020-04-06T20:24:34Z

Here's the first part to getting update to work with multiple indexes (second part to show updated/new plugins is in progress).

I extracted some of the new code that was added for search into the namingutils.go file. I'm not exactly sure it fits there, it was originally named utils.go where I think it could have made more sense. Let me know if this isn't the right place to put the functionality to get a list of index name/URLs. I also moved the helper function for creating an empty git repo to a method on TempDir to use in the new test.

Related issue: #483

/area multi-index

ahmetb · 2020-04-07T00:11:37Z

cmd/krew/cmd/namingutils.go

+// allIndexes returns a slice of index name and URL pairs
+func allIndexes(p environment.Paths) ([]indexoperations.Index, error) {
+	indexes := []indexoperations.Index{
+		{
+			Name: constants.DefaultIndexName,
+			URL:  constants.DefaultIndexURI,
+		},
+	}
+	if os.Getenv(constants.EnableMultiIndexSwitch) != "" {
+		out, err := indexoperations.ListIndexes(p)
+		if err != nil {
+			return nil, errors.Wrapf(err, "failed to list plugin indexes available")
+		}
+		if len(out) != 0 {
+			indexes = out
+		}
+	}
+	return indexes, nil
+}


It might've been better if we extracted the feature gate into indexoperations.ListIndexes so we don't reimplement it here.

IIRC there's at least one more place we hardcode this:

[]indexoperations.Index{ { Name: constants.DefaultIndexName, URL: constants.DefaultIndexURI, },

you should be able to find very easily.

yeah if you handle it in one place (for now, until we remove the feature gate saves the following +60 lines in namingutils_test.go for us, and reduces duplication)

Ah good call, I didn't think about that.

So this was something that we talked about when going over the proposal:

If no indexes are configured, a message is printed explaining how to add krew-index as default

I just want to make sure we're cool with that. The current functionality is that krew-index gets added by default, but once multi-index is out that means that users won't be able to krew install ctx right after install.

I think we're conflating the two issues here:

You should hardcode that index to that method only when X_KREW_MULTI_INDEX is "not" set.

How do we add the default=krew-index (once multi-index work is complete) is another discussion which I'm tracking at Implement custom plugin indexes #566. (Right now we're auto-adding it if it's missing on install/update/upgrade) but this is a side-effect of old code, and it's not about this PR.

You should hardcode that index to that method only when X_KREW_MULTI_INDEX is "not" set.

We decided before that krew index list after a fresh install should work and return an empty list. Do you want me to change that behavior and accompanying tests if we want it to return the hardcoded index when env var isn't set?

How do we add the default=krew-index (once multi-index work is complete) is another discussion which I'm tracking at #566. (Right now we're auto-adding it if it's missing on install/update/upgrade) but this is a side-effect of old code, and it's not about this PR.

I brought it up because that was part of the proposal and it seemed like this would be the PR to take care of the help message when no indexes are configured. If that's not the case then what should I do for now when the env var is set, but no indexes are configured?

We decided before that krew index list after a fresh install should work and return an empty list. Do you want me to change that behavior and accompanying tests if we want it to return the hardcoded index when env var isn't set?

We should keep this behavior. I only wanted that we call indexoperations.ListIndexes always, but without the feature flag, it returns default index, with the feature flag it lists from filesystem.

I can make this change if you want me to.

I brought it up because that was part of the proposal and it seemed like this would be the PR to take care of the help message when no indexes are configured

That can be a separate PR, let's keep this one small.

Ok I can take care of it here 👍

internal/testutil/tempdir.go

ahmetb · 2020-04-07T00:15:40Z

cmd/krew/cmd/update.go

-		return errors.Wrap(err, "failed to update the local index")
+	indexes, err := allIndexes(paths)
+	if err != nil {
+		return err


ahmetb · 2020-04-07T00:17:26Z

cmd/krew/cmd/update.go

@@ -105,9 +105,16 @@ func showUpdatedPlugins(out io.Writer, preUpdate, posUpdate []index.Plugin, inst
 func ensureIndexUpdated(_ *cobra.Command, _ []string) error {


method needs rename? :)

ahmetb · 2020-04-07T00:19:31Z

cmd/krew/cmd/update.go

+		indexPath := paths.IndexPath(idx.Name)
+		klog.V(1).Infof("Updating the local copy of plugin index (%s)", indexPath)
+		if err := gitutil.EnsureUpdated(idx.URL, indexPath); err != nil {
+			return errors.Wrapf(err, "failed to update the local index %s", idx.Name)


potentially the most interesting part of the PR here:

Do we bail out on the first index failed to update ––OR–– do we go ahead and update all possible indexes, and collect the errors (or at least one of them) + report it at the end.

For example, today, krew install ctx BAD_PLUGIN ns will install ctx & ns, but the overall command will fail. This was intentionally designed for good UX.

With krew install and krew upgrade both calling ensureIndexUpdated every time before they do their job, I think a bad index (e.g. deleted repo) will render the majority of krew user surface unusable ––especially if that index has a name that's alphabetically earlier than "default".

Thoughts @corneliusweig ?

In the long run, we might want to update indexes in parallel. If doing this in parallel, it is no longer possible to fail on the first index update, because "first" is not a proper concept then. So to avoid behaviour change in the future, we should strive to update everything and collect errors.

Besides, it's just better UX IMO.

chriskim06 · 2020-04-08T01:01:53Z

~~I'm working on adding integration tests for this.~~

I switched this back to sequentially updating the indexes for now, I ran into an issue with the test that checks for the index failed update. The test hangs when the update fails in a goroutine for some reason and I've been trying to figure out why.

Would it be ok to create an issue and follow up PR for making the update concurrent?

Switch back to sequential updates. The test where gitutil.EnsureUpdated fails seems to hang when it happens in a goroutine for some reason.

ahmetb · 2020-04-11T20:46:31Z

integration_test/update_test.go

+	if len(out) == 0 {
+		t.Error("no plugins in index foo after update")
+	}


shouldn't you be also checking what's in default index now?
also you can use something like krew search here to find plugins in all indexes.

Ya good call 👍

I think it should be enough to look for two plugin names in the output. For example, ctx and foo/ctx.

ahmetb · 2020-04-11T20:48:25Z

cmd/krew/cmd/update.go

+		indexPath := paths.IndexPath(idx.Name)
+		klog.V(1).Infof("Updating the local copy of plugin index (%s)", indexPath)
+		if err := gitutil.EnsureUpdated(idx.URL, indexPath); err != nil {
+			klog.V(1).Infof("error updating index %s: %s", idx.Name, err)


you should probably do this as klog.Warningf otherwise the user will never know (unless they use -v) what the error is.

See how we handle multiple plugin install failures in install.go.

krew/cmd/krew/cmd/install.go

Line 160 in b2b8a40

klog.Warningf("failed to install plugin %q: %v", plugin.Name, err)

ahmetb · 2020-04-11T20:52:11Z

cmd/krew/cmd/update.go

@@ -132,6 +145,9 @@ func ensureIndexUpdated(_ *cobra.Command, _ []string) error {
 	// TODO(chriskim06) consider commenting this out when refactoring for custom indexes
 	showUpdatedPlugins(os.Stderr, preUpdateIndex, posUpdateIndex, installedPlugins)

+	if len(failed) != 0 {
+		return errors.Errorf("failed to update the following indexes: %s\n", strings.Join(failed, ", "))


Something else we do in install.go is that we collect the first error as returnErr and wrap+return it.

krew/cmd/krew/cmd/install.go

Lines 161 to 163 in b2b8a40

if returnErr == nil {

returnErr = err

}

krew/cmd/krew/cmd/install.go

Lines 178 to 180 in b2b8a40

if len(failed) > 0 {

return errors.Wrapf(returnErr, "failed to install some plugins: %+v", failed)

}

The reason for this is that when you wrap+return errors, when executed with -v 1 or above, we the retain ability to print full stack trace (in this case, at least for 1 error which is good enough).

corneliusweig

When you update with the comments by Ahmet, this looks pretty good to me.
Thanks for bringing the multi-index project one step further ahead!

corneliusweig · 2020-04-11T21:53:20Z

integration_test/update_test.go

+	defer cleanup()
+
+	test = test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+	os.Setenv(constants.EnableMultiIndexSwitch, "1")


Please add a comment like

// to enable new paths in environment.NewPaths()

corneliusweig · 2020-04-11T22:00:03Z

integration_test/update_test.go

+	if len(out) == 0 {
+		t.Error("no plugins in index foo after update")
+	}


I think it should be enough to look for two plugin names in the output. For example, ctx and foo/ctx.

ahmetb · 2020-04-12T21:48:09Z

cmd/krew/cmd/update.go

@@ -132,6 +149,9 @@ func ensureIndexUpdated(_ *cobra.Command, _ []string) error {
 	// TODO(chriskim06) consider commenting this out when refactoring for custom indexes
 	showUpdatedPlugins(os.Stderr, preUpdateIndex, posUpdateIndex, installedPlugins)



I’ve realized this if (+counting) isnt necessary since errors.Wrap already handles nil err. Maybe remove?

Sure I can do that. Do you think it will look weird if this ends with the return errors.Wrapf(...)?

I think it’s fine. I just realized the check is redundant.

Ok I'll remove the check then.

ahmetb · 2020-04-12T21:49:05Z

integration_test/update_test.go

+		t.Error("expected update to fail")
+	}
+	if !strings.Contains(string(out), "failed to update the following indexes: foo") {
+		t.Error("expected index update to fail for foo")


Try doing this for debuggability:

%q doesn’t contain msg=%q

What are the two strings in the example you gave? I'm guessing the output of krew update and foo but want to make sure.

Out & “failed to ...foo”

You can export latter into a variable.

ahmetb · 2020-04-13T16:44:48Z

/lgtm
/approve
thanks!

k8s-ci-robot · 2020-04-13T16:45:00Z

[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 area/multi-index cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 6, 2020

k8s-ci-robot requested review from corneliusweig and soltysh April 6, 2020 20:24

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

ahmetb reviewed Apr 7, 2020

View reviewed changes

internal/testutil/tempdir.go Show resolved Hide resolved

ahmetb reviewed Apr 7, 2020

View reviewed changes

chriskim06 mentioned this pull request Apr 7, 2020

Implement custom plugin indexes #566

Closed

22 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2020

chriskim06 added 3 commits April 7, 2020 17:15

Update each index instead of just default

d9f30f8

Fix linting issue

de38a18

Split into two tests

6717fe8

chriskim06 force-pushed the index-updates branch from 0c7d67e to 6717fe8 Compare April 8, 2020 00:16

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2020

Code review changes

a4c94da

chriskim06 added 2 commits April 7, 2020 18:03

Wrap error in search

0e31f9a

Add integration tests

c674426

Switch back to sequential updates. The test where gitutil.EnsureUpdated fails seems to hang when it happens in a goroutine for some reason.

ahmetb reviewed Apr 11, 2020

View reviewed changes

corneliusweig reviewed Apr 11, 2020

View reviewed changes

Code review changes

993758c

ahmetb approved these changes Apr 12, 2020

View reviewed changes

Code review changes

8a8195d

k8s-ci-robot assigned ahmetb Apr 13, 2020

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

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

k8s-ci-robot merged commit c99749f into kubernetes-sigs:master Apr 13, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd: Update each index instead of just default #588

cmd: Update each index instead of just default #588

chriskim06 commented Apr 6, 2020

ahmetb Apr 7, 2020

ahmetb Apr 7, 2020

chriskim06 Apr 7, 2020

chriskim06 Apr 7, 2020

ahmetb Apr 7, 2020

chriskim06 Apr 7, 2020 •

edited

Loading

ahmetb Apr 7, 2020

chriskim06 Apr 7, 2020

ahmetb Apr 7, 2020

ahmetb Apr 7, 2020

ahmetb Apr 7, 2020

corneliusweig Apr 7, 2020

chriskim06 commented Apr 8, 2020 •

edited

Loading

ahmetb Apr 11, 2020

chriskim06 Apr 11, 2020

corneliusweig Apr 11, 2020

ahmetb Apr 11, 2020 •

edited

Loading

ahmetb Apr 11, 2020

corneliusweig left a comment

corneliusweig Apr 11, 2020

corneliusweig Apr 11, 2020

ahmetb Apr 12, 2020

chriskim06 Apr 12, 2020

ahmetb Apr 12, 2020

chriskim06 Apr 12, 2020

ahmetb Apr 12, 2020

chriskim06 Apr 12, 2020

ahmetb Apr 12, 2020

ahmetb commented Apr 13, 2020

k8s-ci-robot commented Apr 13, 2020

		@@ -105,9 +105,16 @@ func showUpdatedPlugins(out io.Writer, preUpdate, posUpdate []index.Plugin, inst
		func ensureIndexUpdated(_ *cobra.Command, _ []string) error {

	if len(failed) > 0 {
	return errors.Wrapf(returnErr, "failed to install some plugins: %+v", failed)
	}

		@@ -132,6 +149,9 @@ func ensureIndexUpdated(_ *cobra.Command, _ []string) error {
		// TODO(chriskim06) consider commenting this out when refactoring for custom indexes
		showUpdatedPlugins(os.Stderr, preUpdateIndex, posUpdateIndex, installedPlugins)

cmd: Update each index instead of just default #588

cmd: Update each index instead of just default #588

Conversation

chriskim06 commented Apr 6, 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 Apr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskim06 commented Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb Apr 11, 2020 • edited Loading

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

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

ahmetb commented Apr 13, 2020

k8s-ci-robot commented Apr 13, 2020

chriskim06 Apr 7, 2020 •

edited

Loading

chriskim06 commented Apr 8, 2020 •

edited

Loading

ahmetb Apr 11, 2020 •

edited

Loading