Override default index #666

chriskim06 · 2020-11-14T23:19:13Z

Let me know what you guys think about the integration tests. I decided to reuse the krew-index tarball when KREW_DEFAULT_INDEX_URI is passed in a test by unarchiving it in the directory specified. Even though it's the same contents as krew-index I felt it still tests this feature since the default index is changing to some temp dir instead of https://github.com/kubernetes-sigs/krew-index.

I figured I could do the docs change in a follow up PR, but let me know if you want me to add it here.

Related issue: #637

chriskim06 · 2020-11-14T23:19:34Z

/assign @ahmetb
/assign @corneliusweig

corneliusweig

Thanks for looking into this, @chriskim06!

Given that this is a really simple logic change, I don't think we should go crazy with testing all config variations. So I think it would be better to keep just one of the integration tests, and maybe add unit tests? However, the logic is simple enough so that it does not require a unit test IMO.

corneliusweig · 2020-11-15T14:16:58Z

pkg/constants/constants.go

+	if uri := os.Getenv("KREW_DEFAULT_INDEX_URI"); uri != "" {
+		return uri
+	}
+	return "https://github.com/kubernetes-sigs/krew-index.git"


Maybe keep this as an unexported constant?

corneliusweig · 2020-11-15T14:27:08Z

pkg/constants/constants.go

+var DefaultIndexURI = getDefaultIndexURI()
+
+func getDefaultIndexURI() string {
+	if uri := os.Getenv("KREW_DEFAULT_INDEX_URI"); uri != "" {
+		return uri
+	}
+	return "https://github.com/kubernetes-sigs/krew-index.git"
+}


I'd suggest to use this pattern:

Suggested change

var DefaultIndexURI = getDefaultIndexURI()

func getDefaultIndexURI() string {

if uri := os.Getenv("KREW_DEFAULT_INDEX_URI"); uri != "" {

return uri

}

return "https://github.com/kubernetes-sigs/krew-index.git"

}

const defaultIndexURI = "https://github.com/kubernetes-sigs/krew-index.git"

var DefaultIndexURI string = defaultIndexURI

func init() {

if uri := os.Getenv("KREW_DEFAULT_INDEX_URI"); uri != "" {

DefaultIndexURI = uri

}

}

Then you can unit-test this logic by explicitly calling init() in your test. However, I think this function is so simple that it does necessarily not need one.

corneliusweig · 2020-11-15T14:28:46Z

integration_test/version_test.go

@@ -36,7 +48,7 @@ func TestKrewVersion(t *testing.T) {
 		}
 		optionValue := lineSplit.Split(line, 2)
 		if len(optionValue) < 2 {
-			t.Errorf("Expect `krew version` to output `OPTION VALUE` pair separated by spaces, got: %v", optionValue)
+			test.t.Errorf("Expect `krew version` to output `OPTION VALUE` pair separated by spaces, got: %v", optionValue)


Better return an error from this function and log the result in the caller, i.e.

Suggested change

test.t.Errorf("Expect `krew version` to output `OPTION VALUE` pair separated by spaces, got: %v", optionValue)

return fmt.Errorf("Expect `krew version` to output `OPTION VALUE` pair separated by spaces, got: %v", optionValue)

corneliusweig · 2020-11-15T14:29:25Z

integration_test/version_test.go

+	checkRequiredSubstrings(test, "https://github.com/kubernetes-sigs/krew-index.git", stdOut)
+}
+
+func TestKrewVersion_CustomDefaultIndexURI(t *testing.T) {


I think this test is the best to keep, because it is simple and does not do any extra installation. 👍

corneliusweig · 2020-11-15T14:33:31Z

integration_test/install_test.go

@@ -141,6 +141,18 @@ func TestKrewInstall_CustomIndex(t *testing.T) {
 	test.AssertExecutableNotInPATH("kubectl-" + validPlugin2)
 }

+func TestKrewInstall_CustomDefaultIndexURI(t *testing.T) {


I don't think this test is necessary. We don't need to check all our machinery with & without setting all env variables. You can't do that exhaustively anyway, because you get a factorial explosion of test cases.

I'd recommend to have a single simple test that checks if the env var gets picked up.

corneliusweig · 2020-11-15T14:36:32Z

integration_test/testutil_test.go

+	// check for KREW_DEFAULT_INDEX_URI
+	for _, e := range it.env {
+		if strings.Contains(e, "KREW_DEFAULT_INDEX_URI") {
+			val := strings.Split(e, "=")[1]
+			it.extractKrewIndexTar(val)
+			it.Krew("index", "add", "default", val).RunOrFail()
+			return
+		}
+	}


Let's not complicate the test setup further. This adds considerable complexity for a one-line config change.

I'd prefer not to have a special case for KREW_DEFAULT_INDEX_URI here.

corneliusweig · 2020-11-15T14:57:00Z

Btw, look at the issue number 😈

chriskim06 · 2020-11-15T16:34:52Z

Thanks for the feedback! You're right that the integration tests were definitely the most complex piece of this. I'll make the changes you suggested.

corneliusweig

Just one last nit, then let's merge this!

integration_test/version_test.go

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

chriskim06 · 2020-11-22T20:49:03Z

@corneliusweig @ahmetb could I get another round of reviews 🙏

corneliusweig · 2020-11-22T22:40:06Z

@chriskim06 Sure, looks good! Thanks for taking care of this!
/lgtm
/approve

k8s-ci-robot · 2020-11-22T22:40:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chriskim06, corneliusweig

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~~ [corneliusweig]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahmetb · 2020-11-23T05:01:26Z

pkg/constants/constants.go

 )
+
+// DefaultIndexURI points to the upstream index.
+var DefaultIndexURI = defaultIndexURI


sorry for the after-the-merge review.

I think we should not have code in pkg/constants that are actually not constants and change in runtime.

I recommend keeping this package as-is and implementing a DefaultIndex() method in another package, like pkg/index.

no worries, that makes sense. I think I can get a PR for that in later today

chriskim06 added 5 commits November 14, 2020 12:27

Add initial changes

94234b3

Use krew-index tar for alternative default index

bfcddb8

Add tests for index list and install

242b2c7

Use consistent naming for new tests

a9b062e

Don't initialize index in test and let update clone it

14a703c

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 14, 2020

k8s-ci-robot requested review from ahmetb and soltysh November 14, 2020 23:19

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 14, 2020

k8s-ci-robot assigned ahmetb and corneliusweig Nov 14, 2020

corneliusweig reviewed Nov 15, 2020

View reviewed changes

chriskim06 added 2 commits November 15, 2020 08:01

Add code change suggestions

b24a2df

Remove extra tests

4239d90

chriskim06 added 2 commits November 15, 2020 08:35

Remove outdated comment

406cf8e

Fix code patterns error

82ef9e4

corneliusweig approved these changes Nov 16, 2020

View reviewed changes

integration_test/version_test.go Outdated Show resolved Hide resolved

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

Update integration_test/version_test.go

c1674d7

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

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

k8s-ci-robot merged commit bb0fe8b into kubernetes-sigs:master Nov 22, 2020

ahmetb reviewed Nov 23, 2020

View reviewed changes

chriskim06 mentioned this pull request Nov 24, 2020

Default index function #668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override default index #666

Override default index #666

chriskim06 commented Nov 14, 2020

chriskim06 commented Nov 14, 2020

corneliusweig left a comment

corneliusweig Nov 15, 2020

corneliusweig Nov 15, 2020

corneliusweig Nov 15, 2020

corneliusweig Nov 15, 2020

corneliusweig Nov 15, 2020

corneliusweig Nov 15, 2020

corneliusweig commented Nov 15, 2020

chriskim06 commented Nov 15, 2020

corneliusweig left a comment

chriskim06 commented Nov 22, 2020

corneliusweig commented Nov 22, 2020

k8s-ci-robot commented Nov 22, 2020

ahmetb Nov 23, 2020

chriskim06 Nov 23, 2020

	test.t.Errorf("Expect `krew version` to output `OPTION VALUE` pair separated by spaces, got: %v", optionValue)
	return fmt.Errorf("Expect `krew version` to output `OPTION VALUE` pair separated by spaces, got: %v", optionValue)

Override default index #666

Override default index #666

Conversation

chriskim06 commented Nov 14, 2020

chriskim06 commented Nov 14, 2020

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

corneliusweig commented Nov 15, 2020

chriskim06 commented Nov 15, 2020

corneliusweig left a comment

Choose a reason for hiding this comment

chriskim06 commented Nov 22, 2020

corneliusweig commented Nov 22, 2020

k8s-ci-robot commented Nov 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment