-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Use scope=swarm for service related network inspect and revendor docker/docker #184
Conversation
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 Report
@@ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐯
Sorry for my ignorance about network scoping, but I see that the node-local networks PR (moby/moby#32981) says it added the The only comment I have on the code itself is that there are many places that specify |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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?
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>
Thanks all for the review. The PR has been updated and now the unnecessary For moby/moby#32981, it adds allows a node-local network to be promoted to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM |
This fix update docker/cli to 4c224a7 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>
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