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

Do not hardcode reboot command #5208

Merged

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Apr 24, 2024

Proposed Commit Message

fix(cmd): Do not hardcode reboot command

Attempting a `cloud-init clean --reboot` fails on alpine because this
command is hard-coded. This code already has an Init object available,
which has a Distro attribute. Stamp out the duplicate hard-coded
implementation and re-use distro reboot code to acquire cross-distro
compatiblity.

Error:
Could not reboot this system using "['shutdown', '-r', 'now']": Unexpected error while running comma  nd.
Command: ['shutdown', '-r', 'now']
Exit code: -
Reason: [Errno 2] No such file or directory: b'shutdown'
Stdout: -
Stderr: -

Context

This change is mostly test updates - the code change is trivial: just reuse the existing reboot code which already has multi-distro support.

Attempting a `cloud-init clean --reboot` fails on alpine because this
command is hard-coded. This code already has an Init object available,
which has a Distro attribute. Stamp out the duplicate hard-coded
implementation and re-use distro reboot code to acquire cross-distro
compatiblity.

Error:
Could not reboot this system using "['shutdown', '-r', 'now']": Unexpected error while running command.
Command: ['shutdown', '-r', 'now']
Exit code: -
Reason: [Errno 2] No such file or directory: b'shutdown'
Stdout: -
Stderr: -
@dermotbradley
Copy link
Contributor

I guess I've never tried a cloud-init clean --reboot before.

@holmanb
Copy link
Member Author

holmanb commented Apr 25, 2024

I guess I've never tried a cloud-init clean --reboot before.

It's honestly not much of a value add, so I'm not terribly surprised. I like it because it shaves off a command when iterating for development and it gets used a fair amount in integration testing so I figure it's worth supporting for all distros.

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

One question, but otherwise LGTM!

@@ -0,0 +1,9 @@
import pytest
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of this file?

Copy link
Member Author

@holmanb holmanb Apr 30, 2024

Choose a reason for hiding this comment

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

That was a mistake. Thanks, I'll drop this file.

@holmanb holmanb merged commit efd37c0 into canonical:main May 1, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants