Remove --port and update --publish for services to support syntaxes by vdemeester · Pull Request #28943 · moby/moby

vdemeester · 2016-11-29T17:26:06Z

This is part of #28527 — "The newly introduced --port flag should likely disappear in favor of --publish, ensuring that the short form maps cleanly to the long form"

Removing --port, --port-add, --port-rm flags
Add support for simple and complex syntax on --publish flag (and --publish-add, --publish-rm), using PortOpt
Validate the changes on PortOpt with unit tests.

$ docker service create --name t1 --publish 81:81/tcp --publish target=80,published=80,protocol=tcp,mode=ingress busybox top
# […]
$ docker service update --publish-add 82 --publish-rm 80:80/tcp --publish-add target=83 --publish-rm target=80 t1
# […]

Probably need a small refactoring to share the short/long syntax detection
Add one integration test to validate it works (but not all possible cases, covered by unit tests)

/cc @thaJeztah @icecrime @dnephin @mavenugo

🐸

Signed-off-by: Vincent Demeester [email protected]

icecrime · 2016-11-29T17:34:09Z

Cc @vieux: I'm not sure if you want to take this for 1.13.0.

thaJeztah · 2016-11-29T20:56:41Z

cli/command/service/opts.go

No, --port was added in 1.13, and is not present in 1.12

@thaJeztah you are too quick for me 😅

LK4D4 · 2016-11-30T20:17:14Z

LGTM
ping @vieux

mavenugo

Thanks @vdemeester
minor comments on tests. I will try it out and give more comments if any.

mavenugo · 2016-12-07T16:39:11Z

opts/port_test.go

Good tests. Can you also pls include a "ingress" mode test ?

ah you're rihgt, I thought there was one, my bad 😅

mavenugo · 2016-12-07T17:04:02Z

opts/port_test.go

Is there a reason to not check for other default states such as Protocol, PublishMode ?
I think it is important to make sure the default protocol (tcp) and publishmode (ingress) are parsed appropriately

I should have added comment. This one is just about the required field (target here). The default protocol and publish mode are handled server side so the client doesn't send them (that's why the Swarm.PortConfig is that plain).

dnephin

LGTM

dnephin · 2016-12-07T18:27:18Z

cli/command/service/opts.go

This line is unnecessary now

Oh right 😅

mavenugo · 2016-12-08T11:29:55Z

Thanks @vdemeester

LGTM

thaJeztah · 2016-12-08T13:29:09Z

@vdemeester can you also update the completion scripts?

https://github.com/docker/docker/blob/9ec471049d4a435a489fa27767faebadb0f66e3c/contrib/completion/bash/docker#L2755

https://github.com/docker/docker/blob/d23bb3d0831fa200d45510e9f85b0086b2533553/contrib/completion/zsh/_docker#L1798

Should we update the changelog as part of this PR as well? (I suggest to add both the "old" and "new" PR as a link); https://github.com/docker/docker/blob/caaa52c124539acb396c0b711ab95bc7ed459540/CHANGELOG.md#L59

vdemeester · 2016-12-08T13:30:08Z

ah right 👼

vdemeester · 2016-12-08T13:33:16Z

@thaJeztah updated 👼

mavenugo · 2016-12-08T19:53:41Z

@vdemeester rebase needed :(

thaJeztah · 2016-12-08T21:10:56Z

Found one issue; if defaults are omitted during service update, the previous value is not changed;

publish on port 8080, "ingress" mode

docker service create --publish published=8080,target=80,protocol=tcp --name web nginx:alpine

docker service inspect --format '{{ json .Spec.EndpointSpec.Ports }}' web
[{"Protocol":"tcp","TargetPort":80,"PublishedPort":8080,"PublishMode":"ingress"}]

change to "host" mode

docker service update --publish-rm 80 --publish-add published=8080,target=80,protocol=tcp,mode=host web

docker service inspect --format '{{ json .Spec.EndpointSpec.Ports }}' web
[{"Protocol":"tcp","TargetPort":80,"PublishedPort":8080,"PublishMode":"host"}]

change back to "ingress" mode

docker service update --publish-rm 80 --publish-add published=8080,target=80,protocol=tcp web

docker service inspect --format '{{ json .Spec.EndpointSpec.Ports }}' web
[{"Protocol":"tcp","TargetPort":80,"PublishedPort":8080,"PublishMode":"host"}]

publish on port 8080, "tcp" (default)

docker service create --publish published=8080,target=80 --name web nginx:alpine

docker service inspect --format '{{ json .Spec.EndpointSpec.Ports }}' web
[{"Protocol":"tcp","TargetPort":80,"PublishedPort":8080,"PublishMode":"ingress"}]

change to "udp"

docker service update --publish-rm 80 --publish-add published=8080,target=80,protocol=udp web

docker service inspect --format '{{ json .Spec.EndpointSpec.Ports }}' web
[{"Protocol":"udp","TargetPort":80,"PublishedPort":8080,"PublishMode":"ingress"}]

change back to "tcp" (default)

docker service update --publish-rm 80 --publish-add published=8080,target=80 web

docker service inspect --format '{{ json .Spec.EndpointSpec.Ports }}' web
[{"Protocol":"udp","TargetPort":80,"PublishedPort":8080,"PublishMode":"ingress"}]

vdemeester · 2016-12-08T21:33:06Z

So… this is conflicting with #25860 😅 I temporarly disabled the validation and added comment because I'm not sure about what to do…

vdemeester · 2016-12-08T22:57:47Z

Please look closely at 9d4aa36 (this fixes @thaJeztah's comment BUT changes a little bit the UX to something that feels more natural).

With that commit, --publish-add 8081:81 --publish-add 8082:82 --publish-rm 80 --publish-rm 81/tcp --publish-rm 82/ucp would thus result in 81 and 82 to be published (instead of only 82 currently).

/cc @dnephin @thaJeztah @icecrime @mavenugo @aaronlehmann @stevvooe

Add support for simple and complex syntax to `--publish` through the use of `PortOpt`. Signed-off-by: Vincent Demeester <[email protected]>

`--publish-add 8081:81 --publish-add 8082:82 --publish-rm 80 --publish-rm 81/tcp --publish-rm 82/tcp` would thus result in 81 and 82 to be published. Signed-off-by: Vincent Demeester <[email protected]>

vdemeester · 2016-12-11T22:15:22Z

Rebased 👼

thaJeztah · 2016-12-13T15:20:35Z

Testing my previous examples, I ran into this; adding a port in mode "host" cannot be removed using the "simple" syntax

$ docker service create --publish published=8080,target=80,protocol=tcp,mode=host --name web nginx:alpine
oq5jnkhug6hjddtewift80dz9

$ docker service update --publish-rm 80 web
web

$ docker service inspect --format '{{ json .Spec.EndpointSpec.Ports }}' web
[{"Protocol":"tcp","TargetPort":80,"PublishedPort":8080,"PublishMode":"host"}]

This worked;

$ docker service update --publish-rm target=80,mode=host web
web

$ docker service inspect --format '{{ json .Spec.EndpointSpec.Ports }}' web
null

mavenugo · 2016-12-14T09:12:48Z

@thaJeztah @vdemeester @vieux we need this for 1.13.0 and I think @thaJeztah's UX observation is expected. Do we have any concerns with this PR still to be merged and cherry-picked ?

thaJeztah · 2016-12-14T10:20:49Z

@mavenugo I'm good with this PR, just wanted confirmation that we agree on the change in behavior, i.e.;

previously, when having both --publish-rm, and --publish-add, --publish-rm would always be executed last. This meant that you could not "replace" a port in a single update.
the new change always performs --publish-rm first, then adds the new ports specified in --publish-add.

I think this was an oversight in the original implementation, and I cannot think of a practical use of adding a new port and removing it in the same update. Similarly, I cannot think of a case where this would be a real "breaking" change.

If we agree on this, I suggest we look at other -rm and -add flags, which currently may use the "wrong" behavior as well (at least secrets)

mavenugo · 2016-12-14T16:52:27Z

@thaJeztah I agree. the current behaviour is more correct than the previous one.
This PR already has my LGTM.

thaJeztah · 2016-12-14T16:55:45Z

ok, let's go for it 👍

Remove --port and update --publish for services to support syntaxes

GordonTheTurtle added the status/0-triage label Nov 29, 2016

vdemeester added status/2-code-review process/cherry-pick and removed status/0-triage labels Nov 29, 2016

vdemeester added this to the 1.13.0 milestone Nov 29, 2016

thaJeztah reviewed Nov 29, 2016

View reviewed changes

vdemeester force-pushed the publish-long-short-syntax branch from f9ef9e5 to 395d3bf Compare November 30, 2016 09:23

vdemeester mentioned this pull request Nov 30, 2016

Move composefile -> engine api type conversion to cli/compose/convert #29003

Merged

vdemeester force-pushed the publish-long-short-syntax branch from 395d3bf to 0e2bbdd Compare December 6, 2016 10:54

icecrime mentioned this pull request Dec 6, 2016

[UX] Guidelines for short/long syntax flags #28527

Closed

3 tasks

mavenugo reviewed Dec 7, 2016

View reviewed changes

dnephin approved these changes Dec 7, 2016

View reviewed changes

vdemeester force-pushed the publish-long-short-syntax branch from 0e2bbdd to b7fe613 Compare December 8, 2016 10:16

mavenugo approved these changes Dec 8, 2016

View reviewed changes

vdemeester force-pushed the publish-long-short-syntax branch from b7fe613 to 6dda197 Compare December 8, 2016 13:32

vdemeester force-pushed the publish-long-short-syntax branch from 6dda197 to a05fc7a Compare December 8, 2016 21:32

vdemeester force-pushed the publish-long-short-syntax branch 2 times, most recently from ed452c7 to 7332444 Compare December 9, 2016 20:18

vdemeester added 2 commits December 11, 2016 23:14

Remove --port and update --publish for services to support syntaxes

75bf18c

Add support for simple and complex syntax to `--publish` through the use of `PortOpt`. Signed-off-by: Vincent Demeester <[email protected]>

Make --publish-rm precedes --publish-add, so that add wins

9d4aa36

`--publish-add 8081:81 --publish-add 8082:82 --publish-rm 80 --publish-rm 81/tcp --publish-rm 82/tcp` would thus result in 81 and 82 to be published. Signed-off-by: Vincent Demeester <[email protected]>

vdemeester force-pushed the publish-long-short-syntax branch from 7332444 to 9d4aa36 Compare December 11, 2016 22:15

thaJeztah merged commit 2fe62f2 into moby:master Dec 14, 2016

vdemeester deleted the publish-long-short-syntax branch December 14, 2016 17:00

vdemeester mentioned this pull request Dec 14, 2016

[1.13.x] Publish long short syntax 1 13 #29429

Merged

vieux removed the process/cherry-pick label Dec 16, 2016

This was referenced Jan 12, 2017

Add support for host mode port PublishMode in services #27917

Merged

Modifying/Replacing Swarm Mode Load Balancer Service #23813

Closed

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#28943 from vdemeester/publish-long-short-syntax

4a5426e

Remove --port and update --publish for services to support syntaxes

Conversation

vdemeester commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

icecrime commented Nov 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Nov 30, 2016

Uh oh!

mavenugo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavenugo commented Dec 8, 2016

Uh oh!

thaJeztah commented Dec 8, 2016

Uh oh!

vdemeester commented Dec 8, 2016

Uh oh!

vdemeester commented Dec 8, 2016

Uh oh!

mavenugo commented Dec 8, 2016

Uh oh!

thaJeztah commented Dec 8, 2016

Uh oh!

vdemeester commented Dec 8, 2016

Uh oh!

vdemeester commented Dec 8, 2016 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester commented Dec 11, 2016

Uh oh!

thaJeztah commented Dec 13, 2016

Uh oh!

mavenugo commented Dec 14, 2016

Uh oh!

thaJeztah commented Dec 14, 2016

Uh oh!

mavenugo commented Dec 14, 2016

Uh oh!

thaJeztah commented Dec 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vdemeester commented Nov 29, 2016 •

edited

Loading

vdemeester commented Dec 8, 2016 •

edited by thaJeztah

Loading