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

Change 'hasImage()' to use inspect instead of list #230

Closed
rhuss opened this issue Jul 23, 2015 · 4 comments
Closed

Change 'hasImage()' to use inspect instead of list #230

rhuss opened this issue Jul 23, 2015 · 4 comments
Labels
Milestone

Comments

@rhuss
Copy link
Collaborator

rhuss commented Jul 23, 2015

The problem is that GET /images/json?filter= is not documented and probably not officially supported. Also it has a bug: When you filter with a name containing one of the default registries index.docker.io and docker.io you won't match images without a registry prefix, make this call quite asymmetrically.

(the current solution is to try every known public registry name)

AFAIS a GET /images/(name)/json doesn't suffer this problem. For hasImage() one could simply check for the status code 202 and/or 404.

@jgangemi what do you think ?

@rhuss rhuss added the fixed label Jul 23, 2015
@jgangemi
Copy link
Collaborator

lol - i just saw this. looks like filter is officially supported but nothing wrong w/ this change.

@rhuss
Copy link
Collaborator Author

rhuss commented Jul 23, 2015

Yeah, but filter is flawed /wr to the default registry part --> moby/moby#14894

Seems that they not really picked that up, also it could be that filter is moved into filters. Also, inspect seems to be more naturally than list for detecting the existence of an image. I already changed it and all the issues I had with this default registry naming vanished. So I'm happy ;-)

However, I removed some stuff (as always ;-) and made UrlBuilder consistence (it used to use a mixture of DockerUrl and String and I went back to String since I didn't see much value-add to this extra object.

@rhuss
Copy link
Collaborator Author

rhuss commented Jul 23, 2015

Sorry for being such a jackass, but hopefully you find it also a bit more straight forward. I don't mind to introduce new abstractions like argument or model objects, but only if the complexity is high enough. I still have the feeling that most of the time an image id (string) is enough, so I removed Image again. Also I think ListArgs and BuildArgs are a bit overkill when you only have two query parameters, that can be easily and more typesafely mapped to method arguments directly.

@jgangemi
Copy link
Collaborator

no worries, your project :)

the DockerUrl probably was overkill, esp once i started handing the args in the UrlBuilder itself.

the ListArgs and BuildArgs as an idea i stole from the spotify client. i figured now that DockerAccess was a bit more generic, it would be an easier way to pass in args instead of overloading methods in UrlBuilder. i guess if we find ourselves doing that down the road, we can revisit.

@rhuss rhuss added this to the 0.13.3 milestone Jul 24, 2015
@rhuss rhuss closed this as completed in e5ce47a Aug 13, 2015
leusonmario pushed a commit to leusonmario/docker-maven-plugin that referenced this issue Aug 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants