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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74f344a
test(integration): TestDocker2DirDryRun
frq-asgard-josi Jul 18, 2025
efbac99
fix(sync): fix dry-run failing consecutive runs by creating directori…
frq-asgard-josi Jul 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions cmd/skopeo/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func parseRepositoryReference(input string) (reference.Named, error) {
// destinationReference creates an image reference using the provided transport.
// It returns a image reference to be used as destination of an image copy and
// any error encountered.
func destinationReference(destination string, transport string) (types.ImageReference, error) {
func destinationReference(destination string, transport string, dryRun bool) (types.ImageReference, error) {
var imageTransport types.ImageTransport

switch transport {
Expand All @@ -181,9 +181,11 @@ func destinationReference(destination string, transport string) (types.ImageRefe
if !os.IsNotExist(err) {
return nil, fmt.Errorf("Destination directory could not be used: %w", err)
}
// the directory holding the image must be created here
if err = os.MkdirAll(destination, 0755); err != nil {
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.

return nil, fmt.Errorf("Error creating directory for image %s: %w", destination, err)
}
}
imageTransport = directory.Transport
default:
Expand Down Expand Up @@ -697,7 +699,7 @@ func (opts *syncOptions) run(args []string, stdout io.Writer) (retErr error) {
destSuffix = path.Base(destSuffix)
}

destRef, err := destinationReference(path.Join(destination, destSuffix)+opts.appendSuffix, opts.destination)
destRef, err := destinationReference(path.Join(destination, destSuffix)+opts.appendSuffix, opts.destination, opts.dryRun)
if err != nil {
return err
}
Expand Down
25 changes: 25 additions & 0 deletions integration/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,31 @@ func (s *syncSuite) TestDocker2DirUntagged() {
assert.Len(t, nManifests, len(tags))
}

func (s *syncSuite) TestDocker2DirDryRun() {
t := s.T()
tmpDir := t.TempDir()

// FIXME: It would be nice to use one of the local Docker registries instead of needing an Internet connection.
image := pullableRepo
imageRef, err := docker.ParseReference(fmt.Sprintf("//%s", image))
require.NoError(t, err)

require.NoError(t, err)
assertSkopeoSucceeds(t, "", "sync", "--scoped", "--dry-run", "--src", "docker", "--dest", "dir", image, tmpDir)

sysCtx := types.SystemContext{}
tags, err := docker.GetRepositoryTags(context.Background(), &sysCtx, imageRef)
require.NoError(t, err)
assert.NotZero(t, len(tags))

d, err := os.Open(tmpDir)
require.NoError(t, err)
defer d.Close()
nDirEntries, err := d.ReadDir(1)
require.NoError(t, err)
assert.Equal(t, 0, len(nDirEntries))
}

func (s *syncSuite) TestYamlUntagged() {
t := s.T()
tmpDir := t.TempDir()
Expand Down