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
create a shutdown_command method in distro classes #567
create a shutdown_command method in distro classes #567
Conversation
9b16140
to
983aaa2
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.
wouldn't this be a candidate to go into the distro classes, rather than having to map it out here, we should only be calling the distro's shutdown method
yes, it would be better. Note though that Eventually, I can sign the CLA if this piece of code is agreed upon. Who's the "canonical project manager or conact" here ? (unless advised otherwise, I'd rather force-push a single commit with the different changes here). |
according to https://cloudinit.readthedocs.io/en/latest/topics/hacking.html it's ‘Rick Harding’. |
Under FreeBSD, we want to use "shutdown -p" for poweroff. Alpine linux also has some specificities. We choose to define a method that returns the shutdown command line to use, rather than a method that actually does the shutdown. This makes it easier to have the tests in test_handler_power_state do their verifications. Two tests are added for the special behaviours that are known so far.
b843f87
to
2c5d0ef
Compare
/me waves When you feel it's ready and the CLA is signed let me know and we'll get someone to help review and guide it through. Thanks! |
At this point I'd be happy to get feedback. The patch can be reviewed. As the gitlab bot says, CLA signed and all tests pass. |
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.
Oops, I reviewed this a few days ago but never hit the button, apologies for the delay. Thanks for this change, the refactor onto Distro
is much appreciated! I have some inline comments. :-)
(And sorry to make you duplicate a review, Lucas!)
On Thu, Sep 10, 2020 at 12:58:32PM -0700, lucasmoura wrote:
+ opt_map = {'halt': '-H', 'poweroff': '-p', 'reboot': '-r'}
Can we turn that variable into a `Distro` attribute ? We would them just need to update it here instead of redefining the whole `shutdown_command` method
IIUC, that entails:
- Keep the override for alpine linux, which only groks seconds and not
minutes, and no message.
- have opt_map be self.shutdown_opt_map, initialized by Distro.__init__
first, and overwritten by BSD.__init__
Yes, that would do, and eliminate the cumbersome override of the whole
command.
Do we agree that this is what you have in mind ?
|
I can't speak for Lucas, but this is exactly what I had in mind when I wrote my similar comment a few days ago. 👍 |
@OddBloke @emmanuelthome Yes, that was my idea too |
Co-authored-by: Daniel Watkins <daniel@daniel-watkins.co.uk>
This follows from the review suggestions. Indeed, we can avoid the cumbersome duplication of the Distro.shutdown_command in the BSD command by mere variation of a shutdown_options_map class object.
3ec4b9b
to
ae8ee25
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 for this second pass, this is looking really good now. I do still have a few (small!) inline comments.
Co-authored-by: Daniel Watkins <daniel@daniel-watkins.co.uk>
Co-authored-by: Daniel Watkins <daniel@daniel-watkins.co.uk>
Co-authored-by: Daniel Watkins <daniel@daniel-watkins.co.uk>
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.
LGTM now, thanks for all your work!
See https://www.freebsd.org/cgi/man.cgi?shutdown(8)