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

Use scope=swarm for service related network inspect and revendor docker/docker #184

Merged
merged 2 commits into from Jun 13, 2017

Conversation

Projects
None yet
6 participants
@yongtang
Member

yongtang commented Jun 13, 2017

This fix updates docker/docker to 4310f7da7e6bcd8185bf05e032f9b7321cfa6ea2 and use scope=swarm for service related network inspect.

This fix use scope=swarm for service related network inspect. The purpose is that, in case multiple networks with the same name exist in different scopes, it is still possible to obtain the network for services.

This fix is related to #167, moby/moby#30897, moby/moby#33561, moby/moby#30242

Update docker/docker to 4310f7da7e6bcd8185bf05e032f9b7321cfa6ea2
This fix updates docker/docker to 4310f7da7e6bcd8185bf05e032f9b7321cfa6ea2

This fix is related to moby/moby#33630 and #167

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 13, 2017

Codecov Report

Merging #184 into master will decrease coverage by <.01%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
- Coverage   45.34%   45.33%   -0.01%     
==========================================
  Files         171      171              
  Lines       11482    11480       -2     
==========================================
- Hits         5206     5204       -2     
  Misses       5979     5979              
  Partials      297      297

codecov-io commented Jun 13, 2017

Codecov Report

Merging #184 into master will decrease coverage by <.01%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
- Coverage   45.34%   45.33%   -0.01%     
==========================================
  Files         171      171              
  Lines       11482    11480       -2     
==========================================
- Hits         5206     5204       -2     
  Misses       5979     5979              
  Partials      297      297
@vdemeester

LGTM 🐯

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jun 13, 2017

Collaborator

Sorry for my ignorance about network scoping, but I see that the node-local networks PR (moby/moby#32981) says it added the scope=swarm option to network create. Will the change here work correctly with networks that were created before that PR, and networks that are not node-local?

The only comment I have on the code itself is that there are many places that specify Verbose: false, but these are not needed because Verbose will default to false.

Collaborator

aaronlehmann commented Jun 13, 2017

Sorry for my ignorance about network scoping, but I see that the node-local networks PR (moby/moby#32981) says it added the scope=swarm option to network create. Will the change here work correctly with networks that were created before that PR, and networks that are not node-local?

The only comment I have on the code itself is that there are many places that specify Verbose: false, but these are not needed because Verbose will default to false.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jun 13, 2017

Collaborator

I believe network scopes have existed since swarm was integrated, we just started exposing them to new endpoints more recently. So every swarm network should already have scope=swarm even if it was created before that PR.

Collaborator

dnephin commented Jun 13, 2017

I believe network scopes have existed since swarm was integrated, we just started exposing them to new endpoints more recently. So every swarm network should already have scope=swarm even if it was created before that PR.

@dnephin

+1 to @aaronlehmann comment about removing Verbose: false, since it's default.

How will this work with older daemons that don't accept that query param? I believe they will just ignore the unexpected param, which means the behaviour is not any different from using an older client. Does that sound right?

Use `scope=swarm` for service related network inspect.
This fix use `scope=swarm` for service related network inspect.
The purpose is that, in case multiple networks with the same
name exist in different scopes, it is still possible to obtain
the network for services.

This fix is related to moby/moby#33630 and #167

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jun 13, 2017

Member

Thanks all for the review. The PR has been updated and now the unnecessary Verbose: false has been removed.

For moby/moby#32981, it adds allows a node-local network to be promoted to the swarm scope. Once a previous node-local network is promoted to swarm scope, it will be possible to be used by service created. For that the scope=swarm option passed in this PR is consistent with moby/moby#32981

Member

yongtang commented Jun 13, 2017

Thanks all for the review. The PR has been updated and now the unnecessary Verbose: false has been removed.

For moby/moby#32981, it adds allows a node-local network to be promoted to the swarm scope. Once a previous node-local network is promoted to swarm scope, it will be possible to be used by service created. For that the scope=swarm option passed in this PR is consistent with moby/moby#32981

@dnephin

LGTM

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jun 13, 2017

Collaborator

LGTM

Collaborator

aaronlehmann commented Jun 13, 2017

LGTM

@aaronlehmann aaronlehmann merged commit 25bc9f1 into docker:master Jun 13, 2017

3 of 4 checks passed

codecov/patch 25% of diff hit (target 50%)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 45.33% (-0.01%) compared to 7baaa90
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.07.0 milestone Jun 13, 2017

@yongtang yongtang deleted the yongtang:167-network-inspect-scope branch Jun 13, 2017

yongtang added a commit to yongtang/docker that referenced this pull request Jun 15, 2017

Update docker/cli to 4c224a778638da8664b963d95948f0de8149a988
This fix update docker/cli to 4c224a778638da8664b963d95948f0de8149a988

This fix brings change of docker/cli#184 to docker, which is needed by moby#30897

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Jun 15, 2017

Convert command line test to api test.
This commit converts command line test to api test for

DockerSwarmSuite.TestServiceCreateWithDuplicateNetworkNames

This fix is related to docker/cli#184

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment