-
Notifications
You must be signed in to change notification settings - Fork 244
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
fix: use distribution to parse references #275
fix: use distribution to parse references #275
Conversation
fixes #273
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.
Hey @iamnoah, thanks a lot for this fix!
LGTM so far, with one fixable and one concern:
- Please fix the go import to satisfy the linter:)
- A question/request for change, please see below
img := ContainerImage{} | ||
img.RegistryURL = reference.Domain(parsed) | ||
// remove default registry for backwards-compatibility | ||
if img.RegistryURL == "docker.io" && !strings.HasPrefix(imgRef, "docker.io") { |
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.
@jannfis this was my workaround. If docker.io was explicity the registry, retain it, else remove it. Is there a case I’m missing?
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.
Ah, now I got it. Sorry, I was a little confused and missed the check for the prefix on original image ref.
OK, so if the parsed registry domain is docker.io
, but the original image ref is not prefixed with docker.io
, we assume default registry and set it to blank. Yes, that makes sense and would restore original behavior. Thanks for clarifying it.
OK, so I think the last remaining issue is that This is currently also a per-registry configurable in the image updater (see https://argocd-image-updater.readthedocs.io/en/stable/configuration/registries/#configuring-a-custom-container-registry, |
Done. |
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. Thanks @iamnoah !
fixes #273