-
Notifications
You must be signed in to change notification settings - Fork 642
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
docker container fetch limit is now configurable #513
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,7 +53,7 @@ public String containerLogs(String containerId, boolean follow) { | |
.p("follow", follow) | ||
.build(); | ||
} | ||
|
||
public String createContainer(String name) { | ||
return u("containers/create") | ||
.p("name", name) | ||
|
@@ -70,13 +70,19 @@ public String inspectContainer(String containerId) { | |
return u("containers/%s/json", containerId) | ||
.build(); | ||
} | ||
|
||
public String listContainers(int limit) { | ||
return u("containers/json") | ||
.p("limit", limit) | ||
.build(); | ||
} | ||
|
||
public String listAllContainers() { | ||
return u("containers/json") | ||
.p("all", 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldnt this be "1" ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep -- misread the documentation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no worries, I already did some refactorings, will push my changes in an own branch soon. |
||
.build(); | ||
} | ||
|
||
public String pullImage(ImageName name, String registry) { | ||
return u("images/create") | ||
.p("fromImage", name.getNameWithoutTag(registry)) | ||
|
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.
It doesnt makes sense to pick up only the first image for extracting the fetchLimit. Currently it only makes sense as a globale parameter (as you already put it in). Since I guess it makes also only sense to put it into the global scope (i.e. valid for all images), I'll move it from ServiceHub to DockerAccess and remove the two variants of listImages() (since the list will automatically truncate).
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.
Well spotted. I meant to ask you about fetching the resolvedImage. I was not happy with the get(0) it's a bit dangerous ..
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.
Yeah, that was a misunderstanding on my side. A global parameter is good enough.
However, our problem indeed is how we can make the lookup of a container by its name and by its image more effective. Because then we could omit the call to list() altogether. And it would be much more effective, too, especially for a large set of running containers. At the time this was implemented, there was no way with the API todo this direct lookup, but it could be that there is now an alternative API call.
I will go hunting a bit ... ;-)
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.
Just saw that there are new filters for the list call:
So maybe
since
andancestors
could be a match for our use case ....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.
E.g.
Hopefully the filter is inclusive. 'will try this out now ...
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.
gah -- we are on 1.11 -- I will upgrade Nucleus's docker tomorrow to 1.24..
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.
I'm also only on 1.21, however there is
since
query param (not as part of the filter), but it seems to eclusive, not inclusive (so the entry with the name itself is not included).For the name lookup I have to check why I haven't use the
/containers/<name>/json
call. I remember darkly, there was a reason ;-)'have to run now, see you tomorrow ...
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.
Cool, filtering with 'ancestor=' works. Unfortunately only available since 1.23
But with this two features we should get rid of the local iteration of image names (we leave it in nevertheless so that the plugin will work with older clients, too).