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 into a modular command-based architecture inspired by ramalama to improve maintainability and facilitate adding new commands and debugging

Enhancements:

Refactor qmctl to separate command definitions from execution logic for clearer organization
Adopt a command-based design that simplifies adding new subcommands and debugging
Align code structure with ramalama-inspired architecture for better readability and maintainability

Chores:

Update .gitignore to reflect new project structure

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

Reviewer's Guide

This MR refactors the monolithic qmctl tool into a clean, command-based architecture inspired by the ramalama project, modularizing command handlers and streamlining the main dispatch logic for easier extension and debugging.

File-Level Changes

Change	Details	Files
Introduce a Command abstraction and registry	Define a Command interface with Execute and Help methods Create a central registry (map) of command names to handlers Move dispatch logic out of a switch into a generic lookup/execute flow	`tools/qmctl/qmctl`
Modularize individual commands into separate handlers	Extract each subcommand (e.g., create, delete, list) into its own handler struct Register handlers in init or setup method Remove inline command logic from the main file	`tools/qmctl/qmctl`
Centralize flag parsing and help output	Unify flag definitions for all commands Implement a generic help command that lists available subcommands Ensure each handler provides its own usage text	`tools/qmctl/qmctl`
Standardize error handling and logging	Return error objects from Execute and handle centrally in main Log errors with consistent formatting and exit codes Remove ad-hoc print statements spread across command code	`tools/qmctl/qmctl`
Update ignore rules for new build artifacts	Add qmctl binary to .gitignore Ignore temporary files generated during build	`.gitignore`

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

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

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

Added the licence Thank you

tools/qmctl/qmctl

engelmi

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

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]>

dougsland · 2025-06-30T20:36:14Z

@ArtiomDivak ready for review?

ArtiomDivak · 2025-06-30T20:42:54Z

Yes its ready for review

engelmi

LGTM, great work!

engelmi · 2025-07-09T05:57:26Z

@sourcery-ai review

engelmi · 2025-07-09T05:57:44Z

@dougsland PR lgtm, PTAL as well

sourcery-ai

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

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

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]>

dougsland · 2025-07-10T20:07:11Z

@ArtiomDivak @engelmi I have few comments that can be addressed later, for example.

we need a --help that show example of real commands. Example, cp needs "qm:"
don't raise errors to users, some don't understand the ValidationError output, better add some text only message.

./qmctl cp l a
Traceback (most recent call last):
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1057, in <module>
    main()
    ~~~~^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1053, in main
    run_command_with_error_handling(args, controller, parser)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 762, in run_command_with_error_handling
    args.func(args, controller)
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1038, in handle_cp_command
    controller.copy_in_container(
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        paths=args.paths, output_json=args.json, pretty=True
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 564, in copy_in_container
    src, dst = self._validate_paths_for_cp(paths)
               ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 172, in _validate_paths_for_cp
    raise ValidationError(
        f"Provide `{self.container}:` only in source or destination"
    )
ValidationError: Provide `qm:` only in source or destination

 ./qmctl exec
Traceback (most recent call last):
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1057, in <module>
    main()
    ~~~~^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1053, in main
    run_command_with_error_handling(args, controller, parser)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 762, in run_command_with_error_handling
    args.func(args, controller)
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1014, in handle_exec_command
    controller.exec_in_container(
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        command=args.cmd, output_json=args.json, pretty=True
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 534, in exec_in_container
    self._validate_command_provided(
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        command,
        ^^^^^^^^
        "No command provided to execute in the container."
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 160, in _validate_command_provided
    raise ValidationError(f"No command provided{context_msg}.")
ValidationError: No command provided No command provided to execute in the container..

 ./qmctl execin
Traceback (most recent call last):
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1057, in <module>
    main()
    ~~~~^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1053, in main
    run_command_with_error_handling(args, controller, parser)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 762, in run_command_with_error_handling
    args.func(args, controller)
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1026, in handle_execin_command
    controller.execin_in_container(
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        command=args.cmd, output_json=args.json, pretty=True
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 594, in execin_in_container
    self._validate_exec_command(command)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 180, in _validate_exec_command
    self._validate_command_provided(command, "to execute in container")
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 160, in _validate_command_provided
    raise ValidationError(f"No command provided{context_msg}.")
ValidationError: No command provided to execute in container.

# ./qmctl execin ls
Traceback (most recent call last):
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1057, in <module>
    main()
    ~~~~^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1053, in main
    run_command_with_error_handling(args, controller, parser)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 762, in run_command_with_error_handling
    args.func(args, controller)
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 1026, in handle_execin_command
    controller.execin_in_container(
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        command=args.cmd, output_json=args.json, pretty=True
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 594, in execin_in_container
    self._validate_exec_command(command)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^
  File "/root/artiom/qm/tools/qmctl/./qmctl", line 183, in _validate_exec_command
    raise ValidationError("No command provided to execute.")

dougsland · 2025-07-10T20:08:51Z

@ArtiomDivak great stuff, I have created:
#866
#865

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

ArtiomDivak self-assigned this Jun 10, 2025

ArtiomDivak requested review from Yarboa, aesteve-rh, dougsland, kleinffm, nsednev, pengshanyu and rhatdan 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 kleinffm, nsednev, pengshanyu and rhatdan 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 8f0b6ba to 654f8c2 Compare June 17, 2025 09:24

engelmi requested changes Jun 19, 2025

View reviewed changes

tools/qmctl/qmctl Outdated Show resolved Hide resolved

tools/qmctl/qmctl Outdated Show resolved Hide resolved

tools/qmctl/qmctl Outdated Show resolved Hide resolved

tools/qmctl/qmctl Outdated Show resolved Hide resolved

tools/qmctl/qmctl Outdated Show resolved Hide resolved

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 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 3 times, most recently from d7c14b2 to 6e2ac8a Compare June 29, 2025 08:39

ArtiomDivak requested review from Yarboa, dougsland and engelmi June 29, 2025 08:43

ArtiomDivak force-pushed the issue-838 branch from 6e2ac8a to 0de6c6e Compare June 29, 2025 09:12

ArtiomDivak added 2 commits June 30, 2025 22:16

Added QMCTL class

19f5fef

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 0de6c6e to 76cbf4e Compare June 30, 2025 19:16

engelmi approved these changes Jul 4, 2025

View reviewed changes

sourcery-ai bot approved these changes Jul 9, 2025

View reviewed changes

tools/qmctl/qmctl Outdated Show resolved Hide resolved

tools/qmctl/qmctl Outdated Show resolved Hide resolved

tools/qmctl/qmctl Outdated Show resolved Hide resolved

Added the core of QMCTL

a6d05af

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 1db918f to a6d05af Compare July 9, 2025 06:15

Merge branch 'main' into issue-838

8f34c77

dougsland merged commit 85a8640 into containers:main Jul 10, 2025
16 checks passed

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

Restructure qmctl #839

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!

engelmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dougsland Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

ArtiomDivak Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

engelmi left a comment

Choose a reason for hiding this comment

Uh oh!

dougsland commented Jun 30, 2025

Uh oh!

ArtiomDivak commented Jun 30, 2025

Uh oh!

engelmi left a comment

Choose a reason for hiding this comment

Uh oh!

engelmi commented Jul 9, 2025

Uh oh!

engelmi commented Jul 9, 2025

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dougsland commented Jul 10, 2025

Uh oh!

dougsland commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ArtiomDivak commented Jun 10, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jun 10, 2025 •

edited

Loading

dougsland commented Jul 10, 2025 •

edited

Loading