Restructure qmctl #839

ArtiomDivak · 2025-06-10T06:30:02Z

This MR refactors the qmctl file to improve its structure, making it easier to add new commands and debug in the future. The new design is heavily inspired by the clean, command-based architecture of the "ramalama" project, aiming for a more organized and maintainable codebase.

closes #838

Summary by Sourcery

Restructure qmctl to improve code organization and maintainability by adopting a command-based architecture

Enhancements:

Refactor qmctl into a clean, command-based architecture inspired by the 'ramalama' project
Modularize command handling to simplify addition of new commands and enhance debuggability

sourcery-ai · 2025-06-10T06:30:06Z

Reviewer's Guide

Refactored qmctl into a clean, command-based architecture by introducing a command registry, extracting handlers into separate functions, and unifying argument parsing and error reporting.

File-Level Changes

Change	Details	Files
Introduce command registration model for subcommands	Add command registry data structure Implement registerCommand function with metadata Replace manual subcommand dispatch with dynamic lookup	`tools/qmctl/qmctl`
Extract individual command handlers into separate functions	Move subcommand logic out of main into dedicated handler functions Update dispatch flow to call new handlers Isolate command-specific logic and error pathways	`tools/qmctl/qmctl`
Unify argument parsing and error reporting across commands	Centralize flag definitions and parsing logic Standardize error output format Implement consistent usage messages	`tools/qmctl/qmctl`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @ArtiomDivak - I've reviewed your changes - here's some feedback:

Blocking issues:

Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)

General comments:

Factor out the repeated container-exists and argument-validation logic into a decorator or helper to DRY up the multiple exec_* and show_* methods.
Replace the scattered sys.exit(1) calls by raising custom exceptions in command handlers and handling them centrally in the CLI entrypoint to simplify control flow.
Consider breaking each command into its own module or class rather than growing a single monolithic QMCTL class to better align with the intended clean, command-based architecture.

Here's what I looked at during the review

🟡 General issues: 1 issue found
🔴 Security: 3 blocking issues
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

tools/qmctl/qmctl_class.py

tools/qmctl/qmctl

tools/qmctl/qmctl_class.py

sourcery-ai

Hey @ArtiomDivak - I've reviewed your changes and they look great!

Blocking issues:

Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)

Here's what I looked at during the review

🟢 General issues: all looks good
🔴 Security: 4 blocking issues
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

tools/qmctl/qmctl

Yarboa

@ArtiomDivak Did you try to run the tests through tmt locally?
Please refer that one all tier-0 testing are failing due to the changes

qm/tests/e2e/README.md

Line 139 in daec692

    
           Refer [FMF](https://fmf.readthedocs.io/en/stable) for tmt test metadata specification

sourcery-ai

Hey @ArtiomDivak - I've reviewed your changes and they look great!

Blocking issues:

Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)

Prompt for AI Agents

Please address the comments from this code review:
## Security Issues

### Issue 1
<location> `tools/qmctl/qmctl:62` </location>

<issue_to_address>
**security (dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

*Source: opengrep*
</issue_to_address>

### Issue 2
<location> `tools/qmctl/qmctl:215` </location>

<issue_to_address>
**security (dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

*Source: opengrep*
</issue_to_address>

### Issue 3
<location> `tools/qmctl/qmctl:537` </location>

<issue_to_address>
**security (dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

*Source: opengrep*
</issue_to_address>

### Issue 4
<location> `tools/qmctl/qmctl:613` </location>

<issue_to_address>
**security (dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

*Source: opengrep*
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

tools/qmctl/qmctl

engelmi

Added a few comments.
This PR is quite big, so I am not sure if it would be better to split it.

tools/qmctl/qmctl

engelmi · 2025-06-19T05:54:13Z

tools/qmctl/qmctl

+        """Initialize the QMCTL class.
+
+        Args:
+            config_path (str): The path to the container config file.


Type hints in the comments, e.g. the (str) for config_path, are nice, but it would be more important to add type hints on your actual variables. In addition to readability, this also adds tooling support for IDEs such as auto-complete or suggesting operations.
For example, you can add types to your function params:

def __init__( self, config_path: str = "/usr/share/containers/systemd/qm.container", verbose: bool = False, container_name: str = "qm", ):

and the same to your variables:

self.config_path: str = config_path self.container: str = container_name self.verbose: bool = verbose

The big advantage here is that your IDE can now suggest operations. For example when using self.config_path your IDE assumes its a string and can suggest operations such as lower().

@ArtiomDivak Could you add type hints?

Added hints

I only see type hints added to that specific code part I used as an example. I meant adding those for the whole code, every function parameter etc. Its also ok to not have type hints, but having a mix is off.
Reopening this thread.

tools/qmctl/qmctl

dougsland · 2025-06-20T13:17:57Z

tools/qmctl/qmctl

@@ -1,256 +1,448 @@
 #!/usr/bin/env python
-# Licensed under the Apache License, Version 2.0


please keep the license

dougsland · 2025-06-20T13:20:57Z

tools/qmctl/qmctl

-        if not command:
-            self._print_output({"error": "No command provided to execute."}, output_json, pretty)
-            sys.exit(1)
+    def _check_exit_if_string_in_string(self, string, substring, message):


It's just one if, don't think we need a new function.

dougsland · 2025-06-20T13:21:47Z

tools/qmctl/qmctl

+            self._print_output_and_exit(
+                f"{msg} No output returned.")
+
+    def _check_if_command_is_not_None(self, command, msg):


It's just a if, I don't think we need this function.

This MR I added QMCTL class wich will be responsiable to run all the command function and return the result. Also added all the imports needed to all the file Signed-off-by: Artiom Divak <[email protected]>

ArgumentParserWithDefaults class automatically adds default values to the help text of arguments SubcommandInitializer is a generic class to initialize subparsers for command-line applications Signed-off-by: Artiom Divak <[email protected]>

engelmi

Please remove the __pycache__ (and maybe add it to .gitignore)

This commits adds the main function the init of the subcommand and the handle function for the subcommand with other vital function. Signed-off-by: Artiom Divak <[email protected]>

ArtiomDivak requested a review from engelmi June 10, 2025 06:30

ArtiomDivak self-assigned this Jun 10, 2025

ArtiomDivak requested review from rhatdan, dougsland, Yarboa, nsednev, aesteve-rh, pengshanyu and kleinffm as code owners June 10, 2025 06:30

sourcery-ai bot approved these changes Jun 10, 2025

View reviewed changes

ArtiomDivak force-pushed the issue-838 branch from 75c4665 to 1b4cccc Compare June 10, 2025 06:45

ArtiomDivak removed request for rhatdan, kleinffm, nsednev and pengshanyu June 11, 2025 07:02

ArtiomDivak marked this pull request as draft June 11, 2025 07:05

ArtiomDivak force-pushed the issue-838 branch 6 times, most recently from d60705a to 355789e Compare June 12, 2025 07:12

ArtiomDivak marked this pull request as ready for review June 12, 2025 07:13

sourcery-ai bot approved these changes Jun 12, 2025

View reviewed changes

ArtiomDivak removed the request for review from aesteve-rh June 12, 2025 07:44

Yarboa reviewed Jun 15, 2025

View reviewed changes

ArtiomDivak force-pushed the issue-838 branch 2 times, most recently from 9d4c8c3 to 11e2cf0 Compare June 16, 2025 07:24

ArtiomDivak marked this pull request as draft June 16, 2025 08:04

ArtiomDivak force-pushed the issue-838 branch 2 times, most recently from 13020b1 to 60695cb Compare June 16, 2025 08:12

ArtiomDivak marked this pull request as ready for review June 16, 2025 08:26

sourcery-ai bot requested changes Jun 16, 2025

View reviewed changes

ArtiomDivak force-pushed the issue-838 branch 2 times, most recently from 8f0b6ba to 654f8c2 Compare June 17, 2025 09:24

engelmi requested changes Jun 19, 2025

View reviewed changes

ArtiomDivak force-pushed the issue-838 branch 2 times, most recently from 6976bb6 to 86fe6e3 Compare June 19, 2025 14:08

dougsland reviewed Jun 20, 2025

View reviewed changes

ArtiomDivak added 2 commits June 22, 2025 08:50

Added QMCTL class

db1cdcb

This MR I added QMCTL class wich will be responsiable to run all the command function and return the result. Also added all the imports needed to all the file Signed-off-by: Artiom Divak <[email protected]>

ArtiomDivak force-pushed the issue-838 branch from 86fe6e3 to df5de41 Compare June 22, 2025 05:51

engelmi reviewed Jun 23, 2025

View reviewed changes

ArtiomDivak force-pushed the issue-838 branch from df5de41 to b15c8c5 Compare June 24, 2025 08:43

Added the core of QMCTL

d7c14b2

This commits adds the main function the init of the subcommand and the handle function for the subcommand with other vital function. Signed-off-by: Artiom Divak <[email protected]>

ArtiomDivak force-pushed the issue-838 branch from b15c8c5 to d7c14b2 Compare June 25, 2025 06:09

		@@ -1,256 +1,448 @@
		#!/usr/bin/env python
		# Licensed under the Apache License, Version 2.0

Restructure qmctl #839

Are you sure you want to change the base?

Restructure qmctl #839

Uh oh!

Conversation

ArtiomDivak commented Jun 10, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yarboa left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

engelmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

engelmi Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

ArtiomDivak Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

engelmi Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dougsland Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

dougsland Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

dougsland Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

engelmi left a comment

Choose a reason for hiding this comment

ArtiomDivak commented Jun 10, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jun 10, 2025 •

edited

Loading