Skip to content

Reverse order of long-form ports - #2252

Merged
thaJeztah merged 1 commit into
docker:masterfrom
zappy-shu:2250-reverse-port-long-format-order
Jan 15, 2020
Merged

thaJeztah merged 1 commit into
docker:masterfrom
zappy-shu:2250-reverse-port-long-format-order

Conversation

@zappy-shu

Copy link
Copy Markdown

- What I did

Reverses the order long-form port options when converted to short-form to correctly match the documentation and docker service create. See https://docs.docker.com/engine/reference/commandline/service_create/#publish-service-ports-externally-to-the-swarm--p---publish

Post change -p published=8111,target=8112 is the equivalent of 8111:8112

- How I did it

Changed the order of the values when converting from long to short-form when the options are parsed

- How to verify it

Build and run docker run -d --name run_test -p published=8111,target=80 nginx. nginx should be accessible from the host with port 8111

- Description for the changelog

Fixes #2250

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

cat-2019-11-19

@silvin-lubecki silvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The change looks good, but do you think we can add a test on this fix @zappy-shu ?

@thaJeztah

Copy link
Copy Markdown
Member

Looks like this was added in 29612cc, which was part of #1102

So this issue affects docker 18.09 and 19.03

Reverses the order long-form port options when converted to short-form
to correctly match the documentation and `docker service create`.

Post change `-p published=8111,target=8112` is the equivalent of
`8111:8112`

Signed-off-by: Nick Adcock <nick.adcock@docker.com>
@zappy-shu
zappy-shu force-pushed the 2250-reverse-port-long-format-order branch from c7916e4 to 154a1f6 Compare January 15, 2020 12:12
@zappy-shu

Copy link
Copy Markdown
Author

@silvin-lubecki PTAL

@silvin-lubecki silvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still LGTM 👍

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

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.

docker run -p with new syntax is backwards

4 participants