Redesign command execution - #1037
Conversation
Generalize all command execution and move to one manager. Engines now use command execution manager instead of manual processing each command type. Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a unified CommandExecutionManager to centralize command execution logic across HCK tests, post-start commands, and Functest JSON steps, replacing the custom TestStep class with the unified Models::CommandInfo class. The feedback highlights a critical backward-compatibility issue where making the desc field required in CommandInfo will break existing Functest JSON test cases that omit it. It is recommended to make desc nilable, update command_desc to handle nil values gracefully, and raise AutoHCKError in a more idiomatic Ruby style.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR centralizes command/step execution across engines by introducing a CommandExecutionManager that runs guest/host commands, file actions, reboots, and QMP interactions, and updates HCK test and functest flows to route execution through that manager.
Changes:
- Added
AutoHCK::CommandExecutionManagerand wired it into HCK client post-start commands, HCK test hooks, and functest step execution. - Unified functest step modeling to reuse
Models::CommandInfo(including QMP configs) rather than maintaining a separateTestStepstruct. - Updated setup/engine code paths to retain client instances and expose replacement maps/command managers from the appropriate layer.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/setupmanagers/qemuhck/qemuhck.rb | Stores created clients and prefers their replacement maps when available. |
| lib/setupmanagers/hckclient.rb | Routes post-start command execution through CommandExecutionManager. |
| lib/setupmanagers/functest_client.rb | Adds a client-scoped CommandExecutionManager with functest reboot strategy. |
| lib/models/command_info.rb | Expands CommandInfo into a unified step/command descriptor (incl. QMP configs). |
| lib/engines/hcktest/tools.rb | Adds restart_machine_and_wait API stub for tooling compatibility. |
| lib/engines/hcktest/tests.rb | Replaces bespoke command/file-action execution with CommandExecutionManager. |
| lib/engines/hcktest/hcktest.rb | Runs post-start host commands via CommandExecutionManager and exposes it. |
| lib/engines/functest/test_executor.rb | Passes CommandExecutionManager into step execution flow. |
| lib/engines/functest/test_context.rb | Exposes replacement_map for step/manager replacement integration. |
| lib/engines/functest/test_case.rb | Switches functest step arrays to Models::CommandInfo. |
| lib/engines/functest/step_handler.rb | Wraps CommandExecutionManager#execute with functest timeout/error/output logic. |
| lib/auxiliary/command_execution_manager.rb | New centralized execution implementation for commands/steps across engines. |
| lib/all.rb | Autoloads CommandExecutionManager. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def step_replacement(step) | ||
| return @context.replacement_map if step.variables.empty? | ||
|
|
||
| def run_guest_command(step, command, desc = nil) | ||
| desc ||= command[0..60] | ||
| command = @context.substitute_variables(command, step.variables) | ||
| extra = step.variables.each_with_object({}) do |(placeholder, var_name), hash| | ||
| value = @context.substitute_variables("@#{var_name}@") | ||
| hash[placeholder] = value unless value.empty? | ||
| end | ||
|
|
||
| @context.replacement_map.merge(extra) | ||
| end |
| sig do | ||
| params( | ||
| command_info: Models::CommandInfo, | ||
| replacement: ReplacementMap | ||
| ).returns(T::Hash[Symbol, T.untyped]) | ||
| end | ||
| # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity | ||
| def execute(command_info, replacement: ReplacementMap.new) | ||
| desc = command_desc(command_info) | ||
| result = T.let({ desc: desc }, T::Hash[Symbol, T.untyped]) | ||
|
|
||
| result[:guest_outputs] = execute_guest(command_info, replacement, desc) if guest_command?(command_info) | ||
| execute_guest_reboot(desc) if command_info.guest_reboot | ||
| execute_host(command_info, replacement, desc) if host_command?(command_info) | ||
| execute_files_actions(command_info, replacement) unless command_info.files_action.empty? | ||
| execute_barrier(command_info) if command_info.barrier | ||
| result[:qmp_result] = execute_qmp_command(command_info) if command_info.qmp_command | ||
| result[:qmp_event] = execute_qmp_wait_event(command_info) if command_info.qmp_wait_event | ||
|
|
||
| result | ||
| end |
Run basic functinal tests to check second main engine. Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Generalize all command execution and move to one manager. Engines now use command execution manager instead of manual processing each command type.