-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fetch network details with network lists greedily #1812
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
Conversation
Signed-off-by: Viktor Adam <rycus86@gmail.com>
Signed-off-by: Viktor Adam <rycus86@gmail.com>
15be737
to
2878900
Compare
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.
Thanks, that's definitely a useful feature to have.
My only issue with it as it stands is that I don't think the greedy
flag belongs in the APIClient
. Advanced, usability-minded functionality belongs in DockerClient
instead. APIClient
should be as lean and resource-efficient as possible.
Note that using API 1.25 will still return the container details; see https://github.com/moby/moby/pulls/30673 The list of containers was removed from the network list because it can be a heavy operation in larger setups (many containers, many networks), (inspecting a single network still returns attached containers) |
Signed-off-by: Viktor Adam <rycus86@gmail.com>
a17dfdb
to
36ed843
Compare
Thanks for having a look at this! I've moved the new flag to the model only and left the API client unchanged. I only added it there because of #1775 (comment) but it makes sense to leave the API client as it was. @thaJeztah thanks for letting me know! |
Hi @shin- , Sorry I haven't done reviews on GitHub yet, is there a link/button somewhere to update that I've made the changes that were requested? Thank you! |
@rycus86 I saw your changes, just didn't have time to take a closer look yet with Thanksgiving break and other priorities. Thank you for your patience. 👍 |
Right, sorry, forgot about Thanksgiving! :) |
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, thank you!
This change would fix #1775 plus adds some missing documentation and input arguments around networks.
Per the comment on #1775 (comment) this change enables adding a
greedy=True
parameter on the model's and the API client's network list function that will inspect each network on the initial list.