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

Fix --label-file weird behavior #838

Merged
merged 1 commit into from
Jan 30, 2018

Conversation

vdemeester
Copy link
Collaborator

@vdemeester vdemeester commented Jan 26, 2018

--label-file has the exact same behavior as --env-file, meaning any
placeholder (i.e. a simple key, no = sign, no value), it will get the
value from the environment variable.

For --label-file it should just add an empty label.

(Same for --label, it should never get value from the environment variables).

Fix moby/moby#31087.

🐅 🦁

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

@codecov-io
Copy link

codecov-io commented Jan 26, 2018

Codecov Report

Merging #838 into master will decrease coverage by 0.01%.
The diff coverage is 64.44%.

@@            Coverage Diff             @@
##           master     #838      +/-   ##
==========================================
- Coverage   52.95%   52.93%   -0.02%     
==========================================
  Files         244      245       +1     
  Lines       15839    15837       -2     
==========================================
- Hits         8387     8384       -3     
- Misses       6898     6899       +1     
  Partials      554      554

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! Left a suggestion

opts/parse.go Outdated
@@ -10,10 +10,10 @@ import (

// ReadKVStrings reads a file of line terminated key=value pairs, and overrides any keys
// present in the file with additional pairs specified in the override parameter
func ReadKVStrings(files []string, override []string) ([]string, error) {
func ReadKVStrings(files []string, override []string, emptyFn func(string) string) ([]string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

A couple of lines below, this still refers to environment variables.

Perhaps we should (for now)

  • un-export ParseKeyValueFile
  • have two wrappers; one variation of ReadKVString for labels, and one for environment variables
  • those wrappers don't need the emptyFn as an argument (while nice, we don't really need that functionality at this point)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😝 I can take care of that 👼

`--label-file` has the exact same behavior as `--env-file`, meaning any
placeholder (i.e. a simple key, no `=` sign, no value), it will get the
value from the environment variable.

For `--label-file` it should just add an empty label.

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

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah merged commit 9de1b16 into docker:master Jan 30, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.02.0 milestone Jan 30, 2018
@vdemeester vdemeester deleted the fix-label-file-behavior branch January 30, 2018 01:33
@thaJeztah thaJeztah modified the milestones: 18.02.0, 18.03.0 Jan 31, 2018
@vdemeester
Copy link
Collaborator Author

Ok this caused a regression, I'll fix it 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants