close
Skip to content

Ingress network should not be attachable#2523

Merged
dperny merged 1 commit intomoby:masterfrom
fcrisciani:no-ingress-attach
Feb 22, 2018
Merged

Ingress network should not be attachable#2523
dperny merged 1 commit intomoby:masterfrom
fcrisciani:no-ingress-attach

Conversation

@fcrisciani
Copy link
Copy Markdown

Ingress network is a special network used only to expose
ports. For this reason the network cannot be explicitly
attached during service create or service update

Signed-off-by: Flavio Crisciani flavio.crisciani@docker.com

Ingress network is a special network used only to expose
ports. For this reason the network cannot be explicitly
attached during service create or service update

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 21, 2018

Codecov Report

Merging #2523 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2523      +/-   ##
==========================================
- Coverage   61.35%   61.33%   -0.02%     
==========================================
  Files         133      133              
  Lines       21724    21726       +2     
==========================================
- Hits        13328    13325       -3     
- Misses       6951     6959       +8     
+ Partials     1445     1442       -3

@dperny
Copy link
Copy Markdown
Collaborator

dperny commented Feb 22, 2018

So, wait, was this really just a case of the validation logic for networks looking at the wrong field?

Copy link
Copy Markdown
Contributor

@anshulpundir anshulpundir left a comment

Choose a reason for hiding this comment

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

Will this break an existing workflows in moby or UCP ?

@fcrisciani
Copy link
Copy Markdown
Author

@dperny yep looks like for this specific case the deprecation of the field, forgot to update the spec validation with the proper new designated field for what concerns the ServiceCreate call. For the ServiceUpdate instead there was no check at all.

@anshulpundir there should not be any existing workflows that where explicitly attaching the ingress network, so if there were any they were already broken

@dperny
Copy link
Copy Markdown
Collaborator

dperny commented Feb 22, 2018

LGTM. Turns out because this is just updating the check of a deprecated field to a check of a current one, I'm gonna merge off just our two LGTMs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants