Implement wait and download action #4
Conversation
Signed-off-by: Kostiantyn Kostiuk <[email protected]>
Signed-off-by: Kostiantyn Kostiuk <[email protected]>
Signed-off-by: Kostiantyn Kostiuk <[email protected]>
Signed-off-by: Kostiantyn Kostiuk <[email protected]>
Summary of ChangesHello @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! This pull request significantly enhances the hardware submission automation tool by introducing a new capability to monitor and download the results of a submission. It also modernizes the command-line interface by switching to Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![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 refactors the script to use argparse for command-line argument parsing and introduces a new wait_download action. The move to argparse is a good improvement. However, I've found a critical issue where the new action is unusable due to an inconsistent name in the parser configuration versus the application logic. I've also identified several areas for improvement regarding code robustness, maintainability, and adherence to best practices, such as using argparse subparsers for action-specific arguments and making string-based checks less brittle. My detailed comments provide specific suggestions to address these points.
| # Validate required arguments for submit action | ||
| if not args.product_name: | ||
| print("Error: --product_name is required for submit action") | ||
| sys.exit(1) | ||
| if not args.guest_names: | ||
| print("Error: --guest_names is required for submit action") | ||
| sys.exit(1) | ||
| if not args.package_path: | ||
| print("Error: --package_path is required for submit action") | ||
| sys.exit(1) |
There was a problem hiding this comment.
The manual validation of required arguments for the 'submit' action (and similarly in 'main_wait_download') is functional but can be improved. Using argparse subparsers is the idiomatic way to handle different sets of arguments for different actions. Each subparser can define its own required arguments, which simplifies the main functions, removes code duplication, and leverages argparse to provide standard, helpful error messages to the user. Consider refactoring to use subparsers for better maintainability.
| wrapper.download_results(args.product_id, args.submission_id, output_file) | ||
| print(f"Results downloaded to: {output_file}") | ||
|
|
||
| if "> driverMetadata Url" in results: |
There was a problem hiding this comment.
Relying on the string "> driverMetadata Url" from the stdout of an external tool is brittle. Any change in the tool's output formatting (e.g., spacing, capitalization) could break this logic silently. A more robust solution would be to have wait_for_submission return a status, or to call another method after it completes to programmatically check for the availability of the metadata URL from a structured response rather than parsing plain text.
kostyanf14
force-pushed
the
RHELMISC-15073
branch
from
3df194b to
f6d0de6
Compare
|
when applying this patch, there is a warning message: warning: 1 line adds whitespace errors. |
|
Test with the following commands: submit：-- pass python3 hw_submission_automation.py -n 'Red Hat VirtIO Block Drivers for Windows Server 2016' -g 16 -p /home/signing-windows-machine-configs/scripts/signed/290_BLK_Win2016_unsigned.hlkx -d 2025-12-15wait_download : -- Error - Access to the path '/home/debug' is denied. python3 hw_submission_automation.py wait_download --product_id 14079040174508234 --submission_id 1152921505699872126 --output_file /home/debugSDCM output:
|
| def main_submit(args): | ||
| # Validate required arguments for submit action | ||
| if not args.product_name: | ||
| print("Error: --product_name is required for submit action") |
There was a problem hiding this comment.
Consider using argparse.ArgumentParser.error() for consistent error reporting.
There was a problem hiding this comment.
This will be complicated because "product_name" is not a mandatory option for argparse, because it is required only for the commit action. So, from the argparse point of view, there is no error.
Signed-off-by: Kostiantyn Kostiuk <[email protected]>
Signed-off-by: Kostiantyn Kostiuk <[email protected]>
You need to provide a path to the ZIP file. Similar to PS: Do you think we need to change this behaviour in our wrapper? |
kostyanf14
force-pushed
the
RHELMISC-15073
branch
from
f6d0de6 to
715827d
Compare
No description provided.