[direnv] Add basic .envrc.example #4747

zmt · 2023-12-20T20:47:28Z

Pull Request check list

Commit conforms to CONTRIBUTING.md
Proper tests/regressions included
Documentation updated

Affected functionality
Add a basic .envrc.example that will build the go sdk using the Makefile and
setup the environment to use it so developers who use direnv
can easily "automatically" use the managed go sdk. Update CONTRIBUTING.md to
add a section describing the intended usage of direnv .envrc.example.

Description of change
While developing, it is good to use the same go tool as the make targets
when running go test. This makes that easy for users of direnv.
Developers who don't use direnv or don't copy the example into place will
see no change.

Signed-off-by: Zack Train <ztrain@uber.com>

zmt · 2023-12-20T21:41:34Z

My .envrc.local has:

go install golang.org/x/tools/gopls@latest

zmt · 2023-12-20T21:44:54Z

I think I should add the make go-check at the top to avoid:

% cd spire
direnv: loading .envrc
direnv: loading .envrc.local
go: cannot find GOROOT directory: /home/user/gocode/src/github.com/spiffe/spire/.build/linux-x86_64/go/1.21.5
direnv: export ~GOROOT ~PATH

when the worktree is completely clean (git -d -f -f -x)

Signed-off-by: Zack Train <ztrain@uber.com>

azdagron · 2023-12-21T20:42:42Z

Hey @zmt, thanks for opening this! I'm all about making contributor lives simpler though I'm a little hesitant introducing this to the repository, since it provides a place for changes to slip in that can run on developer machines without consent once the initial directory has been initially allow-listed. The makefile has targets that can be used to compile/build/test with the expected toolchain. Is there something that we get with this that we don't get with the Makefile?

zmt · 2023-12-21T23:30:26Z

Is there something that we get with this that we don't get with the Makefile?

I find it quicker/better/faster when iterating to use go test directly within single packages rather than going back to top-level and make test of everything. I want to be sure the go test will be equivalent to what make test is doing. This gives me that automagically. I also find it useful for go install (per my .envrc.local for gopls) and go get. Without this, I have to manually do all the stuff it does.

CONTRIBUTING.md

Signed-off-by: Zack Train <ztrain@uber.com>

zmt · 2024-01-02T20:36:55Z

since it provides a place for changes to slip in that can run on developer machines without consent once the initial directory has been initially allow-listed

I triple-checked this after our discussion. You are correct about the source_env .envrc.local: https://direnv.net/man/direnv-stdlib.1.html#codesourceenv-ltfileordirpathgtcode. It just auto-reloads on changes as long as the .envrc is unchanged.

sigh

zmt · 2024-01-02T20:41:08Z

I guess if I remove the hook to source_env arbitrary code, it might be more acceptable, but maybe I should just withdraw this.

zmt · 2024-01-02T21:45:38Z

Related:
direnv/direnv#556
direnv/direnv#448

See potential direnv security improvements in direnv/direnv#556. Signed-off-by: Zack Train <ztrain@uber.com>

Signed-off-by: Zack Train <ztrain@uber.com>

azdagron · 2024-01-02T23:08:04Z

.envrc.example

@@ -0,0 +1,19 @@
+toplevel="$(git rev-parse --show-toplevel)"
+
+# ensure the pre-commit git hook is in place


Can omit setting up the hook now

azdagron · 2024-01-02T23:12:32Z

.envrc.example

+pushd $toplevel >/dev/null &&
+    make go-check &&
+    popd >/dev/null


Feels a little cleaner to just use a subshell? (pushd and popd are verbose)

Suggested change

pushd $toplevel >/dev/null &&

make go-check &&

popd >/dev/null

(cd "$toplevel"; make go-check)

Or, preferably, we can leverage the -C option with make:

Suggested change

pushd $toplevel >/dev/null &&

make go-check &&

popd >/dev/null

make -C "$toplevel" go-check

Yeah - I make extensive use of git -C because I manage my home dir as a git repo, but it slipped my mind to do the same with make.

azdagron · 2024-01-02T23:16:17Z

.envrc.example

+machine="$(go env GOOS)-$(uname -m)"
+
+# export GOROOT and PATH for managed go sdk
+GOROOT="$toplevel/.build/$machine/go/$(cat .go-version)"


Why is GOROOT required? This is typically deduced by where the go binary resides. If PATH causes the right go binary to be executed, then GOROOT shouldn't be required?
It might be useful however to unset GOROOT....

It might be useful however to unset GOROOT....

I started with unset, but I think I had an issue installing gopls in an unexpected spot, so I set it, but am perfectly happy to move it back to just unset it.

Do you have any details on the failure? If not, I'd rather go with the unset route until we do (FYI, our makefile unset's GOROOT)

I am aware that the Makefile unsets GOROOT. I think I just futzed about until the first thing worked for me, but didn't fully clean it up. I don't actually need GOROOT. Updating.

azdagron · 2024-01-02T23:24:25Z

.envrc.example

+
+# export GOROOT and PATH for managed go sdk
+GOROOT="$toplevel/.build/$machine/go/$(cat .go-version)"
+PATH="$GOROOT/bin:$PATH"


There is already a makefile target that prints a modified path with the correct toolchain directory added (no need to repeat the same magic the makefile does to determine the path):

Suggested change

PATH="$GOROOT/bin:$PATH"

PATH="$(make -C "$toplevel" go-bin-path)"

This needs --no-print-directory to work.

azdagron · 2024-01-02T23:26:03Z

.envrc.example

+# export GOROOT and PATH for managed go sdk
+GOROOT="$toplevel/.build/$machine/go/$(cat .go-version)"
+PATH="$GOROOT/bin:$PATH"
+export GOROOT PATH


nit: PATH should already be exported.

Signed-off-by: Zack Train <ztrain@uber.com>

azdagron · 2024-01-03T00:06:17Z

.envrc.example

+unset GOROOT
+make -C "$toplevel" go-check
+go_bin_path="$(make --no-print-directory -C $toplevel go-bin-path)"
+PATH="$go_bin_path:$PATH"


make go-bin-path already appends the rest of the PATH so suffixing with $PATH shouldn't be required

Oh! No wonder the path looked so long.

Signed-off-by: Zack Train <ztrain@uber.com>

azdagron · 2024-01-03T00:26:12Z

.envrc.example

+# build and use the managed go sdk
+unset GOROOT
+make -C "$toplevel" go-check
+PATH="$(make --no-print-directory -C $toplevel go-bin-path)"


We should quote toplevel in case there are spaces to prevent word splitting

Suggested change

PATH="$(make --no-print-directory -C $toplevel go-bin-path)"

PATH="$(make --no-print-directory -C "$toplevel" go-bin-path)"

I got it in one spot but not the other 😞

Signed-off-by: Zack Train <ztrain@uber.com>

azdagron

\o/

Add a basic .envrc.example that will build the go sdk using the Makefile and setup the environment to use it so developers who use direnv can easily "automatically" use the managed go sdk. Update CONTRIBUTING.md to add a section describing the intended usage of direnv .envrc.example. Signed-off-by: Zack Train <ztrain@uber.com>

[direnv] Add basic .envrc

e3727d1

Signed-off-by: Zack Train <ztrain@uber.com>

zmt requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners December 20, 2023 20:47

zmt added 5 commits December 20, 2023 22:04

smoothing out the .envrc

a873f3b

Signed-off-by: Zack Train <ztrain@uber.com>

silence pushd/popd

3b386f4

Signed-off-by: Zack Train <ztrain@uber.com>

[direnv] document and fixup more

aec0a9a

Signed-off-by: Zack Train <ztrain@uber.com>

lint

f6c0070

Signed-off-by: Zack Train <ztrain@uber.com>

Merge branch 'main' into envrc

2f20827

zmt commented Dec 21, 2023

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

split out spiffe#4761

e03e9ed

Signed-off-by: Zack Train <ztrain@uber.com>

zmt marked this pull request as draft January 2, 2024 20:40

Merge remote-tracking branch 'origin/main' into envrc

6ebb66d

zmt added 2 commits January 2, 2024 22:15

source_env is insecure

14ffd70

See potential direnv security improvements in direnv/direnv#556. Signed-off-by: Zack Train <ztrain@uber.com>

move to .envrc.example

b826e88

Signed-off-by: Zack Train <ztrain@uber.com>

zmt marked this pull request as ready for review January 2, 2024 22:32

zmt changed the title ~~[direnv] Add basic .envrc~~ [direnv] Add basic .envrc.example Jan 2, 2024

azdagron reviewed Jan 2, 2024

View reviewed changes

address comments

f61a7a1

Signed-off-by: Zack Train <ztrain@uber.com>

azdagron reviewed Jan 3, 2024

View reviewed changes

even cleaner now

ac6287a

Signed-off-by: Zack Train <ztrain@uber.com>

azdagron reviewed Jan 3, 2024

View reviewed changes

missed some shellcheck and quotes

089e7b4

Signed-off-by: Zack Train <ztrain@uber.com>

azdagron approved these changes Jan 3, 2024

View reviewed changes

MarcosDY added 2 commits January 3, 2024 12:48

Merge branch 'main' into envrc

23a5c4d

Merge branch 'main' into envrc

f02f1da

MarcosDY added this to the 1.9.0 milestone Jan 3, 2024

MarcosDY merged commit 990bbc3 into spiffe:main Jan 3, 2024
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[direnv] Add basic .envrc.example #4747

[direnv] Add basic .envrc.example #4747

zmt commented Dec 20, 2023 •

edited

Loading

zmt commented Dec 20, 2023

zmt commented Dec 20, 2023

azdagron commented Dec 21, 2023

zmt commented Dec 21, 2023 •

edited

Loading

zmt commented Jan 2, 2024

zmt commented Jan 2, 2024 •

edited

Loading

zmt commented Jan 2, 2024 •

edited

Loading

azdagron Jan 2, 2024

azdagron Jan 2, 2024

zmt Jan 2, 2024

azdagron Jan 2, 2024

zmt Jan 2, 2024 •

edited

Loading

azdagron Jan 2, 2024

zmt Jan 3, 2024

azdagron Jan 2, 2024

zmt Jan 3, 2024

azdagron Jan 2, 2024

azdagron Jan 3, 2024

zmt Jan 3, 2024

azdagron Jan 3, 2024

zmt Jan 3, 2024

azdagron left a comment

		@@ -0,0 +1,19 @@
		toplevel="$(git rev-parse --show-toplevel)"

		# ensure the pre-commit git hook is in place

	PATH="$GOROOT/bin:$PATH"
	PATH="$(make -C "$toplevel" go-bin-path)"

	PATH="$(make --no-print-directory -C $toplevel go-bin-path)"
	PATH="$(make --no-print-directory -C "$toplevel" go-bin-path)"

[direnv] Add basic .envrc.example #4747

[direnv] Add basic .envrc.example #4747

Conversation

zmt commented Dec 20, 2023 • edited Loading

zmt commented Dec 20, 2023

zmt commented Dec 20, 2023

azdagron commented Dec 21, 2023

zmt commented Dec 21, 2023 • edited Loading

zmt commented Jan 2, 2024

zmt commented Jan 2, 2024 • edited Loading

zmt commented Jan 2, 2024 • 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

zmt Jan 2, 2024 • 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

azdagron left a comment

Choose a reason for hiding this comment

zmt commented Dec 20, 2023 •

edited

Loading

zmt commented Dec 21, 2023 •

edited

Loading

zmt commented Jan 2, 2024 •

edited

Loading

zmt commented Jan 2, 2024 •

edited

Loading

zmt Jan 2, 2024 •

edited

Loading