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

Support more types of image names #119

Merged
merged 12 commits into from
Oct 8, 2020
Merged

Support more types of image names #119

merged 12 commits into from
Oct 8, 2020

Conversation

jgough
Copy link
Contributor

@jgough jgough commented Oct 7, 2020

Now works with many cases of different image names including ports in registry or containers in folders.

Verified with the following image formats:
host:port/dir/image:tag
host:port/image:tag
host:port/image
host/image:tag
image
image:tag

Now works with many cases of different image names including ports in registry or containers in folders.

Verified with the following image formats:
host:port/dir/image:tag
host:port/image:tag
host:port/image
host/image:tag
image
image:tag
@jgough
Copy link
Contributor Author

jgough commented Oct 7, 2020

Was having some major issues as our registry has a port number on it. The tag detection in the original code was confused by this and this pull request fixes this issue.

@taraspos
Copy link
Collaborator

taraspos commented Oct 7, 2020

Cool.
I mentioned some of the other possible issues there #90 (comment). However, it is not completely related to your PR.

I have one request, could you please add some tests with all of the questions you described, to make sure everything works as expected and will keep working in the future :)

host:port/dir/image:tag
host:port/image:tag
host:port/image
host/image:tag
image
image:tag

Now the host must contain at least one dot.  Was failing on images of the form dir/image
@jgough
Copy link
Contributor Author

jgough commented Oct 7, 2020

Hope that suffices. I've not really coded in go before so hopefully I've done that right.

@taraspos
Copy link
Collaborator

taraspos commented Oct 7, 2020

Thanks! Looks good. However, I have a couple of small comments.

Copy link
Collaborator

@taraspos taraspos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgough is just realized that this must be implemented somewhere already and it is possible to reuse it.

So I went to check if there is a method for parsing image in the docker client we are using, and indeed there is one, with tests written on the library side. I would prefer to use it: https://github.com/fsouza/go-dockerclient/blob/main/misc.go#L179-L199

Could you please use it?

core/common_test.go Outdated Show resolved Hide resolved
core/common.go Outdated Show resolved Hide resolved
@taraspos taraspos merged commit 9c278ca into mcuadros:master Oct 8, 2020
@taraspos
Copy link
Collaborator

taraspos commented Oct 8, 2020

Also fixes #90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants