Skip to content

Commit

Permalink
Add $GOPATH/bin to $PATH
Browse files Browse the repository at this point in the history
  • Loading branch information
toolmantim committed May 3, 2018
1 parent a047875 commit 02e95b2
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 0 deletions.
4 changes: 4 additions & 0 deletions hooks/pre-checkout
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ if [[ -n "${BUILDKITE_PLUGIN_GOLANG_IMPORT:-}" ]]; then
# Now checkout into the correct import location within that gopath
export BUILDKITE_BUILD_CHECKOUT_PATH="$GOPATH/src/$BUILDKITE_PLUGIN_GOLANG_IMPORT"

This comment has been minimized.

Copy link
@lox

lox May 3, 2018

Contributor

GOPATH is a colon delimited collection of paths, wonder if we should test for this?

This comment has been minimized.

Copy link
@toolmantim

toolmantim May 4, 2018

Author Contributor

We don’t handle existing GOPATHs, we just set it to a single value for the build. Is that wrong? I did have handling existing paths in the roadmap on the readme tho 🤔

This comment has been minimized.

Copy link
@lox

lox May 4, 2018

Contributor

Nah, seems fine.

echo "BUILDKITE_BUILD_CHECKOUT_PATH=$BUILDKITE_BUILD_CHECKOUT_PATH"

# Add golang bin commands to path
export PATH="$PATH:$GOPATH/bin"

This comment has been minimized.

Copy link
@lox

lox May 3, 2018

Contributor

This should be $GOBIN!

This comment has been minimized.

Copy link
@toolmantim

toolmantim May 4, 2018

Author Contributor

This plugin assumes you don’t have any GOPATH or GOBIN set on the agent to begin with, so in that case it’d default to GOPATH/bin yeah?

But it brings up a good bug I think? With GOPATH we overwrite it, but for GOBIN we don’t. So it’d break if it was actually set, yeah? Maybe we should just explicitly set GOBIN as well?

This comment has been minimized.

Copy link
@lox

lox May 4, 2018

Contributor

I think having GOBIN set would only be useful if you wanted go get to install things into it. This seems fine to me!

This comment has been minimized.

Copy link
@toolmantim

toolmantim May 4, 2018

Author Contributor

I think that’s what the original issue asked for tho? So it’s maybe good to add? It’s a simple change.

This comment has been minimized.

Copy link
@lox

lox May 4, 2018

Contributor

Ah :) Yup, in that case we need to set GOBIN as well as add it to PATH!

This comment has been minimized.

echo PATH="$PATH"
else
echo "+++ :golang: No 'import' option specified"
exit 1
Expand Down
9 changes: 9 additions & 0 deletions tests/pre-checkout.bats
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,13 @@ export BUILDKITE_PIPELINE_SLUG="my-pipeline"

assert_success
assert_output --partial "BUILDKITE_BUILD_CHECKOUT_PATH=/builds/.golang/my-pipeline/src/my-dir"
}

@test "adds GOPATH/bin to PATH" {
export PATH=/bin

run $PWD/hooks/pre-checkout

assert_success
assert_output --partial "PATH=/bin:/builds/.golang/my-pipeline/bin"
}

2 comments on commit 02e95b2

@toolmantim
Copy link
Contributor Author

@toolmantim toolmantim commented on 02e95b2 May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearly I am not qualified to write a Golang plugin! 😂

I’ll read up and do some testing, and fix those two things. Thanks for the catch @lox! 🙏🏼

@lox
Copy link
Contributor

@lox lox commented on 02e95b2 May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, they were probably nitpicks, and I think you were correct!

Please sign in to comment.