Repository: lima-vm/lima

Issue:
editflags: `--mount`: `.mounts |= unique_by(.location)` seems incorrect
> I know this is just existing code that is being moved, but the logic is incorrect: the mounts need to be unique by `.mountPoint`, not by `.location`. The same location can be mounted to several mount points, and this logic would delete all but one of them.
>
> But the mount point may only be filled in later (defaults to location), so we can't do anything here beyond making sure that the new mount points are defined first, so they will overrride existing mounts (mostly relevant only for the `writable` property).
>
> I think this would be better (but have not tested it):
>
> ```suggestion
> 				// mount options take precedence over template settings
> 				expr := fmt.Sprintf(".mounts = %s + .mounts", mountListExpr)
> ```
>
> Could also open a new issue to address later.

_Originally posted by @jandubois in https://github.com/lima-vm/lima/pull/3947#discussion_r2319832679_
            

Discussion:

Solution patch:
diff --git a/cmd/limactl/editflags/editflags.go b/cmd/limactl/editflags/editflags.go
index 6b8d21d6..7047a79c 100644
--- a/cmd/limactl/editflags/editflags.go
+++ b/cmd/limactl/editflags/editflags.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"math/bits"
 	"runtime"
+	"slices"
 	"strconv"
 	"strings"
 
@@ -179,7 +180,7 @@ func buildMountListExpression(ss []string) (string, error) {
 		if err != nil {
 			return "", err
 		}
-		expr += fmt.Sprintf(`{"location": %q, "writable": %v}`, loc, writable)
+		expr += fmt.Sprintf(`{"location": %q, "mountPoint": %q, "writable": %v}`, loc, loc, writable)
 		if i < len(ss)-1 {
 			expr += ","
 		}
@@ -225,6 +226,7 @@ func YQExpressions(flags *flag.FlagSet, newInstance bool) ([]string, error) {
 			"mount",
 			func(_ *flag.Flag) (string, error) {
 				ss, err := flags.GetStringSlice("mount")
+				slices.Reverse(ss)
 				if err != nil {
 					return "", err
 				}
@@ -232,7 +234,8 @@ func YQExpressions(flags *flag.FlagSet, newInstance bool) ([]string, error) {
 				if err != nil {
 					return "", err
 				}
-				expr := `.mounts += ` + mountListExpr + ` | .mounts |= unique_by(.location)`
+				// mount options take precedence over template settings
+				expr := fmt.Sprintf(".mounts = %s + .mounts", mountListExpr)
 				mountOnly, err := flags.GetStringSlice("mount-only")
 				if err != nil {
 					return "", err



Test patch:
diff --git a/cmd/limactl/editflags/editflags_test.go b/cmd/limactl/editflags/editflags_test.go
index ae9e178e..c580bb0d 100644
--- a/cmd/limactl/editflags/editflags_test.go
+++ b/cmd/limactl/editflags/editflags_test.go
@@ -181,13 +181,13 @@ func TestYQExpressions(t *testing.T) {
 			name:        "mount",
 			args:        []string{"--mount", "/foo", "--mount", "./bar:w"},
 			newInstance: false,
-			expected:    []string{`.mounts += [{"location": "` + expand("/foo") + `", "writable": false},{"location": "` + expand("./bar") + `", "writable": true}] | .mounts |= unique_by(.location)`},
+			expected:    []string{`.mounts = [{"location": "` + expand("./bar") + `", "mountPoint": "` + expand("./bar") + `", "writable": true},{"location": "` + expand("/foo") + `", "mountPoint": "` + expand("/foo") + `", "writable": false}] + .mounts`},
 		},
 		{
 			name:        "mount-only",
 			args:        []string{"--mount-only", "/foo", "--mount-only", "/bar:w"},
 			newInstance: false,
-			expected:    []string{`.mounts = [{"location": "` + expand("/foo") + `", "writable": false},{"location": "` + expand("/bar") + `", "writable": true}]`},
+			expected:    []string{`.mounts = [{"location": "` + expand("/foo") + `", "mountPoint": "` + expand("/foo") + `", "writable": false},{"location": "` + expand("/bar") + `", "mountPoint": "` + expand("/bar") + `", "writable": true}]`},
 		},
 		{
 			name:        "mixture of mount and mount-only",