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.

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

feat(import): copy folder contents #453

Merged
merged 2 commits into from
Nov 6, 2023
Merged

feat(import): copy folder contents #453

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
faa7772
feat(import): copy dir contents
rchincha Apr 3, 2023
91ed8ea
feat(import): allow dest dir renaming during import
rchincha Oct 2, 2023
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
2 changes: 1 addition & 1 deletion cmd/stacker/grab.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,5 +43,5 @@ func doGrab(ctx *cli.Context) error {
return err
}

return stacker.Grab(config, s, name, parts[1], cwd, nil, -1, -1)
return stacker.Grab(config, s, name, parts[1], cwd, "", nil, -1, -1)
}
15 changes: 15 additions & 0 deletions pkg/overlay/overlay-dirs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@
break
}

log.Debugf("overlayDirs: %+v", overlayDirs)

Check warning on line 112 in pkg/overlay/overlay-dirs.go

View check run for this annotation

Codecov / codecov/patch

pkg/overlay/overlay-dirs.go#L111-L112 

Added lines #L111 - L112 were not covered by tests
for ovlindex, ovldir := range overlayDirs {
if ovldir.Dest == "" {
continue
Expand All @@ -124,6 +126,15 @@
return errors.Wrapf(err, "unable to stat %s", contents)
}

contents, err = filepath.EvalSymlinks(contents)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
continue

Check warning on line 132 in pkg/overlay/overlay-dirs.go

View check run for this annotation

Codecov / codecov/patch

pkg/overlay/overlay-dirs.go#L129-L132 

Added lines #L129 - L132 were not covered by tests
}

return errors.Wrapf(err, "unable to eval symlink %s", contents)

Check warning on line 135 in pkg/overlay/overlay-dirs.go

View check run for this annotation

Codecov / codecov/patch

pkg/overlay/overlay-dirs.go#L135 

Added line #L135 was not covered by tests
}

dest := path.Join(contents, ovldir.Dest)
realdest, err := filepath.EvalSymlinks(dest)
if err != nil {
Expand All @@ -135,6 +146,10 @@
}

overlayDirs[ovlindex].Dest = strings.TrimPrefix(realdest, contents)
if overlayDirs[ovlindex].Dest == "" {
overlayDirs[ovlindex].Dest = ovldir.Dest
}

Check warning on line 151 in pkg/overlay/overlay-dirs.go

View check run for this annotation

Codecov / codecov/patch

pkg/overlay/overlay-dirs.go#L149-L151 

Added lines #L149 - L151 were not covered by tests

if ovldir.Dest != overlayDirs[ovlindex].Dest {
log.Infof("overlay dest %s is a symlink, patching to %s", ovldir.Dest, overlayDirs[ovlindex].Dest)
break
Expand Down
9 changes: 7 additions & 2 deletions pkg/stacker/grab.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
)

func Grab(sc types.StackerConfig, storage types.Storage, name string, source string, targetDir string,
mode *fs.FileMode, uid, gid int,
idest string, mode *fs.FileMode, uid, gid int,
) error {
c, err := container.New(sc, name)
if err != nil {
Expand All @@ -31,7 +31,12 @@
}

bcmd := []string{insideStaticStacker, "internal-go"}
err = c.Execute(append(bcmd, "cp", source, "/stacker/"+path.Base(source)), nil)

if idest == "" || source[len(source)-1:] != "/" {
err = c.Execute(append(bcmd, "cp", source, "/stacker/"+path.Base(source)), nil)
} else {
err = c.Execute(append(bcmd, "cp", source, "/stacker/"), nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like you're not using 'idest' here at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

line #35
if idest == ""

}

Check warning on line 39 in pkg/stacker/grab.go

View check run for this annotation

Codecov / codecov/patch

pkg/stacker/grab.go#L34-L39 

Added lines #L34 - L39 were not covered by tests
if err != nil {
return err
}
Expand Down
31 changes: 27 additions & 4 deletions pkg/stacker/import.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,17 @@
return dest, nil
}

dest := path.Join(cacheDir, path.Base(imp))
var dest string
if imp[len(imp)-1:] != "/" {
if idest != "" && path.Base(imp) != path.Base(idest) {
dest = path.Join(cacheDir, path.Base(idest))
} else {
dest = path.Join(cacheDir, path.Base(imp))
}
} else {
dest = cacheDir
}

Check warning on line 136 in pkg/stacker/import.go

View check run for this annotation

Codecov / codecov/patch

pkg/stacker/import.go#L127-L136 

Added lines #L127 - L136 were not covered by tests

if err := os.MkdirAll(dest, 0755); err != nil {
return "", errors.Wrapf(err, "failed making cache dir")
}
Expand Down Expand Up @@ -156,7 +166,20 @@
fallthrough
case mtree.Extra:
srcpath := path.Join(imp, d.Path())
destpath := path.Join(cacheDir, path.Base(imp), d.Path())
var destpath string
if imp[len(imp)-1:] != "/" {
if idest != "" && path.Base(imp) != path.Base(idest) {
if idest[len(idest)-1:] != "/" {
destpath = path.Join(cacheDir, path.Base(idest), d.Path())
} else {
destpath = path.Join(cacheDir, path.Base(imp), d.Path())
}
} else {
destpath = path.Join(cacheDir, path.Base(imp), d.Path())
}
} else {
destpath = path.Join(cacheDir, d.Path())
}

Check warning on line 182 in pkg/stacker/import.go

View check run for this annotation

Codecov / codecov/patch

pkg/stacker/import.go#L169-L182 

Added lines #L169 - L182 were not covered by tests

if d.New().IsDir() {
fi, err := os.Lstat(destpath)
Expand Down Expand Up @@ -260,7 +283,7 @@
return "", err
}
defer cleanup()
err = Grab(c, storage, snap, url.Path, cache, mode, uid, gid)
err = Grab(c, storage, snap, url.Path, cache, idest, mode, uid, gid)

Check warning on line 286 in pkg/stacker/import.go

View check run for this annotation

Codecov / codecov/patch

pkg/stacker/import.go#L286 

Added line #L286 was not covered by tests
if err != nil {
return "", err
}
Expand Down Expand Up @@ -349,7 +372,7 @@

dest := i.Dest

if i.Dest[len(i.Dest)-1:] != "/" {
if i.Dest[len(i.Dest)-1:] != "/" && i.Path[len(i.Path)-1:] != "/" {

Check warning on line 375 in pkg/stacker/import.go

View check run for this annotation

Codecov / codecov/patch

pkg/stacker/import.go#L375 

Added line #L375 was not covered by tests
dest = path.Dir(i.Dest)
}

Expand Down
3 changes: 3 additions & 0 deletions pkg/types/layer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,9 @@
if err != nil {
return ret, err
}
if rawImport.Path[len(rawImport.Path)-1:] == "/" {
absImportPath += "/"
}

Check warning on line 394 in pkg/types/layer.go

View check run for this annotation

Codecov / codecov/patch

pkg/types/layer.go#L392-L394 

Added lines #L392 - L394 were not covered by tests
absImport := Import{Hash: rawImport.Hash, Path: absImportPath, Dest: rawImport.Dest, Mode: rawImport.Mode, Uid: rawImport.Uid, Gid: rawImport.Gid}
ret.Imports = append(ret.Imports, absImport)
}
Expand Down
124 changes: 124 additions & 0 deletions test/import.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,130 @@ eigth:
[ -f /dir/file2 ]
[ -f /dir/files/file3 ]
EOF
stacker build
}

@test "import with dir contents" {
mkdir folder1
touch folder1/file1
mkdir folder1/subfolder2
touch folder1/subfolder2/subfile1
cat > stacker.yaml <<EOF
first:
from:
type: oci
url: $CENTOS_OCI
import:
- path: folder1/
dest: /folder1/
run: |
[ -f /folder1/file1 ]

hallyn marked this conversation as resolved.
Show resolved Hide resolved
second:
from:
type: built
tag: first
run: |
mkdir -p /folder1
touch /folder1/file1
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do?
'second' comes from 'first', which has /folder1, so why create it (with mkdir) and then touch '/folder1/file1' which already exists. is that supposed to force a copy-up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup.

touch /folder1/file2

third:
from:
type: oci
url: $CENTOS_OCI
import:
- path: stacker://second/folder1/
dest: /folder1/
run: |
[ -f /folder1/file1 ]
[ -f /folder1/file2 ]

fourth:
from:
type: oci
url: $CENTOS_OCI
import:
- path: folder1/
andaaron marked this conversation as resolved.
Show resolved Hide resolved
dest: /
run: |
ls -l /
[ -f /file1 ]
mkdir -p /folder4/subfolder5
touch /folder4/subfolder5/subfile6

fifth:
from:
type: oci
url: $CENTOS_OCI
import:
- path: folder1/subfolder2/
dest: /folder3/
- path: folder1/subfolder2
dest: /folder4
- path: stacker://fourth/folder4/subfolder5/
dest: /folder6/
- path: stacker://fourth/folder4/
dest: /folder7/
Copy link
Contributor

Choose a reason for hiding this comment

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

You are not testing an import of a 'stacker://' path without a trailing /.

You should add an import of:

 - path: stacker://fourth/folder4
   dest: /folder8

And then a test of

  [ -d /folder8/folder4 ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is if folder8 is present then [ -d /folder8/folder4 ] will evaluate to true else false. But let me add this test and see what happens.

Copy link
Contributor

@smoser smoser Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

oh, yuck.

i think that 'target' (the full path to the imported thing) should be fully determined by the 'dest', and not dependent on either the 'path' or the contents of the filesystem to which it is being imported. At worst, target should be determined by 'path' and 'dest', rather than 'path', 'dest' and 'target-filesystem'.

that just seems the least surprising behavior to me.

Copy link
Contributor Author

@rchincha rchincha Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The added test failed. The trailing slash determines what the behavior is - a trailing slash means contents, else the folder itself, applies for path and dest independently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

a trailing slash means contents, else the folder itself,

that part makes sense.

applies for path and dest independently

that I don't understand.

run: |
ls -l /folder*
[ -f /folder3/subfile1 ]
[ ! -e /folder3/subfolder2 ]
[ -f /folder4/subfile1 ]
[ -f /folder6/subfile6 ]
[ ! -e /folder6/subfolder5 ]
[ -f /folder7/subfolder5/subfile6 ]
EOF
stacker build
}
andaaron marked this conversation as resolved.
Show resolved Hide resolved

@test "dir path behavior" {
mkdir -p folder1
touch folder1/file1
mkdir -p folder1/subfolder2
touch folder1/subfolder2/subfile1
cat > stacker.yaml <<EOF
src_folder_dest_non_existent_folder_case1:
from:
type: docker
url: docker://ubuntu:latest
import:
- path: folder1
dest: /folder2
run: |
[ -f /folder2/file1 ]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unfortunate. Using rsync semantics:

serge@jerom ~/delme$ mkdir folder1
serge@jerom ~/delme$ touch folder1/file1
serge@jerom ~/delme$ rsync -va folder1 folder2
sending incremental file list
created directory folder2
folder1/
folder1/file1

sent 138 bytes  received 69 bytes  414.00 bytes/sec
total size is 0  speedup is 0.00
serge@jerom ~/delme$ ls folder2
folder1

So let's make sure to have a very clear documentation for these semantics. Right now this testcase appears to be the clearest documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind you I think I like the behavior you have here better. I'm surprised by what rsync is doing.

So I'm not asking you to change it. Just document it :)


src_folder_dest_non_existent_folder_case2:
from:
type: docker
url: docker://ubuntu:latest
import:
- path: folder1/
dest: /folder2
run: |
[ -f /folder2/file1 ]

src_folder_dest_non_existent_folder_case3:
from:
type: docker
url: docker://ubuntu:latest
import:
- path: folder1
dest: /folder2/
run: |
ls -al /
ls -al /folder2
[ -f /folder2/folder1/file1 ]

src_folder_dest_non_existent_folder_case4:
from:
type: docker
url: docker://ubuntu:latest
import:
- path: folder1/
dest: /folder2/
run: |
[ -f /folder2/file1 ]
EOF
stacker build
}
Loading