Add --write-env-file cli option
#643
Conversation
|
Hey @DazWorrall, thanks for taking the time to implement this and also being patient whilst we thought it over! I know I suggested it initially, but reckon we should ditch the config option and just expose a env file to each job with it's path in |
|
(What you suggested in #615 (comment) 😓) |
|
One question we've been talking back and forward on is whether you'd expect environmental changes from hooks to be reflected in this file. Thoughts? |
|
We wouldn't I don't think - we're explicitly looking to see what was configured on the job (i.e. the equivalent of calling the |
|
@lox removed the config option. |
| // Prepare a file to recieve the given job environment | ||
| if file, err := ioutil.TempFile("", fmt.Sprintf("job-env-%s", runner.Job.ID)) ; err != nil { | ||
| return runner, err | ||
| } else { |
There was a problem hiding this comment.
This doesn't need the else block, see https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow.
There was a problem hiding this comment.
Without the else block file will be out of scope:
if file, err := ioutil.TempFile("", fmt.Sprintf("job-env-%s", runner.Job.ID)) ; err != nil {
return runner, err
}
logger.Debug("[JobRunner] Created env file: %s", file.Name())
runner.envFile = fileWill fail to compile with agent/job_runner.go:73:51: undefined: file.
I could do:
file, err := foo()
if err != nil { }But that doesn't match the style elsewhere.
There was a problem hiding this comment.
But that doesn't match the style elsewhere.
Yup, that's fair. Match the style of the surrounding code and I'll deal with it holistically to match the golang style at some point.
agent/job_runner.go
Outdated
| if r.envFile != nil { | ||
| if err := os.Remove(r.envFile.Name()); err != nil { | ||
| logger.Warn("[JobRunner] Error cleaning up env file: %s", err) | ||
| } else { |
There was a problem hiding this comment.
Same as above with else block.
|
Thanks @DazWorrall will talk over with @keithpitt! |
|
Cool @keithpitt and I discussed it. I think we will subsequently write env updates to that file in our bootstrap, but that shouldn't affect you, or if it does you could take a snapshot of the env file in a bootstrap script or the env hook. Beyond that, the only concern that popped up was how we'd handle newlines in env variables. Docker is notoriously bad at this moby/moby#12997 docker/compose#3527. This can be hacked around if needed at least. I'm going to merge this and then do some testing. Thanks so much driving this discussion and for the contribution! We'll endeavour to get this out in a release soon. |
|
We already sanitise variables before passing them to docker FYI, not our first brush with newlines 🙂 |
|
@lox Thanks for shipping this quickly. I have done some testing and we're running into issues with the newlines in environment variables (as you pointed out already). We have concluded that encoding the output file as JSON would solve our issues. That would make it trivial to parse for our use case. |
|
Another option would be the export format we parse here https://github.com/buildkite/agent/blob/master/env/export.go |
Partially addresses #615
This fulfils our desire to present only a clean environment to docker container (our runner gear can check for the presence of this file), and a future
buildkite-agent envcommand can simply echo this back if it exists locally, before falling back to agent API.WDYT @lox?