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

cli: add a separate --domainname flag #1130

Merged
merged 1 commit into from
Dec 7, 2018

Conversation

cyphar
Copy link
Contributor

@cyphar cyphar commented Jun 18, 2018

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!:.

Cute cat by .:I'm-a-kitty-cat!:.

Signed-off-by: Aleksa Sarai asarai@suse.de

@cyphar
Copy link
Contributor Author

cyphar commented Jun 18, 2018

This is still WIP because I haven't included any documentation changes. Documentation has been added.

It's also related to moby/moby#37302.

@cyphar
Copy link
Contributor Author

cyphar commented Jun 18, 2018

This should be ready to review. It has tests and documentation (though it depends somewhat on the server-side changes as well).

@cyphar
Copy link
Contributor Author

cyphar commented Aug 16, 2018

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

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.

SGTM 🐯

@cyphar
Copy link
Contributor Author

cyphar commented Dec 1, 2018

moby/moby#37302 was merged, so this should be ready-to-go.

Copy link
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

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 6e3d5494

contrib/completion/bash/docker Outdated Show resolved Hide resolved
@cyphar cyphar force-pushed the separate-domainname-flag branch 2 times, most recently from 18b591e to ed970f2 Compare December 2, 2018 13:22
@codecov-io
Copy link

Codecov Report

Merging #1130 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            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-io
Copy link

codecov-io commented Dec 2, 2018

Codecov Report

Merging #1130 into master will increase coverage by <.01%.
The diff coverage is 100%.

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

@cyphar cyphar force-pushed the separate-domainname-flag branch 2 times, most recently from 3fcb197 to 4fc223c Compare December 3, 2018 10:41
@cyphar
Copy link
Contributor Author

cyphar commented Dec 7, 2018

/cc @albers

Copy link
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

LGTM, but please remove the unrelated doc change.

docs/reference/commandline/service_create.md Outdated Show resolved Hide resolved
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>
@cyphar
Copy link
Contributor Author

cyphar commented Dec 7, 2018

@albers Removed the formatting change.

@albers albers merged commit 561f6e3 into docker:master Dec 7, 2018
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Dec 7, 2018
@albers
Copy link
Collaborator

albers commented Dec 7, 2018

Thanks for the contribution, @cyphar!

@cyphar cyphar deleted the separate-domainname-flag branch December 7, 2018 15:42
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.

6 participants