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

WIP added implementation for scoped-without-registry support on sync #1100

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

c0ffee
Copy link

@c0ffee c0ffee commented Nov 9, 2020

Adds support to sync 2 registries with all namespaces but without adding the registry as prefix

Fixes #1072 based on the comments of #854

Name of the commandline argument can be discussed

Help is appreciated especially for writing meaningful tests!

✔️ Code
❌ Tests
❌ Documentation

@rhatdan
Copy link
Member

rhatdan commented Nov 17, 2020

@c0ffee Needs a rebase.
@mtrmac @vrothberg PTAL

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.

Thanks! Did you try @mtrmac's suggestion in #854 (comment)? Not sure a new flag is needed. Can you elaborate why you prefer a flag over #854 (comment)?

@@ -548,6 +550,13 @@ func (opts *syncOptions) run(args []string, stdout io.Writer) error {
case docker.Transport:
// docker -> dir or docker -> docker
destSuffix = ref.DockerReference().String()
if opts.scopedWithoutRegistry {
removeRegistry := func(s string) string {
Copy link
Member

Choose a reason for hiding this comment

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

No need for the anonymous function.

@c0ffee c0ffee marked this pull request as draft November 21, 2020 10:55
@mtrmac
Copy link
Contributor

mtrmac commented Nov 24, 2020

Yeah, I think it’s been a mistake to hard-code any naming policy in sync as a default behavior, and adding more and more options of that kind won’t make the basic mistake go away.

AFAICS to this day you can’t copy $registry1/ns1/repo to $registry2/ns2/some-other-repo-name if I’m not mistaken.

  • on the CLI, a fix for that would automatically give users an easy-to-understand way to build any other repo hierarchy as well, wouldn’t it?
  • The YAML format is not trivially extensible to allow this (there is no way to put the “destination” in a $registry.images-by-tag for example… it still should happen, or maybe deprecate the YAML format and just let users write a shell script, I’m still not sure what makes the YAML format necessary.

@c0ffee
Copy link
Author

c0ffee commented Nov 26, 2020

we use this basically to synchronize some of our on-prem host registries to a merged remote one and some images use several namespaces which makes it uncomfortable

from my perspective the behavior of scope is not what you would expect and this is why all these issues exists, so the best way would be to fix this, but this is not possible right?

i actually like the yaml format, one nice way to use it is to check it in a repo and use it as a kind of promoting way which images should be available on the external mirror(controlled access on the repo and CI chain is using it as input)

before i put now more work into this? what would be the preferred way to continue?

@mtrmac
Copy link
Contributor

mtrmac commented Nov 28, 2020

For CLI, an option (sadly it has to be an option) to do no mapping and just let the user specify a destination repo, per the previous comment.

For YAML, I’m not immediately sure. It would be possible to keep the existing format and add a repo map (source repo → destination repo) at the same level as the current images/images-by-tag-regex, but that’s pretty clumsy.

@koweblomke
Copy link

I understand the concerns of not littering the CLI with all kind of options. But in this case couldn't it be said that the current way is the default way and when you want the namespace of the source repository to be respected is actually an option?

So instead of --scoped-without-registry add on option --keep-namespace

I understand this doesn't make the mistake go away, but it makes the tool much more useful.

@github-actions
Copy link

github-actions bot commented Jun 4, 2021

A friendly reminder that this PR had no activity for 30 days.

@PG2000
Copy link

PG2000 commented Nov 6, 2021

@vrothberg Can you explain what the right direction would be here?
For me the use case makes also sense and i would continue to work on it if thats ok for @c0ffee

@vrothberg
Copy link
Member

For CLI, an option (sadly it has to be an option) to do no mapping and just let the user specify a destination repo, per the previous comment.

For YAML, I’m not immediately sure. It would be possible to keep the existing format and add a repo map (source repo → destination repo) at the same level as the current images/images-by-tag-regex, but that’s pretty clumsy.

@PG2000, that looks like a good starting point to me.

@github-actions github-actions bot removed the stale-pr label May 27, 2022
@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Dec 7, 2022
@github-actions github-actions bot removed the stale-pr label Dec 10, 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.

Sync: Add option to keep full name on target when using YAML
6 participants