feat(import): copy folder contents #453

rchincha · 2023-04-03T03:54:52Z

What type of PR is this?

Which issue does this PR fix:

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rchincha · 2023-04-03T03:55:58Z

layer1:
  from:
    type: docker
    url: docker://ubuntu:latest
  import:
    - path: folder1/
      dest: /
  run: |
    ls /

^ at least this works with this PR.

Evaluating what changes will be needed.

pkg/stacker/import.go

test/import.bats

andaaron

LGTM

smoser · 2023-04-19T17:38:25Z

test/import.bats

+  - path: stacker://fourth/folder4/subfolder5/
+    dest: /folder6/
+  - path: stacker://fourth/folder4/
+    dest: /folder7/


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 ]

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.

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.

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

Removed the test.

a trailing slash means contents, else the folder itself,

that part makes sense.

applies for path and dest independently

that I don't understand.

smoser

As I understand from reading, a trailing '/' on a path that is a directory means that the contents of the provided path should be placed at 'dest', rather than inside of dest as 'dest/$(basename $path)'.

In #455 I suggested a different set of rules / behavior.

I think we should just not bother with the 'basename' behavior. It makes things much more clear.

The other behavior I think we should iron out is what happens when the 'dest' exists? Does it replace it or "fill" it?

rchincha · 2023-04-28T04:31:35Z

Documenting docker behavior

https://docs.docker.com/engine/reference/builder/#add

Looks like source's file mode is detected at build time from the filesystem and the dest file mode is inferred from trailing slash (and hence copying file to dir is an error, etc)

[Source folder1 is a dir, Destination /folder1 doesn't exist]

COPY folder1 /folder1

creates /folder1 and copies contents on folder1 -> /folder1

COPY folder1/ /folder1

same as 1.

COPY folder1 /folder1/

same as 1.

COPY folder1/ /folder1/

same as 1.

[Source folder1 is a dir, Destination /folder1 already exists]

Same as above

[Source file1, Destination /file1 doesn't exist]

COPY file1 /file1

creates a /file1 and copies file1 -> /file1

COPY file1/ /file1

same as 1, although there is a trailing slash in source. So it is detecting mode type.

COPY file1 /file1/

creates a /file1 folder and copies file1 -> /file1/file1

COPY /file1/ /file1/

Same as 3.

[Source file1, Destination /file1 already exists]

COPY file1 /file1

copies file1 -> file1

COPY file1/ /file1

same as 1.

COPY file1 /file1/

error since it expects file1/ to be a dir but we created it as a file first.

COPY file1/ /file1/

same as 3

smoser · 2023-04-28T18:13:28Z

ADD definitely looks like a bunch of iterative behaviors. complete with behavior like:

If is a local tar archive in a recognized compression format (identity, gzip, bzip2 or xz) then it is unpacked as a directory.

Also, ADD seems to indicate add not "merge", and I really could be missing something in the long description of ADD's behavior, but I don't see what happens if I ADD directory 'foo' to /etc which already has 'foo'. Do the contents get merged somehow? As I can tell, that is "union" and I only see that term used wrt tar.

if you want to use that as your reference of something to implement, and then attempt to keep your implementation bug-for-bug in sync with docker's then I guess its fine. I just think that it is more complex than required.

rchincha · 2023-04-28T18:26:51Z

ADD definitely looks like a bunch of iterative behaviors. complete with behavior like:

If is a local tar archive in a recognized compression format (identity, gzip, bzip2 or xz) then it is unpacked as a directory.

Also, ADD seems to indicate add not "merge", and I really could be missing something in the long description of ADD's behavior, but I don't see what happens if I ADD directory 'foo' to /etc which already has 'foo'. Do the contents get merged somehow? As I can tell, that is "union" and I only see that term used wrt tar.

if you want to use that as your reference of something to implement, and then attempt to keep your implementation bug-for-bug in sync with docker's then I guess its fine. I just think that it is more complex than required.

Not yet sure what stacker does in all these scenarios to compare/contrast yet.

Not one-on-one, but we are constrained by

stacker cannot deviate too much from docker behavior, but it may not have to
stacker imports (and copies) are set up as overlay-dirs, so filesystem grok'ing inside a container actually
Also arguing for predictable behavior/rules just looking at stackerfile itself

rchincha · 2023-04-28T18:32:47Z

but I don't see what happens if I ADD directory 'foo' to /etc which already has 'foo'. Do the contents get merged somehow? As I can tell, that is "union" and I only see that term used wrt tar.

It is a union. If the entry exists in the destination, it gets overwritten.

codecov · 2023-10-10T22:51:38Z

Codecov Report

Merging #453 (91ed8ea) into main (c9428a0) will decrease coverage by 0.27%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #453      +/-   ##
==========================================
- Coverage   13.32%   13.05%   -0.27%     
==========================================
  Files          40       40              
  Lines        5863     5982     +119     
==========================================
  Hits          781      781              
- Misses       4954     5073     +119     
  Partials      128      128

Files	Coverage Δ
pkg/types/layer.go	`50.27% <0.00%> (-0.42%)`	⬇️
pkg/stacker/grab.go	`0.00% <0.00%> (ø)`
pkg/overlay/overlay-dirs.go	`0.00% <0.00%> (ø)`
pkg/stacker/import.go	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

smoser

I would like to see some tests that cover 'overlay/existing' behavior.

build:
  from:
    type: oci
    url: $CENTOS_OCI
  run: |
    mkdir /output/
    echo "my-ls" > /output/ls
    touch /output/whizbang

assemble:
  from:
    type: oci
    url: $CENTOS_OCI
  import:
   - path: stacker://build/output/
     dest: /usr/
  run: |
    # existing file in /usr/bin should still be there
    [ -f /usr/bin/env ]

    # 'ls' should be updated.
    read line < /usr/bin/ls

    # whizbang should be present
    [ -f /usr/bin/whizbang ]

smoser · 2023-10-11T14:32:24Z

pkg/stacker/grab.go

+	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)


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

line #35
if idest == ""

smoser · 2023-10-11T14:33:40Z

test/convert.bats

@@ -48,7 +48,6 @@ EOF
 }

 @test "alpine" {
-  skip_slow_test


was this set of changes intentional?

was this set of changes intentional?

I see that this is part of "test - remove commit", so maybe remove that commit?

Will do. Once I am convinced that stacker convert tests are not broken with these changes.

smoser · 2023-10-11T14:36:19Z

test/import.bats

+    tag: first
+  run: |
+    mkdir -p /folder1
+    touch /folder1/file1


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?

test/import.bats

import: - path: folder1/ dest: / vs import: - path: folder1 dest: / The former will copy the contents of folder1/ at / and the latter will make and copy a /folder1 at / Signed-off-by: Ramkumar Chinchani <[email protected]>

import: - path: folder1 dest: /folder2 will cause contents of folder1 to show up as and under /folder2 Signed-off-by: Ramkumar Chinchani <[email protected]>

hallyn · 2023-11-06T17:55:08Z

test/import.bats

+  - path: folder1
+    dest: /folder2
+  run: |
+    [ -f /folder2/file1 ]


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.

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 :)

rchincha requested review from smoser and hallyn as code owners April 3, 2023 03:54

smoser requested changes Apr 3, 2023

View reviewed changes

pkg/stacker/import.go Outdated Show resolved Hide resolved

rchincha force-pushed the copy branch from de3c13f to ea14811 Compare April 3, 2023 18:46

andaaron reviewed Apr 3, 2023

View reviewed changes

test/import.bats Show resolved Hide resolved

rchincha force-pushed the copy branch 2 times, most recently from d067a1a to 7e5f826 Compare April 12, 2023 08:39

rchincha force-pushed the copy branch 6 times, most recently from 208a909 to 6e339fe Compare April 17, 2023 01:32

andaaron reviewed Apr 17, 2023

View reviewed changes

test/import.bats Show resolved Hide resolved

rchincha force-pushed the copy branch from ef4c03e to 4e128ad Compare April 19, 2023 01:19

andaaron approved these changes Apr 19, 2023

View reviewed changes

smoser reviewed Apr 19, 2023

View reviewed changes

rchincha force-pushed the copy branch from 950f7bd to 302cb33 Compare April 19, 2023 20:24

smoser mentioned this pull request Apr 20, 2023

Feat: allow creation of layers via stacking (maybe bring back apply?) #448

Open

rchincha added this to the v1.0.0 milestone Apr 21, 2023

This was referenced Jun 13, 2023

fix: move to official stacker project-stacker/c3#93

Open

fix: move to offical stacker URL project-stacker/c3#94

Open

rchincha force-pushed the copy branch from 302cb33 to 155618c Compare September 5, 2023 21:05

rchincha force-pushed the copy branch from 155618c to eae4a48 Compare October 4, 2023 21:56

rchincha force-pushed the copy branch 4 times, most recently from d1b9614 to c85b2fd Compare October 10, 2023 22:03

smoser requested changes Oct 11, 2023

View reviewed changes

rchincha force-pushed the copy branch 3 times, most recently from d081627 to f0ed4d2 Compare November 1, 2023 23:21

rchincha added 2 commits November 2, 2023 16:02

feat(import): copy dir contents

faa7772

import: - path: folder1/ dest: / vs import: - path: folder1 dest: / The former will copy the contents of folder1/ at / and the latter will make and copy a /folder1 at / Signed-off-by: Ramkumar Chinchani <[email protected]>

feat(import): allow dest dir renaming during import

91ed8ea

import: - path: folder1 dest: /folder2 will cause contents of folder1 to show up as and under /folder2 Signed-off-by: Ramkumar Chinchani <[email protected]>

rchincha force-pushed the copy branch from 45b3318 to ecf9826 Compare November 2, 2023 16:03

hallyn reviewed Nov 6, 2023

View reviewed changes

hallyn approved these changes Nov 6, 2023

View reviewed changes

rchincha force-pushed the copy branch from ecf9826 to 91ed8ea Compare November 6, 2023 18:05

rchincha merged commit 533c4a6 into project-stacker:main Nov 6, 2023
7 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(import): copy folder contents #453

feat(import): copy folder contents #453

rchincha commented Apr 3, 2023

rchincha commented Apr 3, 2023

andaaron left a comment

smoser Apr 19, 2023

rchincha Apr 19, 2023

smoser Apr 19, 2023 •

edited

Loading

rchincha Apr 19, 2023 •

edited

Loading

rchincha Apr 19, 2023

smoser Apr 19, 2023

smoser left a comment

rchincha commented Apr 28, 2023 •

edited

Loading

smoser commented Apr 28, 2023

rchincha commented Apr 28, 2023 •

edited

Loading

rchincha commented Apr 28, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

smoser left a comment

smoser Oct 11, 2023

rchincha Oct 16, 2023

smoser Oct 11, 2023

smoser Oct 11, 2023

rchincha Oct 16, 2023

smoser Oct 11, 2023

rchincha Oct 17, 2023

hallyn Nov 6, 2023

hallyn Nov 6, 2023

feat(import): copy folder contents #453

feat(import): copy folder contents #453

Conversation

rchincha commented Apr 3, 2023

rchincha commented Apr 3, 2023

andaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoser Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

rchincha Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoser left a comment

Choose a reason for hiding this comment

rchincha commented Apr 28, 2023 • edited Loading

smoser commented Apr 28, 2023

rchincha commented Apr 28, 2023 • edited Loading

rchincha commented Apr 28, 2023

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

smoser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoser Apr 19, 2023 •

edited

Loading

rchincha Apr 19, 2023 •

edited

Loading

rchincha commented Apr 28, 2023 •

edited

Loading

rchincha commented Apr 28, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading