Skip to content

fix(sync): fix dry-run failing consecutive runs by creating directories in target dir #2660

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.

Sign up for GitHub

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

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

fix(sync): fix dry-run failing consecutive runs by creating directories in target dir #2660

wants to merge 2 commits into from

Conversation

@frq-asgard-josi
Copy link

@frq-asgard-josi frq-asgard-josi commented Jul 18, 2025 •
edited
Loading

skopeo sync --dry-run --dest dir currently fails consecutive executions since the func destinationReference attempts to create directories even when running in dry-run mode and fails if these already exist.

@packit-as-a-service
Copy link

packit-as-a-service bot commented Jul 18, 2025

Ephemeral COPR build failed. @containers/packit-build please check.

@packit-as-a-service
Copy link

packit-as-a-service bot commented Jul 18, 2025

Tests failed. @containers/packit-build please check.

mtrmac
mtrmac reviewed Jul 18, 2025
View reviewed changes
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!

attempts to create directories even when running in dry-run mode

That’s clearly undesirable.

and fails if these already exist.

I can’t see how? MkdirAll is not supposed to fail in that situation. What is failing exactly?

cmd/skopeo/sync.go
return nil, fmt.Errorf("Error creating directory for image %s: %w", destination, err)
if !dryRun {
// the directory holding the image must be created here
if err = os.MkdirAll(destination, 0755); err != nil {
Copy link
Contributor

@mtrmac mtrmac Jul 18, 2025

Choose a reason for hiding this comment

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

I’m afraid I don’t think this works, in general: below, directory.Transport.ParseReference requires the parent directory to exist. We’d need to defer the parsing into a typed ImageReference, and deal with untyped strings in the meantime :/

Copy link
Author

@frq-asgard-josi frq-asgard-josi Jul 21, 2025

Choose a reason for hiding this comment

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

My personal preference would be to expect the parent directory to already exist since this would avoid creating unwanted directories due to mistyping (not that I ever did this 😆) but we should check that the directory exists here and provide a proper error message instead of relying on ParseReference to catch this.

Copy link
Contributor

@mtrmac mtrmac Jul 21, 2025

Choose a reason for hiding this comment

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

The parent directory is not always user-specified: with --scoped, trailing paths of the path come from the source repository name.

@frq-asgard-josi
Copy link
Author

frq-asgard-josi commented Jul 21, 2025

Thanks!

attempts to create directories even when running in dry-run mode

That’s clearly undesirable.

and fails if these already exist.

I can’t see how? MkdirAll is not supposed to fail in that situation. What is failing exactly?

Since the destination exists, os.Stat(destination) doesn't return any err and we return Refusing to overwrite destination directory (Line 179)

@mtrmac mtrmac added the kind/bug A defect in an existing functionality (or a PR fixing it) label Jul 28, 2025
@github-actions
Copy link

github-actions bot commented Aug 28, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mtrmac mtrmac mtrmac left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

kind/bug A defect in an existing functionality (or a PR fixing it) stale-pr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@frq-asgard-josi @mtrmac