Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
add Setenv function #273
Conversation
| + {"foo=1", []string{"foo=1", "arble="}}, | ||
| + {"foo=", []string{"foo=", "arble="}}, | ||
| + {"arble=23", []string{"foo=bar", "arble=23"}}, | ||
| + {"zaphod=42", []string{"foo=bar", "arble=", "zaphod=42"}}, |
|
$$merge$$ |
jameinel
requested changes
Apr 20, 2017
seeing Setenv("foo=") makes me wonder if we would want Delenv.
| +func Setenv(env []string, entry string) []string { | ||
| + i := strings.Index(entry, "=") | ||
| + if i == -1 { | ||
| + panic("no = in environment entry") |
jameinel
Apr 20, 2017
Owner
Anything that would be actively used in production, should really not have a panic() in it, since that tends to have the property of crashing all of Juju instead of just having one worker get restarted.
| + if i == -1 { | ||
| + panic("no = in environment entry") | ||
| + } | ||
| + prefix := entry[0 : i+1] |
| + | ||
| +var _ = gc.Suite(&SetenvSuite{}) | ||
| + | ||
| +var setenvTests = []struct { |
jameinel
Apr 20, 2017
Owner
It seems odd to have these as a slice outside of the function given that they depend so concretely on the initial setup.
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-utils |
jujubot
merged commit 548369f
into
juju:master
Apr 20, 2017
rogpeppe
referenced this pull request
Apr 20, 2017
Merged
make Setenv ignore malformed entries rather than panic #274
added a commit
that referenced
this pull request
Apr 20, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
rogpeppe commentedApr 20, 2017
This makes it easier to override environment variables
since just appending environment variables doesn't
work any more (and never did for non-Go programs).
See golang/go#19877 for
background.