-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
adding LABEL command #44
adding LABEL command #44
Conversation
pkg/commands/label.go
Outdated
|
||
for _, kvp := range labels { | ||
logrus.Infof("Applying label %s=%s", kvp.Key, kvp.Value) | ||
existingLabels[kvp.Key] = kvp.Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might need to parse out backslashes or quotes before setting the label (I had to for the ENV command, so this might be similar)?
for example, the command
LABEL myDog=Rex\ The\ Dog \
in the Dockerfile should be saved as myDog="Rex The Dog"
in the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
existingLabels := config.Labels | ||
|
||
// Let's unescape values before setting the label | ||
shlex := shell.NewLex('\\') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there are two different escape tokens, if you pass in a string version of the command (like here) then the parser will determine which one it is for you, and you can pass that into shell.NewLex()
pkg/commands/label_test.go
Outdated
expectedLabels := map[string]string{ | ||
"foo": "override", | ||
"bar": "baz", | ||
"multiword": "lots of words", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this should parse to "lots\ of\ words", since the first backslash should indicate that the second remains. Hopefully if you make the change above, this will just parse correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, sorry, it actually should stay "lots\\ of\\ words" in the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, if I pass in "lots\\\\ of\\\\ words"
then I'll get "lots\ of\ words"
back out.
Fixes #7