Make preservation of envvars case insensitive on windows #52

Merged
merged 2 commits into from Feb 24, 2015

Conversation

Projects
None yet
2 participants
Contributor

bz2 commented Feb 24, 2015

Treat envvars as case insenstive when whitelisting in OsEnvSuite

Because windows envvars are case insensitive but case preserving, it's possible to have a valid common value, like PATH, which would be accepted by the OS, get scrubbed. This branch changes the whitelisting to use lower cased comparisons on windows.

osenv.go
- envList = windowsVariables
+ // Lowercase variable names for comparison as they are case
+ // insenstive on windows. Fancy folding not required for ascii.
+ lowerEnv := make(map[string]string)
@natefinch

natefinch Feb 24, 2015

Contributor

this should probably be map[string]struct{} ... I'm sure the size isn't a big deal, but since we never use the value, it would be nice to make it a little more clear that the value doesn't matter.

Also, if you know the size ahead of time, you can give make a size hint:

lowerEnv := make(map[string]struct{}, len(windowsVariables) + len(testingVariables))

Contributor

natefinch commented Feb 24, 2015

LGTM with the suggestion above.

bz2 pushed a commit that referenced this pull request Feb 24, 2015

Merge pull request #52 from bz2/windows_env_insenstive
Make preservation of envvars case insensitive on windows

@bz2 bz2 merged commit c230cba into juju:master Feb 24, 2015

@bz2 bz2 deleted the bz2:windows_env_insenstive branch Feb 24, 2015

jujubot added a commit to juju/juju that referenced this pull request Feb 26, 2015

Merge pull request #1671 from bz2/dep_testing_c230cbad
Update juju/testing dependency for windows envvar case fix

Picks up the fix made to OsEnvSuite which is used by juju.

<juju/testing#52>

The hope is this will fix some of the failing windows tests under CI.

(Review request: http://reviews.vapour.ws/r/999/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment