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

Add support for .Node.Hostname templating in swarm services #34686

Merged
merged 1 commit into from Sep 14, 2017

Conversation

@mion00
Copy link
Contributor

@mion00 mion00 commented Aug 30, 2017

This is a follow up on #34061 and docker/swarmkit#1951

Signed-off-by: Carlo Mion mion00@gmail.com

- What I did
Add support for templating Node.Hostname in swarm services

- How I did it
Creating a new field in the docker executor, with informations about the node

- How to verify it
I modified an already present integration test to use the templated hostname.
You can also verify manually creating a swarm service using {{.Node.Hostname}} in one of the supported fields, such as labels, env or hostname, and verify that the newly created container has the appropriate value taken from the node hostname.

- Description for the changelog
Add support for {{.Node.Hostname}} in swarm service templates

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

@mion00 mion00 requested review from dnephin and vdemeester as code owners Aug 30, 2017
@mion00 mion00 force-pushed the mion00:templating-node-hostname-support branch 2 times, most recently from a59836f to b193549 Aug 31, 2017
@@ -5,6 +5,8 @@ import (
"sort"
"strings"

"sync"

This comment has been minimized.

@aaronlehmann

aaronlehmann Sep 2, 2017
Contributor

This "sync" line doesn't need to be separated from the other standard library imports with a blank line.

This comment has been minimized.

@mion00

mion00 Sep 2, 2017
Author Contributor

fixed that, it was the IDE automatically adding that line..

Signed-off-by: Carlo Mion <mion00@gmail.com>
@mion00 mion00 force-pushed the mion00:templating-node-hostname-support branch from b193549 to e2f09fa Sep 2, 2017
@rdxmb
Copy link

@rdxmb rdxmb commented Sep 13, 2017

I am really looking forward to having this feature. Especially for monitoring with things like telegraf this will be very helpful. Any plans getting this merged?

Copy link
Member

@vdemeester vdemeester left a comment

LGTM 🐯

Copy link
Member

@yongtang yongtang left a comment

LGTM

@yongtang yongtang merged commit 2ee8ef8 into moby:master Sep 14, 2017
6 checks passed
6 checks passed
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 36614 has succeeded
Details
janky Jenkins build Docker-PRs 45243 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 5643 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 16710 has succeeded
Details
z Jenkins build Docker-PRs-s390x 5422 has succeeded
Details
@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Sep 14, 2017

@mion00 can you open a follow up PR to update the documentation accordingly? This new option needs to be added to the templating section here: https://github.com/docker/cli/blob/master/docs/reference/commandline/service_create.md#create-services-using-templates

mion00 added a commit to mion00/cli that referenced this pull request Sep 15, 2017
Update placeholders table and add example code
Follow up to moby/moby#34686
mion00 added a commit to mion00/cli that referenced this pull request Sep 15, 2017
Update placeholders table and add example code
Follow up to moby/moby#34686

Signed-off-by: Carlo Mion <mion00@gmail.com>
mion00 added a commit to mion00/cli that referenced this pull request Sep 15, 2017
Update placeholders table and add example code
Follow up to moby/moby#34686

Signed-off-by: Carlo Mion <mion00@gmail.com>
@thaJeztah thaJeztah removed the docs/revisit label Sep 15, 2017
riyazdf added a commit to riyazdf/cli that referenced this pull request Sep 18, 2017
Update placeholders table and add example code
Follow up to moby/moby#34686

Signed-off-by: Carlo Mion <mion00@gmail.com>
riyazdf added a commit to riyazdf/cli that referenced this pull request Sep 18, 2017
Update placeholders table and add example code
Follow up to moby/moby#34686

Signed-off-by: Carlo Mion <mion00@gmail.com>
riyazdf added a commit to riyazdf/cli that referenced this pull request Sep 18, 2017
Update placeholders table and add example code
Follow up to moby/moby#34686

Signed-off-by: Carlo Mion <mion00@gmail.com>
andrewhsu pushed a commit to docker/docker-ce that referenced this pull request Sep 19, 2017
Update placeholders table and add example code
Follow up to moby/moby#34686

Signed-off-by: Carlo Mion <mion00@gmail.com>
Upstream-commit: 21825b68420a9114ca2cb457fd1b7efeacfcf815
Component: cli
@albers
Copy link
Member

@albers albers commented Nov 14, 2017

@andrewhsu I think this nice feature should be added to the 17.11 release notes.

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 14, 2017

Thanks @albers - I did a quick check and it looks like it was already included in the 17.10 release https://github.com/docker/docker-ce/blame/17.10/components/engine/daemon/cluster/executor/container/adapter.go#L44, and included in those release nodes; https://github.com/docker/docker-ce/releases/tag/v17.10.0-ce

@albers
Copy link
Member

@albers albers commented Nov 14, 2017

@thaJeztah Oh yes, you're right. I was so sure I tested this against 17.10 but it must have been 17.09 then. Thanks a lot for sorting this out.

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 14, 2017

No worries thanks! Could've been we missed it

@tgupta1419
Copy link

@tgupta1419 tgupta1419 commented Nov 30, 2018

Is this available in 17.12.1.ce-1

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 30, 2018

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

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.