-
Notifications
You must be signed in to change notification settings - Fork 281
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
shutdown/factory-reset: ignore missing lima-home dir error. #5105
Conversation
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.
Don't call limactl
if you cannot setup LIMA_HOME
.
This should probably go into a separate PR, but I noticed that Maybe also audit if there is other now redundant path logic in other places. |
1a968de
to
f731ab6
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.
I have 2 more questions; not sure if they require changes, but I suspect they do.
if err != nil { | ||
return err | ||
} |
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.
Why do these errors abort the shutdown sequence? Should they also just be logged, and processing continues?
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.
Good point; processing should continue.
default: | ||
return fmt.Errorf("internal error: unknown shutdown initiating command of '%s'", initiatingCommand) | ||
} | ||
err = s.waitForAppToDieOrKillIt(checkProcessQemu, pkillQemu, 15, 2, "qemu") |
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.
Why is the Qemu shutdown inside the Lima conditional? It does not rely on LIMA_HOME
or limactl
as far as I can tell.
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 assumed it was part of lima, but moving it out is fine.
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.
It is, but you don't need LIMA_HOME
or limactl
to kill it, so you can still try...
Don't check or kill lima and qemu processes when the path to the lima directory can't be found. Signed-off-by: Eric Promislow <[email protected]>
f731ab6
to
26ed6dd
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.
Thanks, LGTM
Fixes #4600