cli: add a separate --domainname flag#1130
Merged
albers merged 1 commit intodocker:masterfrom Dec 7, 2018
Merged
Conversation
Contributor
Author
|
It's also related to moby/moby#37302. |
307aeaf to
f0ba2fc
Compare
Contributor
Author
|
This should be ready to review. It has tests and documentation (though it depends somewhat on the server-side changes as well). |
f0ba2fc to
6e3d549
Compare
Contributor
Author
|
Alright, I've fixed the build by removing the Swarm changes -- it looks like Swarm doesn't have a Domainname field in the API so we can't really add a switch for it. /ping @vdemeester @thaJeztah |
Contributor
Author
|
moby/moby#37302 was merged, so this should be ready-to-go. |
albers
reviewed
Dec 1, 2018
Collaborator
albers
left a comment
There was a problem hiding this comment.
I successfully tested run and create, but the feature seems to be absent for service create|update:
$ build/docker run --help | grep domainname
--domainname string Container NIS domain name
$ build/docker create --help | grep domainname
--domainname string Container NIS domain name
$ build/docker service create --help | grep domainname
$ build/docker service update --help | grep domainname
$ build/docker service create --hostname some-host --domainname --some-domain nginx
unknown flag: --domainname
See 'docker service create --help'.
$ build/docker --version
Docker version 18.06.0-dev, build 6e3d549418b591e to
ed970f2
Compare
Codecov Report
@@ Coverage Diff @@
## master #1130 +/- ##
==========================================
+ Coverage 55.22% 55.23% +<.01%
==========================================
Files 289 289
Lines 19389 19391 +2
==========================================
+ Hits 10708 10710 +2
Misses 7984 7984
Partials 697 697 |
Codecov Report
@@ Coverage Diff @@
## master #1130 +/- ##
==========================================
+ Coverage 55.22% 55.23% +<.01%
==========================================
Files 289 289
Lines 19389 19391 +2
==========================================
+ Hits 10708 10710 +2
Misses 7984 7984
Partials 697 697 |
3fcb197 to
4fc223c
Compare
Contributor
Author
|
/cc @albers |
albers
requested changes
Dec 7, 2018
Collaborator
albers
left a comment
There was a problem hiding this comment.
LGTM, but please remove the unrelated doc change.
A while ago, Docker split the "Domainname" field out from the "Hostname" field for the container configuration. There was no real user-visible change associated with this (and under the hood "Domainname" was mostly left unused from the command-line point of view). We now add this flag in order to match other proposed changes to allow for setting the NIS domainname of a container. This also includes a fix for the --hostname parsing tests (they would not error out if only one of .Hostname and .Domainname were incorrectly set -- which is not correct). Signed-off-by: Aleksa Sarai <asarai@suse.de>
4fc223c to
6475790
Compare
Contributor
Author
|
@albers Removed the formatting change. |
albers
approved these changes
Dec 7, 2018
Collaborator
|
Thanks for the contribution, @cyphar! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A while ago, Docker split the "Domainname" field out from the "Hostname"
field for the container configuration. There was no real user-visible
change associated with this (and under the hood "Domainname" was mostly
left unused from the command-line point of view). We now add this flag
in order to match other proposed changes to allow for setting the NIS
domainname of a container.
Cute cat by .:I'm-a-kitty-cat!:.
Signed-off-by: Aleksa Sarai asarai@suse.de