-
Notifications
You must be signed in to change notification settings - Fork 707
editflags: add --mount-only
flag
#3947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Similar to `--mount`, but overrides the existing mounts. e.g., ``` limactl start --mount-only "$(pwd)" ```` This will only make the current directory visible to the VM. The host home directory is not visible (unless pwd is the home). Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
Feedback can also be addressed in separate PRs (or just file open issues).
if err != nil { | ||
return "", err | ||
} | ||
expr := `.mounts += ` + mountListExpr + ` | .mounts |= unique_by(.location)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
expr := `.mounts += ` + mountListExpr + ` | .mounts |= unique_by(.location)` | |
// mount options take precedence over template settings | |
expr := fmt.Sprintf(".mounts = %s + .mounts", mountListExpr) |
Could also open a new issue to address later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func(_ *flag.Flag) (string, error) { | ||
incompatibleFlagNames := []string{"mount", "mount-only"} | ||
for _, name := range incompatibleFlagNames { | ||
ss, err := flags.GetStringSlice(name) | ||
if err != nil { | ||
return "", err | ||
} | ||
if len(ss) > 0 { | ||
return "", errors.New("flag `--mount-none` conflicts with `" + name + "`") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could potentially be a shared helper function because we have other subcommands that have mutually exclusive options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can add a helper when we have move conflicting options
Similar to
--mount
, but overrides the existing mounts.e.g.,
This will only make the current directory visible to the VM. The host home directory is not visible (unless pwd is the home).