stage1/enterexec: remove trailing \n in environment variables.
#2901
Conversation
|
@alepuccetti do you mind filing an issue for a functional test follow-up, and assign it to you? |
|
@alepuccetti also, do you mind writing a more elaborate commit message adhering to https://github.com/coreos/rkt/blob/master/CONTRIBUTING.md#format-of-the-commit-message? |
| @@ -445,7 +445,9 @@ func (e *envFileMap) Set(s string) error { | |||
|
|
|||
| scanner := bufio.NewScanner(file) | |||
| for scanner.Scan() { | |||
| // TODO parse '\' to skip new line character | |||
| if len(scanner.Text()) == 0 { | |||
| continue | |||
There was a problem hiding this comment.
This sounds ok.
But I just noticed that the skipLine() test with len(s) == 0 below for "Empty line" is wrong because strings.SplitN(..., "=", 2) should never return an empty array of string.
There was a problem hiding this comment.
@alban len(s) == 0 is wrong but I think it should be len(s) < 2 because if a line do not have an = SplitN() returns one element. Do we want to allow that? I prefer that if the user wants an empty variable have to write variable=
There was a problem hiding this comment.
I agree, I don't want to implicitly set empty variables if the user does not write the =.
But IMO this is a detail, and it's not related to the issue being fixed.
alepuccetti
force-pushed
the
alessandro/enterexec-bugfix
branch
from
1763bc1
to
387c16e
Compare
alepuccetti
changed the title
\n in environment variables.
alepuccetti
force-pushed
the
alessandro/enterexec-bugfix
branch
from
387c16e
to
9879221
Compare
|
LGTM |
This patch returns an error if the environment file contains a malformed variable. For example, if a variable is unnamed (e.g. `=value`) or without the equals sign (e.g. `VAR_1`). Empty variables may be set by `VAR_1=`. Users may prefix with `#` or `;` those varibles to ignore. This patch skips empty lines in the environment file.
alepuccetti
force-pushed
the
alessandro/enterexec-bugfix
branch
from
9879221
to
ad5ece0
Compare
|
Patch updated, first commit addresses the main bug. Second commit addresses a bug I found while testing. |
|
Semaphoreci fails in setup phase |
|
@alepuccetti triggered rebuild on semaphore |
Loading environment retained the new line character (
\n),this produced an incorrect evaluation of the environment variables.
Fixes: #2893
Related to: #2839