Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/invoke: ensure that custom env vars are prepended to the env #461

Merged
merged 2 commits into from
Jun 7, 2017

Conversation

ericchiang
Copy link
Contributor

Ensure that custom values take priority over values defined in the
environment by pre-pending them to the env, not appending.

closes #410

Ensure that custom values take priority over values defined in the
environment by prepending them to the env, not appending.
@squeed
Copy link
Member

squeed commented May 31, 2017

LGTM.

Copy link

@cmluciano cmluciano left a comment

Choose a reason for hiding this comment

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

lgtm

@ericchiang
Copy link
Contributor Author

bump. what's the process for merging?

@rosenhouse
Copy link
Contributor

The process for merging is described in our Acceptance policy.

I've added a unit test to cover this change. I think it's good to go.

@rosenhouse rosenhouse merged commit 449bd85 into containernetworking:master Jun 7, 2017
@ericchiang ericchiang deleted the fix-env-order branch June 7, 2017 14:08
@rosenhouse
Copy link
Contributor

From the release notes for Go 1.9 beta:

The os/exec package now prevents child processes from being created with any duplicate environment variables. If Cmd.Env contains duplicate environment keys, only the last value in the slice for each duplicate key is used.

That would seem to break the intended behavior of this PR.

@bboreham
Copy link
Contributor

Ha ha, don'tcha love it when two groups fix a problem in conflicting ways...

@rosenhouse
Copy link
Contributor

Just hit this. Had run export CNI_COMMAND=VERSION in my shell and then tried to run some tests that used invoke. Fun times debugging.

Once we figure out how to get #538 green and merge, we should flip this back to appending.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invoke.Args.AsEnv() does not override existing environment
5 participants