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

[Feature] make SplitDockerDomain public #1522

Closed
wants to merge 2 commits into from

Conversation

tanguofu
Copy link

SplitDockerDomain is very useful function to get the docker images name with out domain, so this PR wish to make it public.

SplitDockerDomain is very useful function  to get the docker images name with out domain,  so this PR wish to make it public.

Signed-off-by: guofutan <guofutan@tencent.com>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

@mtrmac PTAL

Are you interested in the name or everything but the domain? E.g., repo/name:tag vs name.

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 20, 2022

There already is reference.Domain and reference.Path; in typical uses, it’s more efficient than re-computing SplitDockerDomain. (Also, I would strongly recommend using reference.* types everywhere, not raw strings, to decrease the risk of adding ad-hoc parsers that don’t process the data exactly consistently, or adding extra ad-hoc syntax.)

Is there anything unusual in your use case that would make the two undesirable?

@tanguofu
Copy link
Author

the repo/name:tag is necessary when sync docker image between docker registry, we need keep the same path and
the same tag.
but there is no helper function to get repo/name:tag , the reference.Path did not has tag.

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 20, 2022

the repo/name:tag is necessary when sync docker image between docker registry, we need keep the same path and
the same tag.

That seems to be hard-coding too much to me (compare containers/skopeo#854 (comment) ); and in that case the existing behavior of checking for, and explicitly adding, docker.io[/library] seems actively unwanted — so SplitDockerDomain might not be exactly right API anyway.

The balancing concerns are

  1. Discouraging string editing. Arguably making SplitDockerDomain available, could cut either way — it’s not good that the input is a string, but if it is called instead of a hand-crafted parser copy&pasted into the potential caller, it’s better to provide an API.
  2. Driving consistent behavior. For that, having callers specifically choose, or not, the docker.io/library semantics is not great.
  3. Keeping the code consistent with GitHub.com/distribution/distribution. So changing the existing implementation is, by default, not very attractive.

A possible more general alternative: remapping namespace prefixes (that includes registry → registry, repo → repo, registry → namespace …). We already have several hand-crafted implementations like sysregistriesv2.Endpoint.rewriteReference, signature.prmRemapIdentity.remapReferencePrefix; maybe that’s something that should actually live in docker/reference.

@TomSweeneyRedHat TomSweeneyRedHat changed the title fea: make SplitDockerDomain public {Feature] make SplitDockerDomain public Apr 20, 2022
@TomSweeneyRedHat TomSweeneyRedHat changed the title {Feature] make SplitDockerDomain public [Feature] make SplitDockerDomain public Apr 20, 2022
@tanguofu
Copy link
Author

sorry i did not get your point @mtrmac , so what is the better way to get the repo/name:tag of docker image do you perfer?

can you give me some instruction? thanks.

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 21, 2022

What is the higher-level problem this is trying to solve?

If this is for the skopeo sync OCI work, note containers/skopeo#1620 (comment) : immediately we just can’t do this.


In my view, code handling Docker-like references should:

  • as soon as possible, convert user input (if at all possible, via ParseNormalizedNamed, though admittedly there are exceptions) into a reference.Named
  • only pass that reference.Named around call stacks and across codebases
  • only turn reference.Named into strings down in c/image/docker or a similar low-level consumer.

That would imply that ideally any remapping/redirecting/combination should have both a reference.Named input and output. Now, skopeo sync right now is very much not like that — but if we are going to add a new API in c/image, I think it should be working a direction towards reference.Named use, not the opposite. (And for skopeo sync, as long as it uses path.Join()/path.Base() to map data between paths and registry references, it can continue to do something like that and work with "/" directly.)

@tanguofu
Copy link
Author

yes now this feature is need by implement skopeo sync, ok i can try how reimplement that with the domain name sync to oci repo.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Dec 7, 2022
@mtrmac
Copy link
Collaborator

mtrmac commented Dec 8, 2022

Assuming the OCI support in skopeo sync will not need this, I’m closing this for now. Please reopen if it turns out that the OCI support does need something like this.

@mtrmac mtrmac closed this Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants