Skip to content

Display empty string instead of <nil> when IP opt is nil.#13879

Merged
duglin merged 1 commit intomasterfrom
unknown repository
Jun 11, 2015
Merged

Display empty string instead of <nil> when IP opt is nil.#13879
duglin merged 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jun 11, 2015

Fixes #13878.

@runcom
Copy link
Copy Markdown
Member

runcom commented Jun 11, 2015

this is weird and seems a regression cause I'm not experiencing the problem on 1.7.0-rc2 but only on latest master :/
@eolamey could you please add a test?

ok found 5fa6014 :)

@runcom runcom added this to the 1.7.0 milestone Jun 11, 2015
@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 11, 2015

It is indeed a regression from 1.7.0-rc2, and comes from #13757, for which I am guilty :) The flag type was changed from flag.StringVar to opts.IPVar. On the bright side, --default-gateway doesn't work at all in 1.7.0-rc2 :)

It is not a regression in opts.IPVar per se, as there was no flag with a nil IP address before.

Should I try to add an integration test, or a unit test in opts/opts_test.go?

@runcom
Copy link
Copy Markdown
Member

runcom commented Jun 11, 2015

@eolamey unit test make sense thanks

Display empty string instead of <nil> when IP opt is nil.
9ad8928 
Fixes #13878.

Signed-off-by: Eric-Olivier Lamey <eo@lamey.me>
@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 11, 2015

@runcom, does this look ok to you?

@runcom
Copy link
Copy Markdown
Member

runcom commented Jun 11, 2015

Yep, assuming janky is happy this LGTM

duglin
duglin reviewed Jun 11, 2015
View reviewed changes
opts/opts_test.go
Copy link
Copy Markdown
Contributor

@duglin duglin Jun 11, 2015

Choose a reason for hiding this comment

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

Should we add a non-empty case too?

Copy link
Copy Markdown
Contributor

@icecrime icecrime Jun 11, 2015

Choose a reason for hiding this comment

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

I'm not sure I understand your comment, there's both "" and "0.0.0.0" (surely non valid, but yet non-empty).

Copy link
Copy Markdown
Contributor

@duglin duglin Jun 11, 2015

Choose a reason for hiding this comment

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

doi - sorry, I misread the test. For some reason I thought that was just one test where 0.0.0.0 ended up mapping to "".
One day I'll learn to read :-)

Copy link
Copy Markdown
Contributor

@icecrime icecrime Jun 11, 2015

Choose a reason for hiding this comment

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

Reading is overrated!

@icecrime
Copy link
Copy Markdown
Contributor

icecrime commented Jun 11, 2015

LGTM! Thanks :-)

@duglin
Copy link
Copy Markdown
Contributor

duglin commented Jun 11, 2015

LGTM

duglin added a commit that referenced this pull request Jun 11, 2015
@duglin
Merge pull request #13879 from eolamey/13878-nil-ip-opt-as-empty-string
b27f960 
Display empty string instead of <nil> when IP opt is nil.
@duglin duglin merged commit b27f960 into moby:master Jun 11, 2015
@vdemeester vdemeester mentioned this pull request Jun 11, 2015
Merged
@jessfraz
Copy link
Copy Markdown
Contributor

jessfraz commented Jun 11, 2015

cherry-picked

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

Reviewers

No reviews

Assignees

@jessfraz jessfraz

Labels

status/2-code-review

Projects

None yet

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

5 participants

@runcom @icecrime @duglin @jessfraz @GordonTheTurtle