[14.0][IMP] cetmix_tower_server: use checkbox for sudo mode #224
Conversation
Replace the sudo selector with a checkbox in the command wizard to ensure the sudo mode is determined by the server settings. This change avoids mismatches between the user selection and server configuration by validating the sudo mode before command execution. Task: 4403
Use sudo() on server_ids for users in group_user to ensure read access to server.use_sudo (restricted to group_manager), preventing AccessError Task: 4403
Move the sudo mode check from the wizard model to execute_command. This change ensures that a mismatched sudo parameter versus the server configuration triggers a ValidationError. Add a new test and update the test test_execute_command_with_variables to reflect this behavior. Task: 4403
WalkthroughThis pull request updates the handling of sudo privileges across the server command execution, tests, and wizard interfaces. The changes adjust how the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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: 0
🧹 Nitpick comments (1)
cetmix_tower_server/wizards/cx_tower_command_execute_wizard.py (1)
47-51: Field type change improves user experienceThe field
use_sudohas been changed from a Selection to a Boolean field, which aligns with the PR objective of replacing the sudo selector with a checkbox. This simplifies the user interface and makes it clearer that sudo usage depends on server settings.However, there's a minor issue with the help text formatting - missing punctuation between sentences.
- "If no sudo is configured will run without sudo", + "If no sudo is configured, will run without sudo",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cetmix_tower_server/models/cx_tower_server.py(1 hunks)cetmix_tower_server/tests/test_command.py(1 hunks)cetmix_tower_server/tests/test_server.py(1 hunks)cetmix_tower_server/wizards/cx_tower_command_execute_wizard.py(3 hunks)cetmix_tower_server/wizards/cx_tower_command_execute_wizard_view.xml(1 hunks)
🔇 Additional comments (6)
cetmix_tower_server/tests/test_command.py (1)
691-691: Good syncing of server sudo mode with test parameter.This update ensures that the server's
use_sudosetting matches thesudoparameter being tested in each loop iteration, which properly accommodates the new validation in theexecute_commandmethod.cetmix_tower_server/wizards/cx_tower_command_execute_wizard_view.xml (1)
55-57: Improved UI clarity with conditional sudo label.The new span element adds a clear "sudo" label that is conditionally displayed only when SSH commands are selected, making the purpose of the checkbox more intuitive for users.
cetmix_tower_server/models/cx_tower_server.py (1)
733-739: Good validation to prevent sudo mode mismatches.This validation ensures consistency between the command execution's sudo parameter and the server's configured sudo mode, preventing potential issues that could arise from mismatched sudo configurations.
cetmix_tower_server/tests/test_server.py (1)
765-783: Well-structured test for sudo mode validation.This test correctly verifies the new validation check by setting up a server with
use_sudo: "p"and attempting to execute a command with a mismatched sudo parameter"n". The assertion properly checks for the expected validation error message.cetmix_tower_server/wizards/cx_tower_command_execute_wizard.py (2)
193-193: Improved sudo handling logicThis change correctly implements the PR objective of ensuring that sudo mode is determined by server settings rather than user selection. When the checkbox is checked, it will use the server's configured sudo setting; otherwise, it passes
None.
274-274: Consistent sudo handling across methodsThe same sudo handling logic is correctly applied here for command execution via SSH, maintaining consistency with the implementation in the
execute_command_on_servermethod.
Replace 'sudo' argument with boolean in execute_command() to simplify the code and reduce error risk. Remove the "sudo mode mismatch" constraint and its test. Unify wizard visibility conditions. Task: 4403
| @@ -730,6 +730,9 @@ def execute_command( | |||
| ) # pylint: disable=no-member | |||
| ) | |||
|
|
|||
| if sudo: | |||
| sudo = self.sudo().use_sudo | |||
|
|
|||
| # Populate `sudo` value from the server settings if not provided explicitly | |||
| if sudo is None: | |||
There was a problem hiding this comment.
Is this check still needed? As well as the one in the line 738? Cannot be they simply combined?
| @@ -191,7 +190,7 @@ def execute_command_on_server(self): | |||
| for server in self.server_ids: | |||
| server.execute_command( | |||
| self.command_id, | |||
| sudo=self.use_sudo, | |||
| sudo=server.use_sudo and self.use_sudo, | |||
There was a problem hiding this comment.
Why do you need to check this is wizard if you check this in the execute_command anyway?
Replace the sudo selector with a checkbox in the command wizard to ensure the sudo mode is determined by the server settings. This change avoids mismatches between the user selection and server configuration by validating the sudo mode before command execution.
Task: 4403
Summary by CodeRabbit