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

fixes when no equal sign in label file #1416

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lifubang
Copy link
Contributor

@lifubang lifubang commented Oct 4, 2018

Signed-off-by: Lifubang lifubang@acmcoder.com

- What I did
When use --label:
./build/docker run -d --name=redis5 --label foo redis

root@dockerdemo:~/gocode/src/github.com/docker/cli# ./build/docker inspect --format '{{.Config.Labels}}' redis5
map[foo:]

When use --label-file:

root@dockerdemo:~/gocode/src/github.com/docker/cli# echo $foo
bar
root@dockerdemo:~/gocode/src/github.com/docker/cli# cat ~/labels.txt 
foo
bar=
root@dockerdemo:~/gocode/src/github.com/docker/cli# build/docker run -d --name=redis7 --label-file ~/labels.txt redis
root@dockerdemo:~/gocode/src/github.com/docker/cli# ./build/docker inspect --format '{{.Config.Labels}}' redis7
map[bar:]

label foo is lost.

The behavior of --label and --label-file is different when there is no equal sign after label variable.
- How I did it
When there is no equal sign after label variable, and no emptyFn, add it to output.

- How to verify it
After fix:

root@dockerdemo:~/gocode/src/github.com/docker/cli# build/docker run -d --name=redis8 --label-file ~/labels.txt redis
root@dockerdemo:~/gocode/src/github.com/docker/cli# ./build/docker inspect --format '{{.Config.Labels}}' redis8
map[bar: foo:]

- Description for the changelog
fixes when no equal sign in label file

- A picture of a cute animal (not mandatory but encouraged)
508c439d40d41d0a9c2e3256ee1ca5a

Signed-off-by: Lifubang <lifubang@acmcoder.com>
@codecov-io
Copy link

Codecov Report

Merging #1416 into master will increase coverage by 0.13%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #1416      +/-   ##
==========================================
+ Coverage   54.26%   54.39%   +0.13%     
==========================================
  Files         289      289              
  Lines       19331    19268      -63     
==========================================
- Hits        10490    10481       -9     
+ Misses       8165     8115      -50     
+ Partials      676      672       -4

Copy link
Collaborator

@vdemeester vdemeester 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.

Haven't checked yet, but is this same function also used for env-files? If so, should it behave the same?

We should at least have a test for this, as the behavior of empty values vs not-set is already tricky, so easy to introduce regressions / change in behavior

@vdemeester
Copy link
Collaborator

Haven't checked yet, but is this same function also used for env-files? If so, should it behave the same?

Yeah, but for env-files the emptyfn is not nil so it should still act the same 😉 But I agree on a test

@lifubang
Copy link
Contributor Author

lifubang commented Oct 4, 2018

Thank you for all your review, I'll add some test cases.

@lifubang lifubang force-pushed the labelfilenoequalsign branch 2 times, most recently from ee54671 to a2bea38 Compare October 4, 2018 11:09
Signed-off-by: Lifubang <lifubang@acmcoder.com>
Copy link
Contributor Author

@lifubang lifubang left a comment

Choose a reason for hiding this comment

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

I have add two test cases for label/env file without equal sign.

@lifubang
Copy link
Contributor Author

lifubang commented Oct 5, 2018

@thaJeztah I don't know if I have understood your points. Do you mean add some test cases, or change code to check the lines value?

@tonistiigi
Copy link
Member

ping @thaJeztah

@thaJeztah thaJeztah self-assigned this Apr 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants