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.

Sign up for GitHub

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

Jump to bottom

CLI flag to generate autostart files #2151

Merged
merged 1 commit into from
Mar 1, 2024
Merged

CLI flag to generate autostart files #2151

merged 1 commit into from
Mar 1, 2024

Conversation

roman-kiselenko
Copy link
Contributor

@roman-kiselenko roman-kiselenko commented Jan 18, 2024 •
edited
Loading

Added a new public CLI command: limactl start-at-login INSTANCE --enabled.
This command facilitates the generation of unit files for launchd/systemd, providing users with a straightforward way to control limactl autostart behavior.

Simplified Integration:

Adjusted launchd/systemd integration to start the limactl hostagent directly, simplifying the launch process.

Fix #2142

Partialy based on #2140

@jandubois jandubois mentioned this pull request Jan 18, 2024
Closed
AkihiroSuda
AkihiroSuda reviewed Jan 19, 2024
View reviewed changes
pkg/start/start.go Outdated Show resolved Hide resolved
jandubois
jandubois reviewed Jan 19, 2024
View reviewed changes
cmd/limactl/editflags/editflags.go Outdated Show resolved Hide resolved
pkg/autostart/io.lima-vm.autostart.INSTANCE.plist Outdated Show resolved Hide resolved
pkg/autostart/io.lima-vm.autostart.INSTANCE.plist Outdated Show resolved Hide resolved
pkg/start/start.go Outdated Show resolved Hide resolved
@roman-kiselenko roman-kiselenko changed the title CLI command to generate autostart files [WIP] CLI command to generate autostart files Jan 23, 2024
@roman-kiselenko roman-kiselenko force-pushed the feature/autostart branch 5 times, most recently from 4aa5475 to 166cb8e Compare January 31, 2024 08:39
@roman-kiselenko roman-kiselenko changed the title [WIP] CLI command to generate autostart files CLI flag to generate autostart files Jan 31, 2024
@roman-kiselenko roman-kiselenko marked this pull request as ready for review January 31, 2024 08:43
@roman-kiselenko
Copy link
Contributor Author

I guess the note in the documentation would be good since the behaviour of --start-at-login is quite complex. 🤔

@AkihiroSuda AkihiroSuda added this to the v0.21.0 milestone Feb 1, 2024
@roman-kiselenko
Copy link
Contributor Author

Review request @lima-vm/maintainers

AkihiroSuda
AkihiroSuda reviewed Feb 2, 2024
View reviewed changes
cmd/limactl/edit.go Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda reviewed Feb 2, 2024
View reviewed changes
cmd/limactl/delete.go Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda reviewed Feb 2, 2024
View reviewed changes
cmd/limactl/edit.go Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda reviewed Feb 2, 2024
View reviewed changes
cmd/limactl/start.go Outdated Show resolved Hide resolved
@roman-kiselenko
Copy link
Contributor Author

roman-kiselenko commented Feb 21, 2024 •
edited
Loading

Moved to separate command, I guess now it's ready.

@lima-vm/maintainers

AkihiroSuda
AkihiroSuda previously approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

jandubois
jandubois requested changes Feb 25, 2024
View reviewed changes
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.

Most of the feedback is minor, but the incorrect order of the template rendering arguments is a blocker.

cmd/limactl/start-at-login.go Outdated Show resolved Hide resolved
cmd/limactl/start-at-login.go Outdated Show resolved Hide resolved
cmd/limactl/start-at-login.go Show resolved Hide resolved
pkg/autostart/autostart.go Outdated Show resolved Hide resolved
pkg/autostart/autostart.go Outdated Show resolved Hide resolved
pkg/autostart/autostart.go Show resolved Hide resolved
pkg/autostart/autostart.go Outdated Show resolved Hide resolved
@roman-kiselenko roman-kiselenko force-pushed the feature/autostart branch 2 times, most recently from ea38cdd to 69288b9 Compare February 25, 2024 08:04
@roman-kiselenko
Copy link
Contributor Author

roman-kiselenko commented Feb 25, 2024 •
edited
Loading

Most of the feedback is minor, but the incorrect order of the template rendering arguments is a blocker.

@jandubois Thanks for your review 🙏
Found some bugs

AkihiroSuda
AkihiroSuda previously approved these changes Feb 26, 2024
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

jandubois
jandubois requested changes Feb 27, 2024
View reviewed changes
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.

I think the PR is almost ready for merging.

The only functional change I would like to see is the removal of the error message when an instance is deleted and no autostart file exists. That is not an error, but expected.

Otherwise I found some conditionals that seem to be always true/false and therefore can be eliminated.

The rest of the feedback is just nitpicking about wording/formatting of the log messages and help text.

cmd/limactl/delete.go Outdated Show resolved Hide resolved
cmd/limactl/delete.go Outdated Show resolved Hide resolved
cmd/limactl/delete.go Outdated Show resolved Hide resolved
cmd/limactl/start-at-login.go Outdated Show resolved Hide resolved
cmd/limactl/start-at-login.go Outdated Show resolved Hide resolved
cmd/limactl/start-at-login.go Outdated Show resolved Hide resolved
pkg/autostart/autostart.go Outdated Show resolved Hide resolved
pkg/autostart/autostart.go Show resolved Hide resolved
pkg/autostart/autostart.go Outdated Show resolved Hide resolved
jandubois
jandubois requested changes Mar 1, 2024
View reviewed changes
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.

I think 2 log messages are not correct yet.

The other 2 comments are just removing the parens in (%q) for consistency.

Otherwise it looks good to go.

cmd/limactl/delete.go Outdated Show resolved Hide resolved
cmd/limactl/start-at-login.go Outdated Show resolved Hide resolved
cmd/limactl/start-at-login.go Outdated Show resolved Hide resolved
cmd/limactl/start-at-login.go Outdated Show resolved Hide resolved
@roman-kiselenko
Added a new public CLI command: `limactl start-at-login INSTANCE
1ed3dc2 
--enabled`.
This command facilitates the generation of unit files for `launchd/systemd`,
providing users with a straightforward way to control `limactl` autostart behavior.

Signed-off-by: roman-kiselenko <roman.kiselenko.dev@gmail.com>
@roman-kiselenko
Copy link
Contributor Author

I think 2 log messages are not correct yet.

The other 2 comments are just removing the parens in (%q) for consistency.

Otherwise it looks good to go.

@jandubois Thank you for your careful review 🙏

jandubois
jandubois approved these changes Mar 1, 2024
View reviewed changes
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.

Thank you, looks great!

@AkihiroSuda AkihiroSuda merged commit 4bc77d5 into lima-vm:master Mar 1, 2024
24 checks passed
@roman-kiselenko roman-kiselenko deleted the feature/autostart branch March 1, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@jandubois jandubois jandubois approved these changes

@norio-nomura norio-nomura Awaiting requested review from norio-nomura

@balajiv113 balajiv113 Awaiting requested review from balajiv113

Assignees
No one assigned
Labels
area/cli limactl CLI user experience
Projects
None yet
Milestone
v0.21.0
Development

Successfully merging this pull request may close these issues.

Add CLI command to generate launchd units for macOS hosts (systemd units for Linux hosts)
5 participants
@roman-kiselenko @AkihiroSuda @jandubois @balajiv113 @norio-nomura