-
Notifications
You must be signed in to change notification settings - Fork 706
Use --start flag to start VM for 'clone' and 'edit' commands #4108
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
base: master
Are you sure you want to change the base?
Conversation
2e4240b
to
9e19785
Compare
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.
Please wait for feedback from @AkihiroSuda before making any changes.
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 still think we should remove --yes
from being a global option that shows up in limactl help
. start
, edit
, and clone
are the only commands that check the value of the --tty
setting, so it really only applies to start
now.
So I would make it a local option in those 3 commands, and make it hidden in edit
and clone
, so it really becomes just a start
option.
@AkihiroSuda You seemed to have reservations about making the deprecated option hidden. What is the reason for that?
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.
You seemed to have reservations about making the deprecated option hidden. What is the reason for that?
Because we have been keeping other deprecated commands/options.
(limactl show-ssh
)
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.
Yes, but I don't understand why this makes sense: the point of deprecation is to transition away from something because it will go away. Keeping it available for new users to discover and use is not helpful.
Only users that already know about these commands (or existing scripts) should continue to use them, but get a warning, so they can transition away before the commands get removed. If there is no intention of removing a command or feature, then there is no point in deprecating it.
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.
So I would make it a local option in those 3 commands, and make it hidden in
edit
andclone
, so it really becomes just astart
option.
We definitely need to make it a local option if we don't want to hide the deprecated option. That way we can mark it in the --help
output for edit
and clone
as deprecated (which it isn't right now):
❯ l edit --help
Edit an instance of Lima or a template
...
Global Flags:
...
-y, --yes Alias of --tty=false
But also so that --yes
doesn't show up for every single subcommand when it really is just an option for start
anymore.
One big reason for getting rid of --yes
is to avoid user confusion, and we are not achieving this by keeping it very visible.
cmd/limactl/main.go
Outdated
if cmd.Flags().Changed("yes") { | ||
switch cmd.Name() { | ||
case "clone", "edit": | ||
logrus.Warn("--yes flag is deprecated for the '" + cmd.Name() + "' command, use --start instead and --tty=false") |
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 don't really understand the warning. --start
has different semantics than --yes
, so --start=false
may be the same as --yes=true
, but --start=true
(start without asking) is different from --yes=false
(ask about starting).
I think the edit
and clone
commands should just print the simplified message:
logrus.Warn("--yes flag is deprecated for the '" + cmd.Name() + "' command, use --start instead and --tty=false") | |
logrus.Warn("The --yes flag is deprecated") |
Users can then run limactl edit --help
to learn more.
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.
What about:
logrus.Warn("--yes flag is deprecated for the '" + cmd.Name() + "' command, use --start instead and --tty=false") | |
logrus.Warn("--yes flag is deprecated (--tty=false is still supported and works in the same way. Also consider using --start)") |
Signed-off-by: Praful Khanduri <[email protected]>
9e19785
to
c57a0c8
Compare
This PR addresses [Issue #3995] regarding the confusing and inconsistent behavior of the --yes flag in limactl clone and limactl edit. The following changes have been made:
Changes
--yes
flag for the clone and edit commands. A warning is shown when used, recommending--start
instead.--start
flag (default: true) to both commands, allowing users to explicitly control whether the instance is started after cloning or editing.