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

Revert environment regexp in opts #16608

Merged
merged 1 commit into from Sep 28, 2015

Conversation

vdemeester
Copy link
Member

Remove the environment regexp introduced in #13694 as it makes some application not working anymore.

As said in #16585

Environment variable names used by the utilities in the Shell and Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase letters, digits, and the '_' (underscore) from the characters defined in Portable Character Set and do not begin with a digit. Other characters may be permitted by an implementation; applications shall tolerate the presence of such names. Uppercase and lowercase letters shall retain their unique identities and shall not be folded together. The name space of environment variable names containing lowercase letters is reserved for applications.

Even if I was liking this, as @tianon said, docker should be tolerant on the environment passed as is — separation of concern, it's up to the application to handle this or not 😅.

The only question I have on that one is the fact that ParseEnvFile behave differently from ValidateEnv as it checks from white spaces where the other don't. What should we do there ?

Fixes #16585

Signed-off-by: Vincent Demeester vincent@sbr.pm

@vdemeester vdemeester changed the title Revert environment regexp from 13694 Revert environment regexp in opts Sep 26, 2015

if !EnvironmentVariableRegexp.MatchString(variable) {
return []string{}, ErrBadEnvVariable{fmt.Sprintf("variable '%s' is not a valid environment variable", variable)}
// trim the front of a variable, but nothing else
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you should add some of the discussion in this PR (and the linked issue) in a comment (and in the commit message) to explain why we're not validating that the characters are valid for environment vars. (also to prevent someone to re-introduce the regex in future)

Copy link
Member Author

Choose a reason for hiding this comment

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

@thaJeztah true 😝

Copy link
Member Author

Choose a reason for hiding this comment

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

and done :)

@vdemeester
Copy link
Member Author

/ping @calavera @duglin

// trim the front of a variable, but nothing else
variable := strings.TrimLeft(data[0], whiteSpaces)
if strings.ContainsAny(variable, whiteSpaces) {
return []string{}, ErrBadEnvVariable{fmt.Sprintf("variable '%s' has white spaces", variable)}
Copy link
Contributor

Choose a reason for hiding this comment

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

why trim whitespace? It's technically allowed to have an env var with leading whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

@phemmer it's how it was done before the regexp so I reverted to it. But yeah.. and in ValidateEnv we do not trim leading space so.. there is something to do.. (probably removing this trim)

Copy link
Contributor

Choose a reason for hiding this comment

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

let's leave this here. I'd rather have the previous behavior, that seemed to work as expected, than a new different behavior.

@calavera
Copy link
Contributor

@vdemeester can we add a couple new regression tests to opts/envfile_test.go with special characters, like . and _ to make sure this doesn't break again?

@vdemeester
Copy link
Member Author

@calavera yes, will do 😉.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester
Copy link
Member Author

@calavera done 😉

@calavera
Copy link
Contributor

LGTM

1 similar comment
@LK4D4
Copy link
Contributor

LK4D4 commented Sep 28, 2015

LGTM

LK4D4 added a commit that referenced this pull request Sep 28, 2015
@LK4D4 LK4D4 merged commit ab8a410 into moby:master Sep 28, 2015
@vdemeester vdemeester deleted the 16585-revert-env-regexp branch September 28, 2015 20:56
@calavera calavera added impact/changelog kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. labels Sep 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/changelog kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants