feat: Multiple postinstall symlinks pointing to the same thing #495

pmengelbert · 2025-01-09T22:40:13Z

What this PR does / why we need it:
Currently, you can't create multiple symlink newpaths that point to the same oldpath. This is due to the fact that map keys have to be unique.

For more information, see #489

This should be merged before #494

Allow for multiple links to point to the same target. Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]>

cpuguy83 · 2025-01-10T00:17:56Z

spec.go

@@ -130,7 +130,8 @@ type PostInstall struct {
 // SymlinkTarget specifies the properties of a symlink
 type SymlinkTarget struct {
 	// Path is the path where the symlink should be placed
-	Path string `yaml:"path" json:"path" jsonschema:"required"`
+	Path  string   `yaml:"path" json:"path" jsonschema:"oneof_required=path"`


We should mark this is deprecated and have people use paths instead.
We can also make it so when we load the spec we migrate Path to Paths then there's only one field to deal with.

How do we mark it as deprecated?

https://go.dev/wiki/Deprecated

done, but I'm going to add tests for it.

tests added.

cpuguy83 · 2025-01-10T00:19:53Z

target.go

@@ -42,6 +42,12 @@ func (t *Target) validate() error {
 		}
 	}

+	if t.Image != nil {


Let's add a validate to t.Image, which itself could return nil if *Image is nil (note: this is what I did in #492

Upon loading the spec, if `Path` is nonempty, move it to `Paths`. This commit also reworks the tests. Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]>

pmengelbert · 2025-01-13T18:27:08Z

@cpuguy83 This should be ready for another round of review.

cpuguy83 · 2025-01-13T19:25:36Z

load.go

+
+	var errs []error
+	if err := i.Post.validate(); err != nil {
+		errs = append(errs, errors.Wrap(err, "postinstall"))


Nit, just "post"

cpuguy83 · 2025-01-13T19:28:38Z

load.go

+
+		// By this point, both the oldpath and the SymlinkTarget are
+		// well-formed. We still need to make sure each 'newpath' is unique.
+		if cfg.Paths == nil {


Validations should not be modifying anything.
We can also iterate on Paths without having to have it be non-nil.

cpuguy83 · 2025-01-13T19:28:57Z

load.go

+			cfg.Paths = []string{}
+		}
+
+		newpaths := slices.Clone(cfg.Paths)


Why the clone? Should not need to modify anything here.
If we want to make sure that cfg.Path is not also somewhere in cfg.Paths you can check that directly.

for _, link := range symlinks { if link.Path == cfg.Path { // error } }

I could have sworn I put this in fillDefaults. I need to move this code there. The clone from moving the old code around. I'll take another pass

Now I remember. Clone so we don't modify the original, but stick the cfg.Path field on the end so we can just loop through. It's weird, I'll change it.

Why the clone? Should not need to modify anything here. If we want to make sure that cfg.Path is not also somewhere in cfg.Paths you can check that directly.

for _, link := range symlinks { if link.Path == cfg.Path { // error } }

And the goal is to make sure the there are no duplicate oldpaths in the whole map[string]SymlinkTarget. The desired behavior is unclear (follow the symlink that has already been created? Or overwrite the existing symlink?) Map iteration is not reliably ordered (and we sort the keys to avoid cache misses), so the user is unlikely to get what they want anyway. Better to just not allow it.

done. This exhibits the desired behavior, doesn't modify the spec, is tested properly, and only allocates the map when needed.

cpuguy83 · 2025-01-13T19:34:41Z

load.go

+
+func validateSymlinks(symlinks map[string]SymlinkTarget) error {
+	var errs []error
+	dests := make(map[string]string, len(symlinks)<<1)


Maybe lets call this seen and only allocate the map if its needed.

* no longer modifies anything in `.validate()` * only allocates map when it's actually needed * stipulates in the test that `.Paths` should end up sorted (this is for llb caching) * s/postinstall/post Signed-off-by: Peter Engelbert <[email protected]>

cpuguy83 · 2025-01-18T00:33:58Z

frontend/windows/handle_container.go

-			s = s.File(llb.Mkdir(path.Dir(dstPath), 0755, llb.WithParents(true)))
-			s = s.File(llb.Copy(s, srcPath, dstPath))
+
+		for oldpath, newpaths := range post.Symlinks {


I think we still need to sort this.

cpuguy83 · 2025-01-18T00:34:30Z

helpers.go

-		for src, tgt := range post.Symlinks {
-			fmt.Fprintf(buf, "mkdir -p %q\n", filepath.Join(rootfsPath, filepath.Dir(tgt.Path)))
-			fmt.Fprintf(buf, "ln -s %q %q\n", src, filepath.Join(rootfsPath, tgt.Path))
+		for oldpath, newpaths := range post.Symlinks {


Should also sort this

cpuguy83 · 2025-01-18T00:38:43Z

load_test.go

+				Post: &PostInstall{
+					Symlinks: map[string]SymlinkTarget{
+						"oldpath": {
+							Path:  "/newpath3",


I don't think we need to test this since its not a valid case.

cpuguy83 · 2025-01-18T00:40:53Z

target.go

+			cfg.Path = ""
+		}
+
+		sort.Strings(cfg.Paths)


I don't think this should be sorted since this should be left to the user.

pmengelbert added 2 commits January 9, 2025 17:10

Update PostInstall symlink spec

404e2a1

Allow for multiple links to point to the same target. Signed-off-by: Peter Engelbert <[email protected]>

Add tests, refine validation, add optimizations

c4cea6d

Signed-off-by: Peter Engelbert <[email protected]>

pmengelbert requested a review from a team as a code owner January 9, 2025 22:40

cpuguy83 reviewed Jan 10, 2025

View reviewed changes

cpuguy83 requested changes Jan 10, 2025

View reviewed changes

pmengelbert added 3 commits January 13, 2025 11:25

Move Path entry to Paths in SymlinkTarget

1f4d654

Upon loading the spec, if `Path` is nonempty, move it to `Paths`. This commit also reworks the tests. Signed-off-by: Peter Engelbert <[email protected]>

Add doc comment deprecating Path in SymlinkTarget

04ba998

Signed-off-by: Peter Engelbert <[email protected]>

Add tests for ImageConfig.fillDefaults()

ae58534

Signed-off-by: Peter Engelbert <[email protected]>

pmengelbert requested a review from cpuguy83 January 13, 2025 18:27

cpuguy83 requested changes Jan 13, 2025

View reviewed changes

pmengelbert force-pushed the pmengelbert/multiple_postinstall_symlinks_pointing_to_the_same_thing/1 branch from 08fa341 to e4a8974 Compare January 16, 2025 16:41

pmengelbert force-pushed the pmengelbert/multiple_postinstall_symlinks_pointing_to_the_same_thing/1 branch from e4a8974 to cf1cf76 Compare January 16, 2025 16:48

cpuguy83 requested changes Jan 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Multiple postinstall symlinks pointing to the same thing #495

feat: Multiple postinstall symlinks pointing to the same thing #495

pmengelbert commented Jan 9, 2025

cpuguy83 Jan 10, 2025 •

edited

Loading

pmengelbert Jan 10, 2025

cpuguy83 Jan 10, 2025

pmengelbert Jan 13, 2025

pmengelbert Jan 13, 2025

cpuguy83 Jan 10, 2025

pmengelbert Jan 13, 2025

pmengelbert commented Jan 13, 2025

cpuguy83 Jan 13, 2025

pmengelbert Jan 16, 2025

cpuguy83 Jan 13, 2025

pmengelbert Jan 16, 2025

cpuguy83 Jan 13, 2025

pmengelbert Jan 16, 2025

pmengelbert Jan 16, 2025

pmengelbert Jan 16, 2025

pmengelbert Jan 16, 2025

cpuguy83 Jan 13, 2025

pmengelbert Jan 16, 2025

cpuguy83 Jan 18, 2025

cpuguy83 Jan 18, 2025

cpuguy83 Jan 18, 2025

cpuguy83 Jan 18, 2025

feat: Multiple postinstall symlinks pointing to the same thing #495

Are you sure you want to change the base?

feat: Multiple postinstall symlinks pointing to the same thing #495

Conversation

pmengelbert commented Jan 9, 2025

cpuguy83 Jan 10, 2025 • edited Loading

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

pmengelbert commented Jan 13, 2025

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

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

cpuguy83 Jan 10, 2025 •

edited

Loading