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

Add dry-run mode to copy skopeo-copy #2178

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

Conversation

deriamis
Copy link

@deriamis deriamis commented Dec 8, 2023

Dry-run functionality was merged into the sync command in #1608. Here, the same sort of functionality is added to the copy command as well.

Also, containerized targets weren't working with docker due to a few missing items. I fixed them in a second commit so I could verify all the make targets passed. If you would prefer that commit in a second PR, I can do so.

@deriamis deriamis changed the title Add dry-run imode to copy skopeo-copy Add dry-run mode to copy skopeo-copy Dec 8, 2023
Dry-run functionality was merged into the sync command in containers#1608.
Here, the same sort of functionality is added to the copy command
as well.

Signed-off-by: Ryan Egesdahl <[email protected]>
The containerized targets weren't working everywhere due to a few
missing items. These are corrected so containerized tests and
builds will successfully.

Signed-off-by: Ryan Egesdahl <[email protected]>
Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

Why does this need to be an option? A single skopeo copy always copies one image, so what good does this do? With skopeo sync, it might be interesting to see which images are going to be copied, because that is dynamic and changing.

In here, just substituting skopeo copy with echo skopeo copy in the callers would, AFAICS, deliver broadly the same kind of information; so why have the new option?


I didn’t actually review the implementation in detail yet, but the git config --global --add calls are a clear blocker.

@@ -127,7 +130,8 @@ help:

# Do the build and the output (skopeo) should appear in current dir
binary: cmd/skopeo
$(CONTAINER_RUN) make bin/skopeo $(if $(DEBUG),DEBUG=$(DEBUG)) BUILDTAGS='$(BUILDTAGS)'
$(CONTAINER_RUN) bash -c \
"git config --global --add safe.directory $(CONTAINER_GOSRC) && make bin/skopeo $(if $(DEBUG),DEBUG=$(DEBUG)) BUILDTAGS='$(BUILDTAGS)'"
Copy link
Contributor

Choose a reason for hiding this comment

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

I consider a single project overwriting users’ configuration like this completely unexpected to the point of being hostile.

Copy link
Author

Choose a reason for hiding this comment

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

It looks a lot worse than it is. This only happens inside a container that is built from an image the user doesn't control, so it can't overwrite their configuration. It's really only modifying root's git config in the container's ephemeral root volume. Without the change, the git command to grab the current commit hash fails and therefore the go build command also fails.

Incidentally, this seems to be a problem with an ownership mismatch due to user ID mapping issues when the container is started. Either the image should be fixed or user ID mapping should be implemented. This is only a stopgap measure so the tests actually run.

@deriamis
Copy link
Author

Why does this need to be an option? A single skopeo copy always copies one image, so what good does this do? With skopeo sync, it might be interesting to see which images are going to be copied, because that is dynamic and changing.

In here, just substituting skopeo copy with echo skopeo copy in the callers would, AFAICS, deliver broadly the same kind of information; so why have the new option?

It's true that copy.Image from containers/image/copy doesn't know how to do a dry-run - as in, ensuring the source image and destination repository both exist but not doing the copy. That would be nice for CI/CD operations when testing publish functionality, like we do here in this MR:

https://gitlab.com/gitlab-org/cloud-native/charts/gitlab-ingress-nginx/-/merge_requests/13/diffs#deda9b12c56fbec4fa6d58b177145eba56e84db3_0_19

Even so, skopeo-copy does a lot of argument checks and also some image name parsing before it writes the dry-run output, so seeing the output at least tells you the command is structured correctly, which is more than echo can do.

I didn’t actually review the implementation in detail yet, but the git config --global --add calls are a clear blocker.

See my reply to your comment about the command. I can move the test infrastructure changes to a separate PR if you still have questions and are otherwise satisfied with what I am doing. I only needed them so I could run the tests locally on my Mac.

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.

2 participants