editflags: update the expression for mount flag

[ashwat287]

fixes #3959

[jandubois]

The change looks fine to me, but I've since noticed that the problem is much more convoluted, with no clear solution. Thankfully it is so obscure that I think nobody will ever run into it.

First thing is that --mount /foo only sets the location, but not the mountPoint. So even if we move the commandline definitions to the front, the template could still override the mountPoint:

❯ cat tmpl.yaml
base: template://default
mounts:
- location: /foo
  mountPoint: /bar

❯ l create -y tmpl.yaml --mount /foo
...

❯ l ls tmpl --yq '.config.mounts[] | [.location, .mountPoint]'
[
    "/foo",
    "/bar"
]
[
    "/Users/jan",
    "/Users/jan"
]

So even though the commandline specified --mount /foo, implying it would mount under /foo, it will actually still be mounted under /bar because of the way additional settings are propagated.

This might be a bug, but it could also be prevented if --mount /foo meant .mounts += [{"location": "/foo", "mountPoint": "/foo"}] instead of just .mounts += [{"location": "/foo"].

So I would be in favour of making this change as well.

But the other issue is that .mounts can also be modified via --set, and the way the options are processed is that all the --mount options are applied first, followed by all the --set options:

❯ l create -y --name foo --mount /foo --set '.mounts += [{"location": "/bar"}]' --mount /baz

❯ l ls foo --yq '.config.mounts[].location'
/foo
/baz
/Users/jan
/bar

And even if we managed to process the settings interleaved in the order they appear on the command line, we would still not have control over the user writing .mount = [...] + .mounts instead of .mounts += [...], so the order would likely still be wrong.

With the correct expression it looks like this:

❯ l create -y --name foo --mount /foo --set '.mounts = [{"location": "/bar"}] + .mounts' --mount /baz
...

❯ l ls foo --yq '.config.mounts[].location'
/bar
/foo
/baz
/Users/jan

But now /bar ends up at the front instead of between /foo and /bar.

Of course /foo and /bar are also in the wrong order because the last definition should come first. So we should reverse the order of the mount expressions before calling buildMountListExpression.

I think this is a problem that does not need to be solved; it is a lot of complexity for something that probably nobody will ever run into.

[jandubois]

Since this is all very confusing, these are the changes I would make:

--- cmd/limactl/editflags/editflags.go
+++ 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
 				}

But I've also confirmed that the merging of mounts in FillDefaults is broken. It also matches on location instead of mountPoint. This comes from the time when we did not yet have a way to specify separate mount points, and they were always the same.

[jandubois]

If #4046 is merged first, then I think adding the mountPoint is no longer necessary because FillDefaults() should do the right thing automatically. I have not tested this though.

[ashwat287]

First thing is that --mount /foo only sets the location, but not the mountPoint. So even if we move the commandline definitions to the front, the template could still override the mountPoint:

❯ cat tmpl.yaml
base: template://default
mounts:
- location: /foo
  mountPoint: /bar

❯ l create -y tmpl.yaml --mount /foo
...

❯ l ls tmpl --yq '.config.mounts[] | [.location, .mountPoint]'
[
    "/foo",
    "/bar"
]
[
    "/Users/jan",
    "/Users/jan"
]

So even though the commandline specified --mount /foo, implying it would mount under /foo, it will actually still be mounted under /bar because of the way additional settings are propagated.

This might be a bug, but it could also be prevented if --mount /foo meant .mounts += [{"location": "/foo", "mountPoint": "/foo"}] instead of just .mounts += [{"location": "/foo"].

So I would be in favour of making this change as well.

can we prompt the user whether they would want to override the mountPoint with the new mountPoint?
If yes they would be able to mount override the mountPoint.
If no the instance created/edited would retain the template mount configuration.

The prompt would look something like this:
The mount with the location: /foo already exists with the mountPoint: /bar, would you like to override the mountPoint with /foo?(Y/N)

[jandubois]

can we prompt the user whether they would want to override the mountPoint with the new mountPoint?

I don't think we want to prompt the user. If there is a problem with their arguments, we show an error and let them fix the command.

But checking settings conflicts is very complex, and I don't think we should do it. We have rules how settings are merged (essentially later definitions can update empty fields in earlier definitions based on a common key, but otherwise are ignored). This mechanism exists for mounts, additionalDisks, and networks.

It is currently implemented in both the template embedding code, and also in FillDefaults(). The redundant (and as we've just seen sometimes divergent) implementation in FillDefaults() should be replaced by using template embedding for defaults.yaml, override.yaml, and the builtin defaults as well, but it needs time to implement it properly.

[ashwat287]

Since this is all very confusing, these are the changes I would make:

--- cmd/limactl/editflags/editflags.go
+++ 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
 				}

But I've also confirmed that the merging of mounts in FillDefaults is broken. It also matches on location instead of mountPoint. This comes from the time when we did not yet have a way to specify separate mount points, and they were always the same.

Then, i guess I'll just make these suggested changes.

[jandubois]

Thanks, LGTM