Resolve power permissions issues #35
Conversation
Add configuration option to use `sudo` with power commands
WalkthroughThe changes introduce two new properties, Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
ovos_PHAL_plugin_system/__init__.py (3)
237-240: Approved: Reboot command now uses sudo based on configurationThe changes to the
handle_reboot_requestmethod successfully implement the use of sudo for the reboot command based on thesudo_powerproperty. This aligns with the PR objectives and addresses the issue of reboot commands failing due to lack of interactive authentication.For consistency with the property name, consider updating the condition:
if self.use_sudo_for_power: command = f"sudo {command}"This assumes you've renamed the property as suggested in the previous comment.
255-258: Approved: Shutdown command now uses sudo based on configurationThe changes to the
handle_shutdown_requestmethod successfully implement the use of sudo for the shutdown command based on thesudo_powerproperty. This is consistent with the changes made to the reboot method and aligns with the PR objectives.For consistency with the property name, consider updating the condition:
if self.use_sudo_for_power: command = f"sudo {command}"This assumes you've renamed the property as suggested in the first comment.
Line range hint
1-358: Summary: Successfully implemented configurable sudo usage for power commandsThe changes in this file successfully address the objectives of the PR and the issue described in #34. Here's a summary of the improvements:
- A new property
sudo_power(suggested to be renamed touse_sudo_for_power) has been added to control the use of sudo for power commands.- Both
handle_reboot_requestandhandle_shutdown_requestmethods have been updated to use sudo based on this property.- The default behavior maintains compatibility with previous versions by using sudo unless explicitly disabled in the configuration.
These changes resolve the power permissions issues and allow for interactive reboots without encountering hangs, improving the overall user experience and functionality of the system.
Consider adding a brief comment in the code explaining the purpose of the
use_sudo_for_powerconfiguration option and its default value. This will help future maintainers understand the rationale behind this feature.
|
wasnt the goal to move all sudo to PHAL admin plugin? This plugin is a Admin plugin, it needs to run as root and to be explicitly enabled in mycroft.conf {
"PHAL": {
"admin": {
"ovos-PHAL-plugin-system": {"enabled": true}
}
}
}if not enabled (omit config above) it will be run as the regular user, you need to ensure polkit policy is set to allow usage of systemctl without sudo. Not yet implemented |
🤦 Right. Just a missed config change on my part |
Add configuration option to use
sudowith power commands (defaultTruefor backwards-compat)Reverts behavior change introduced in #21
Closes #34
Summary by CodeRabbit
New Features
sudo_powerto enhance control over power-related commands.use_sudo_for_powerto managesudousage for power operations.Bug Fixes
sudousage.