Skip to content

controlapi: Allow multiple randomly assigned published ports on service#1657

Merged
aluzzardi merged 1 commit intomoby:masterfrom
aaronlehmann:published-port
Oct 18, 2016
Merged

controlapi: Allow multiple randomly assigned published ports on service#1657
aluzzardi merged 1 commit intomoby:masterfrom
aaronlehmann:published-port

Conversation

@aaronlehmann
Copy link
Copy Markdown
Collaborator

This fixes a 1.12.2 regression.

See moby/moby#27469

cc @thaJeztah @allencloud


portSet := make(map[portSpec]struct{})
for _, port := range epSpec.Ports {
if port.PublishedPort == 0 {
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.

Does this need a comment? (e.g., if no host port is assigned, ports can never conflict)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment.

@thaJeztah
Copy link
Copy Markdown
Member

Thanks @aaronlehmann, makes sense that this is the issue (I was thinking way to complicated 😄)

@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 18, 2016

Current coverage is 56.77% (diff: 100%)

Merging #1657 into master will decrease coverage by 0.04%

@@             master      #1657   diff @@
==========================================
  Files            90         90          
  Lines         14574      14567     -7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           8280       8270    -10   
- Misses         5207       5213     +6   
+ Partials       1087       1084     -3   

Sunburst

Powered by Codecov. Last update 9ff3dfd...5d1faa8

This fixes a 1.12.2 regression.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Copy link
Copy Markdown
Contributor

@mrjana mrjana left a comment

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.

6 participants