-
Notifications
You must be signed in to change notification settings - Fork 17
RHELMISC-15852: Add support file upload/download after pre/post test commands #725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…commands Signed-off-by: Kostiantyn Kostiuk <[email protected]>
Signed-off-by: Kostiantyn Kostiuk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @kostyanf14, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented a new feature that allows for file upload and download operations to be performed on guest and host machines as part of pre and post test commands. This significantly enhances the flexibility of test configurations, enabling automated transfer of necessary files for test setup or collection of results after test execution.
Highlights
- Enhanced Test Configuration: Introduced "files_action" as a new option within "pre_test_commands" and "post_test_commands", allowing specification of file transfer operations.
- Flexible File Actions: Added support for both "remote-to-local" (download) and "local-to-remote" (upload) file transfers, with options to copy or move files, and to handle missing files gracefully.
- Dynamic Path Replacements: Implemented path replacements like "@workspace@", "@safe_test_name@", and "@client_name@" for "local_path" and "remote_path", providing dynamic and context-aware file targeting.
- New Documentation: A new markdown file ("docs/Test-config-behavior.md") has been added to clearly document the new test configuration options, file action behaviors, and execution order.
- Example Usage: Demonstrated the new functionality by adding an example to "lib/engines/hcktest/drivers/NetKVM.json" that creates a file on the guest and then downloads it to the host workspace.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable feature for file upload/download as part of pre/post test commands. The implementation is well-structured, with new models for file actions and clear logic for handling them. The addition of documentation is also a great step.
I've identified a few areas for improvement. There's a critical security vulnerability related to file deletion that needs to be addressed. I've also found a high-severity bug due to a type mismatch that could cause runtime errors. Additionally, there are a couple of medium-severity issues regarding documentation accuracy and configuration consistency. My detailed comments and suggestions aim to resolve these issues and enhance the robustness and security of the new functionality.
end | ||
|
||
@tools.upload_to_machine(client_name, local_path, files_action.remote_path) | ||
FileUtils.rm_rf(local_path) if files_action.move |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using FileUtils.rm_rf
with a user-configurable path (local_path
) is a significant security risk. A misconfigured path could lead to unintentional deletion of important files or directories on the host system. To mitigate this, you should validate that the local_path
is within the project's workspace before performing the delete operation when move
is true.
Note: The suggestion uses Pathname
, which may require require 'pathname'
at the top of the file if it's not already available.
if files_action.move
workspace_realpath = Pathname.new(@project.workspace_path).realpath.to_s
local_realpath = Pathname.new(local_path).realpath.to_s
unless local_realpath.start_with?(workspace_realpath)
raise EngineError, "Cannot move file/directory outside of workspace: #{local_path}"
end
FileUtils.rm_rf(local_path)
end
lib/engines/hcktest/tests.rb
Outdated
replacement.merge!({ '@client_name@' => @client.name }) | ||
updated_action = files_action.dup_and_replace_path(replacement, DEFAULT_FILE_ACTION_REMOTE_PATH, | ||
DEFAULT_FILE_ACTION_LOCAL_PATH) | ||
run_file_action_on_client(@client, updated_action) | ||
|
||
next if @support.nil? | ||
|
||
replacement.merge!({ '@client_name@' => @support.name }) | ||
updated_action = files_action.dup_and_replace_path(replacement, DEFAULT_FILE_ACTION_REMOTE_PATH, | ||
DEFAULT_FILE_ACTION_LOCAL_PATH) | ||
run_file_action_on_client(@support, updated_action) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two issues in this block:
- A type error:
run_file_action_on_client
expects a client name (String) as its first argument, but it's being passed the client object (@client
and@support
). This will likely cause a runtime error. - The
replacement
map is being mutated withmerge!
. While it might not cause a bug in the current context, it's better practice to avoid mutating arguments and instead create a new map for each client to improve code clarity and prevent potential side effects.
The suggested change corrects the type error by passing @client.name
and @support.name
, and uses merge
to create new, non-mutated replacement maps.
client_replacement = replacement.merge({ '@client_name@' => @client.name })
updated_action = files_action.dup_and_replace_path(client_replacement, DEFAULT_FILE_ACTION_REMOTE_PATH,
DEFAULT_FILE_ACTION_LOCAL_PATH)
run_file_action_on_client(@client.name, updated_action)
next if @support.nil?
support_replacement = replacement.merge({ '@client_name@' => @support.name })
updated_action = files_action.dup_and_replace_path(support_replacement, DEFAULT_FILE_ACTION_REMOTE_PATH,
DEFAULT_FILE_ACTION_LOCAL_PATH)
run_file_action_on_client(@support.name, updated_action)
### File Action Configuration Options | ||
|
||
- **remote_path** - Path to file or directory on Client VM | ||
- **local_path** - Path to file or directory on Client VM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description for local_path
is incorrect. It states that it's a path on the Client VM, but the implementation shows it's a path on the host machine where AutoHCK is running. This could lead to confusion when configuring file actions. Please update the documentation for clarity.
- **local_path** - Path to file or directory on Client VM | |
- **local_path** - Path to file or directory on the host running AutoHCK |
"guest_run": "New-Item -ItemType File -Path 'C:\\SupportDevice2\\test.txt' -Value (Get-Date -Format 'yyyy-MM-dd HH:mm:ss')", | ||
"files_action": [ | ||
{ | ||
"remote_path": "C:/SupportDevice2/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other paths in this file and the default remote path defined in the code (C:\\
), it's better to use backslashes (\\
) as path separators for Windows paths in this JSON configuration. While PowerShell often handles forward slashes, being consistent reduces potential ambiguity.
"remote_path": "C:\\SupportDevice2\\",
sig do | ||
params(files_actions: T::Array[Models::FileActionConfig], replacement: ReplacementMap).void | ||
end | ||
def run_file_actions(files_actions, replacement) |
Check notice
Code scanning / Rubocop
A calculated magnitude based on number of assignments, branches, and conditions. Note test
No description provided.