Notify when host and bootstrap agent paths mismatch #3123
Conversation
…opment, if not development then log warning
| // update the buildkite-agent binary available from $PATH | ||
| if err := checkBinaryPaths(); err != nil { | ||
| if version.IsDevelopmentBuild() { | ||
| return fmt.Errorf("check binary paths: %s", err) |
There was a problem hiding this comment.
There might be situations where we want to allow this. I think it's good to err on the side of blowing up, rather than behaving unexpectedly, but I wonder if we should have a parameter to allow it?
There was a problem hiding this comment.
given that it's only used for development builds, i think if we need to turn it off for dev builds we can just comment out the code?
| return fmt.Errorf("failed to locate bootstrap buildkite-agent: %w", err) | ||
| } | ||
|
|
||
| hostPath, err := os.Executable() |
There was a problem hiding this comment.
The documentation points out this may return a symlink, which could cause inconsistencies. It recommends evaluating the symlink:
If a symlink was used to start the process, depending on the operating system, the result might be the symlink or the path it pointed to. If a stable result is needed, path/filepath.EvalSymlinks might help.
| if version.IsDevelopmentBuild() { | ||
| return fmt.Errorf("check binary paths: %s", err) | ||
| } else { | ||
| l.Warn("check binary paths: %s", err) |
There was a problem hiding this comment.
This could create a lot of logs for customers. I wonder if it should be debug level?
There was a problem hiding this comment.
it should only be once at the start of the agent logs (not the job logs), so i think that this is probably fine.
| // checkBinaryPaths looks up both the bootstrap and host buildkite-agent paths, | ||
| // and returns an error if they do not match or if either cannot be determined. | ||
| func checkBinaryPaths() error { | ||
| bootstrapPath, err := exec.LookPath("buildkite-agent") |
There was a problem hiding this comment.
We may also want to resolve this to ensure it's not a symlink, to ensure we're comparing like for like
| if version.IsDevelopmentBuild() { | ||
| return fmt.Errorf("check binary paths: %s", err) | ||
| } else { | ||
| l.Warn("check binary paths: %s", err) |
There was a problem hiding this comment.
it should only be once at the start of the agent logs (not the job logs), so i think that this is probably fine.
| // update the buildkite-agent binary available from $PATH | ||
| if err := checkBinaryPaths(); err != nil { | ||
| if version.IsDevelopmentBuild() { | ||
| return fmt.Errorf("check binary paths: %s", err) |
There was a problem hiding this comment.
given that it's only used for development builds, i think if we need to turn it off for dev builds we can just comment out the code?
Yeah I wasn't sure either but was thinking if it ever did log the warning in production that would be super informative to the user, then I thought of the following scenario too. A production environment in this context includes a user downloading a new agent binary directly to test out a new feature locally. They forgot they installed the agent via brew 12 months ago and during their testing they'll run into the same confusion as us. In this case it will only raise the warning, which is the least disruptive approach while still giving them a clue to whats wrong. |
|
I think the overall approach is good. Though at some point I want to get #1816 on the frontburner again... |
Description
During agent startup check if the bootstrap agent path matches the startup agent path. This is to avoid confusion during development when changes are made to the agent but the bootstrap agent still uses the default agent on the
$PATH. Usually this results in changes being made to the agent, but then those changes not being observed when running the updated agent locally.For example if I
go build -o buildkite-agent .then./buildkite-agent start --spawn 2 --endpoint http://agent.buildkite.localhost/v3 --token buildkiteBy default the above will result in
./buildkite-agentstarting up and waiting for jobs, when a job is run, if that job uploads a pipeline it will use (in my case)/opt/homebrew/bin/buildkite-agent, not./buildkite-agent, so any changes I've made to./buildkite-agentwill not be observed for the agent pipeline upload command.This PR updates the agent to return a error
fatal: check binary paths: mismatched buildkite-agent paths: host="/Users/jordan/src/agent/buildkite-agent" bootstrap="/opt/homebrew/bin/buildkite-agent"on boot. If the host agent is a development build then the error is fatal, otherwise it is just a warning.Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go fmt ./...)