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

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

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.

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.

Good catch. Fixed it so the test code will reject invalid tests.

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.

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

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

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]>

* 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]>

Some of the `fillDefaults` tests were testing invalid situations. To prevent this, validation is run on the test ins and outs before calling `fillDefaults`. The invalid tests have been removed. This commit also removes the sorting from `fillDefaults`, this will be left to the user instead. Signed-off-by: Peter Engelbert <[email protected]>

Instead of sorting the list of symlinks, leave it for the user to do. This commit removes the sorting from `fillDefaults` and implements it in the handlers. The map keys of the symlinks are now sorted as well. Signed-off-by: Peter Engelbert <[email protected]>

pmengelbert · 2025-02-06T15:16:22Z

This has been rebased and is ready for another round of review @cpuguy83

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 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 2 times, most recently from e4a8974 to cf1cf76 Compare January 16, 2025 16:48

cpuguy83 requested changes Jan 18, 2025

View reviewed changes

cpuguy83 added this to the v0.12.0 milestone Jan 29, 2025

pmengelbert force-pushed the pmengelbert/multiple_postinstall_symlinks_pointing_to_the_same_thing/1 branch from a21b267 to 6fd5303 Compare February 5, 2025 16:57

pmengelbert added 8 commits February 5, 2025 12:18

Update PostInstall symlink spec

49a3de1

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

Add tests, refine validation, add optimizations

858111a

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

Move Path entry to Paths in SymlinkTarget

7e6cf20

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

4547f8a

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

Add tests for ImageConfig.fillDefaults()

1aed48b

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

pmengelbert force-pushed the pmengelbert/multiple_postinstall_symlinks_pointing_to_the_same_thing/1 branch from 6fd5303 to 7a63ab6 Compare February 6, 2025 15:14

pmengelbert requested a review from cpuguy83 February 6, 2025 15:16

Address linter comments

fc06e2d

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

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

pmengelbert Feb 5, 2025

cpuguy83 Jan 18, 2025

pmengelbert Feb 5, 2025

cpuguy83 Jan 18, 2025

pmengelbert Feb 5, 2025

cpuguy83 Jan 18, 2025

pmengelbert Feb 5, 2025

pmengelbert commented Feb 6, 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

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 Feb 6, 2025

cpuguy83 Jan 10, 2025 •

edited

Loading