Skip to content
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

limactl shell: ask whether to start the instance if not running #2763

Closed
wants to merge 1 commit into from

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Oct 18, 2024

Before

$ lima
FATA[0000] instance "default" does not exist, run `limactl create default` to create a new instance

After

$ lima
ERRO[0000] instance "default" does not exist, run `limactl create default` to create a new instance 
? Do you want to create and start the instance "default" using the default template now? Yes
? Creating an instance "default" Proceed with the current configuration
[...]
INFO[0114] READY.
suda@lima-default:/Users/suda/gopath/src/github.com/lima-vm/lima$

@AkihiroSuda AkihiroSuda added enhancement New feature or request area/cli limactl CLI user experience labels Oct 18, 2024
@AkihiroSuda AkihiroSuda marked this pull request as ready for review October 18, 2024 20:38
@AkihiroSuda AkihiroSuda added this to the v1.0 milestone Oct 20, 2024
@AkihiroSuda AkihiroSuda requested a review from a team October 22, 2024 07:52
@afbjorklund
Copy link
Member

I think the improvement of the "lima" command is good, but haven't looked at the code yet...

We might need something similar for the shell wrappers, like docker.lima and podman.lima?

instance "docker" does not exist, run `limactl create --name=docker template://docker` to create a new instance
instance "podman" does not exist, run `limactl create --name=podman template://podman` to create a new instance

The nerdctl.lima wrapper should be good, since it only calls lima (which calls limactl shell)

It could need some work, to get the same "user interface" in the bash script (as with survey)

@AkihiroSuda
Copy link
Member Author

We might need something similar for the shell wrappers, like docker.lima and podman.lima?

Yes, but it is beyond the scope of this PR

@afbjorklund
Copy link
Member

The current user experience is actually two steps, one to create and one to start:

$ lima
FATA[0000] instance "default" does not exist, run `limactl create default` to create a new instance 
$ limactl create default
? Creating an instance "default" Proceed with the current configuration
...
INFO[0003] Run `limactl start default` to start the instance. 
$ lima
FATA[0000] instance "default" is stopped, run `limactl start default` to start the instance 
$ limactl start default
...
INFO[0068] READY. Run `lima` to open the shell.         

Or maybe three steps, considering that you still have to run the original lima command again?

So it's a good thing reducing 3 to 1.

@afbjorklund
Copy link
Member

Yes, but it is beyond the scope of this PR

Didn't find any issue for the entire change.

cmd/limactl/shell.go Show resolved Hide resolved
cmd/limactl/start.go Outdated Show resolved Hide resolved
cmd/limactl/start.go Show resolved Hide resolved
cmd/limactl/start.go Show resolved Hide resolved
@AkihiroSuda
Copy link
Member Author

What should we do with this?
@lima-vm/maintainers

@jandubois
Copy link
Member

What should we do with this?

I'll try to take a look soon. I hope we can get this into 1.0.

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this isn't a complete review, but I think the mandatory logic is not working correctly (it shows error messages multiple times).

I'm suggesting an alternative, but I'm not totally sure if it will work; I would need to try it myself. Let me know if you want me to make an attempt!

//
// If mandatory is set to true, and the answer to the confirmation prompt is No,
// the function returns an error that contains an instruction to start the instance manually.
func startInteractive(cmd *cobra.Command, instName string, newInst, mandatory bool) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear what the bool return value represents. It also seems to be completely unused; all 3 call-sites are _, err = startInteractive(…).

return ans, nil
}
if newInst {
rootCmd := cmd.Root()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rootCmd could be setup outside the conditional because it is needed again later.

}
if newInst {
rootCmd := cmd.Root()
// The create command shows the template chooser UI, etc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected that the template would default to the instance name, if a template existed with that name. Otherwise it should show an error and only then default to default.

$ l shell alpine
ERRO[0000] instance "alpine" does not exist, run `limactl create alpine` to create a new instance
? Do you want to create and start the instance "alpine" using the default template now?

This should say "using the alpine template".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the mandatory mechanism is too complex/confusing. It also results in the same error being displayed twice, first as ERRO and then as FATA:

$ l shell alpine
ERRO[0000] instance "alpine" does not exist, run `limactl create alpine` to create a new instance
? Do you want to create and start the instance "alpine" using the default template now? No
FATA[0002] instance "alpine" does not exist, run `limactl create alpine` to create a new instance

I think the mandatory argument can just be removed, and the function always just returns nil when the user elected not to create/start the instance.

The caller is already calling store.Inspect() afterwards, and can then simply exit if the instance isn't in running state.

@jandubois
Copy link
Member

Let me know if you want me to make an attempt!

Ok, I went ahead and created #2832, which I think is simpler to follow. LMK what you think!

@AkihiroSuda AkihiroSuda removed this from the v1.0 milestone Nov 3, 2024
@AkihiroSuda AkihiroSuda closed this Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli limactl CLI user experience enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants