Skip to content
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

introduce /images/json?containers=true parameter #43350

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Mar 8, 2022

** Make sure all your commits include a signature generated with git commit -s **

close #43244

- What I did
Restored the ability to list number of containers using an image with /images/json API

- How I did it
introduced new parameter containers to trigger (pre-existing) counting logic

- How to verify it
curl http://localhost:2375/v1.41/images/json?containers=true | jq

- Description for the changelog
GET /images/json now accepts query parameters containers to count containers using each image

@thaJeztah
Copy link
Member

I think the reason that field was not filled was because this field was added specifically for the /system/df endpoint. When that was added, the same struct was re-used (yeah..), but the field left empty (or set to -1). b308097 (#42550) added some extra comment elsewhere in the code, but may need more details there.

We should look how useful it is to add this information to docker image ls (which uses this endpoint), and check if there's no possible side-effects of adding it here (although I think the container count comes from the memory-store, so probably won't cause much overhead (famous last words)).

The main issue in , however, looks to be due to the ImageSummary being completely un-documented in the swagger file,

type ImageSummary struct {
, so it gives no clue whatsoever what each fields is (and when it's used); that's definitely something we should fix (probably we can copy some docs that were added in #43300)

@thaJeztah thaJeztah changed the title introduce /images/json?conainers=true parameter introduce /images/json?containers=true parameter Mar 8, 2022
@thaJeztah
Copy link
Member

oh! there's a typo in your commit message (conainers - missing a t)

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof
Copy link
Contributor Author

ndeloof commented May 11, 2022

I think the container count comes from the memory-store, so probably won't cause much overhead

ImageService comptes this value by filtering on containers.List(), and the container store it is configured with is container.NewMemoryStore(), so this seems to be fine

@ndeloof ndeloof force-pushed the imagesJson branch 3 times, most recently from 92f389b to 8d92f7f Compare May 11, 2022 15:19
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in the deamon's response, containers count is always -1
2 participants