Check whether PATH has been set up correctly #489

corneliusweig · 2020-02-01T21:12:49Z

Fixes #485
Related Homebrew/homebrew-core#49547

Homebrew refuses to add a post-install message for setting up
the PATH variable with krew's plugin store. Therefore we need
to check this on our side to warn users about an incomplete
setup. The goal is to

print specific instructions for each shell/OS.
not print the instructions when krew installs itself.

codecov-io · 2020-02-01T21:16:03Z

Codecov Report

Merging #489 into master will increase coverage by 0.52%.
The diff coverage is 91.66%.

@@           Coverage Diff            @@
##           master   #489      +/-   ##
========================================
+ Coverage   56.47%    57%   +0.52%     
========================================
  Files          19     21       +2     
  Lines         926    956      +30     
========================================
+ Hits          523    545      +22     
- Misses        350    357       +7     
- Partials       53     54       +1

Impacted Files	Coverage Δ
cmd/krew/cmd/internal/setup_check.go	`91.66% <91.66%> (ø)`
cmd/krew/cmd/internal/security_notice.go	`0% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4303b6a...b9675e8. Read the comment docs.

internal/assertion/setup.go

ahmetb · 2020-02-02T03:17:05Z

internal/assertion/setup.go

+    export PATH="${PATH}:${HOME}/.krew/bin"`
+)
+
+func CheckSetup(w io.Writer, paths environment.Paths) {


tbh the "assert" in the package name could make one think this works like "assert" keyword, but it doesn't. maybe pkg internal/platform?

I'm struggling with a good package name. installation and environment would make most sense to me, but those are taken already. platform doesn't quite ring the right bells to me (although it's better than assertion).
What do you think about internal/setup.WarnIfBinDirNotInPATH?

I went with setup for now. Let me know your thoughts.

Maybe shouldnt be in a separate pkg :)

😃 That could simplify the naming. How about the environment package?

internal/assertion/setup.go

internal/assertion/setup_test.go

corneliusweig · 2020-02-02T13:50:57Z

Thanks for your comments! PTAL

ahmetb · 2020-02-02T22:23:33Z

internal/setup/warning.go

+	}
+
+	krewPluginPath := paths.BinPath()
+	return strings.Contains(os.Getenv("PATH"), krewPluginPath)


Let's use if there's an utility in kubectl plugin PATH scanning logic.

If not, please separate by os.PathListSeparator, and do a == check; not Contains.

Doesn't really look like they have a dedicated util for that: https://github.com/kubernetes/kubernetes/blob/5cc945ad0d2fad2bdde7591490fcf0401fa335aa/staging/src/k8s.io/kubectl/pkg/cmd/plugin/plugin.go#L105

But checking for equality sounds right.

ahmetb · 2020-02-02T22:24:07Z

internal/setup/warning_test.go

+	"sigs.k8s.io/krew/internal/testutil"
+)
+
+func TestCheckSetup_firstRun(t *testing.T) {


rm test func names :)

I guess you meant the forgotten rename?

I don't know what I was saying above.
I think I meant make sure Test names are the same as func name.

internal/setup/warning_test.go

ahmetb · 2020-02-03T20:20:23Z

This PR is getting too large.
We're just supposed to split env and print a message. :) We don't even need this level of excessive testing. Plus now the environment package got refactored further.

Can we split that out to a different PR so that I don't lose context?

IMO this functionality can be in main package, doesn't warrant its own pkg.

Homebrew refuses to add a post-install message for setting up the PATH variable with krew's plugin store. Therefore we need to check this on our side to warn users about an incomplete setup. The goal is to - print specific instructions for each shell/OS. - not print the instructions when krew installs itself.

corneliusweig · 2020-02-04T20:21:43Z

Err.. I agree. This has gotten a little out of hand. It should be simpler now. PTAL

ahmetb · 2020-02-04T20:29:16Z

cmd/krew/cmd/internal/setup_check_test.go

+	paths := environment.NewPaths(tempDir.Path("does-not-exist"))
+	res := IsBinDirInPATH(paths)
+	if res == false {
+		t.Errorf("Expected positive result on first run")


nit: start error messages lowercase

ahmetb · 2020-02-04T20:30:03Z

cmd/krew/cmd/internal/setup_check_test.go

+	if err := os.Mkdir(tempDir.Path("exists"), os.ModePerm); err != nil {
+		t.Fatal(err)
+	}


can we just pass tempDir as path, instead of creating a dir called "exists"?

Sure. I wanted to make it more explicit that the directory is there vs. the non-existent case. But I guess this will do as well.

ahmetb · 2020-02-04T20:31:57Z

/lgtm
just some nits
/hold
feel free to cancel if not worth fixing.

I think it's gotten to 200 lines mostly because we added shell-specific handling, which is a better UX anyway.

ahmetb · 2020-02-04T23:26:07Z

/lgtm
/approve
thanks much!

k8s-ci-robot · 2020-02-04T23:26:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, 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~~ [ahmetb,corneliusweig]

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

corneliusweig · 2020-02-05T00:13:40Z

/hold cancel

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

k8s-ci-robot requested review from ahmetb and soltysh February 1, 2020 21:12

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 1, 2020

ahmetb reviewed Feb 1, 2020

View reviewed changes