-
Notifications
You must be signed in to change notification settings - Fork 22
feat: added TPS RHNQA automation script #373
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: main
Are you sure you want to change the base?
feat: added TPS RHNQA automation script #373
Conversation
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 new Python script to automate the TPS RHNQA workflow, which is a significant improvement over the manual process. The script integrates with Errata Tool and Beaker, handling job submission, monitoring, and result uploading. The addition of beaker-client to pyproject.toml is appropriate for this new functionality. Overall, the script provides a valuable automation solution.
However, there are several areas for improvement, particularly regarding error handling, configuration management, and argument parsing. The current implementation uses direct sys.argv access and hardcoded values for critical settings like dry-run flags and URLs, which reduces flexibility and maintainability. Additionally, the SSL verification default behavior and generic exception handling could be improved for robustness and security.
I've provided specific comments on these points to help enhance the script's reliability, configurability, and adherence to best practices.
| try: | ||
| user = hub.auth.who_am_i() | ||
| print(f"authenticated as: {user['username']}") | ||
| except Exception as e: |
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.
run_tps_rhnqa.py
Outdated
| print(f"\nStep 8:uploading results to Errata Tool") | ||
|
|
||
| #Set to True to test without actually uploading | ||
| UPLOAD_DRY_RUN = True |
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.
run_tps_rhnqa.py
Outdated
| def ET_verify() -> bool | str: | ||
| verify = os.getenv("REDHAT_IT_CA_BUNDLE") | ||
| if verify: | ||
| return verify | ||
| else: | ||
| return False |
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 ET_verify function defaults to False for SSL verification if REDHAT_IT_CA_BUNDLE is not set. Disabling SSL verification by default can expose the application to security vulnerabilities like Man-in-the-Middle attacks. It's safer to either:
- Default to
True(meaningrequestswill use its default CA bundle). - Raise an error if the bundle is expected but not found, forcing explicit configuration.
If REDHAT_IT_CA_BUNDLE is truly optional, ensure the security implications of verify=False are understood and accepted.
| def ET_verify() -> bool | str: | |
| verify = os.getenv("REDHAT_IT_CA_BUNDLE") | |
| if verify: | |
| return verify | |
| else: | |
| return False | |
| @cache | |
| def ET_verify() -> bool | str: | |
| verify = os.getenv("REDHAT_IT_CA_BUNDLE") | |
| if verify: | |
| return verify | |
| # Default to True for secure connections if no custom bundle is provided | |
| return True |
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.
To run locally I was setting the CA bundle to False otherwise getting a SSL cert error
| except Exception as e: | ||
| print(f"job submission failed. Error: {e}") | ||
|
|
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.
Catching a generic Exception can hide underlying issues and make debugging difficult. It's better to catch more specific exceptions (e.g., bkr.client.errors.BeakerError or requests.exceptions.RequestException if the hub.jobs.upload could raise network errors) and handle them appropriately. This allows for more precise error reporting and recovery.
| except Exception as e: | |
| print(f"job submission failed. Error: {e}") | |
| except bkr.client.errors.BeakerError as e: | |
| print(f"job submission failed. Beaker error: {e}") | |
| except Exception as e: | |
| print(f"job submission failed. Unexpected error: {e}") |
| #workaround for nonexisting ET status Error | ||
| if errata_status == 'ERROR': | ||
| errata_status = 'BAD' |
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.
This 'workaround for nonexisting ET status Error' indicates a potential mismatch between Beaker's result states and Errata Tool's expected statuses. While the workaround addresses the immediate problem, it might be worth investigating if a more direct mapping or a new status in Errata Tool could be introduced to accurately reflect 'Error' states from Beaker.
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.
https://gitlab.cee.redhat.com/baseos-qe/tps-results/-/blob/master/tps-results?ref_type=heads#L848
did the same thing as tps-results
| if len(sys.argv) < 2: | ||
| sys.exit(1) |
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 sys.argv directly for command-line arguments is less robust than using a dedicated argument parsing library like argparse. argparse provides features like help messages, type checking, and default values, making the script more user-friendly and maintainable.
| if len(sys.argv) < 2: | |
| sys.exit(1) | |
| import argparse | |
| parser = argparse.ArgumentParser(description="Automate TPS RHNQA workflow.") | |
| parser.add_argument("errata_id", type=str, help="Errata ID to process.") | |
| parser.add_argument("--dry-run-submit", action="store_true", help="Perform a dry run for job submission to Beaker.") | |
| parser.add_argument("--dry-run-upload", action="store_true", help="Perform a dry run for uploading results to Errata Tool.") | |
| args = parser.parse_args() | |
| errata_id = args.errata_id |
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.
Great suggestion! I'll look into this
| ERRATA_URL = "https://errata.devel.redhat.com" | ||
| ERRATA_XMLRPC = "https://errata.devel.redhat.com/errata/errata_service" | ||
| ERRATA_TPS_XMLRPC = "https://errata.devel.redhat.com/tps/tps_service" | ||
| BEAKER_URL = "https://beaker.engineering.redhat.com" | ||
| CHECK_INSTALL_TASK = "/distribution/check-install" | ||
| SETUP_TASK = "/distribution/errata/setup" | ||
| TPS_TASK = "/distribution/errata/tps" | ||
| CLEANUP_TASK = "/distribution/errata/cleanup" |
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.
These URLs and task paths are currently hardcoded. While they might be stable for the current environment, it's generally better to make such configurations external (e.g., environment variables, a dedicated config file, or command-line arguments). This allows for easier adaptation to different environments (e.g., staging, production, local development) without modifying the code.
Consider using os.getenv() for URLs or argparse for command-line options.
35406f5 to
4ec5a1d
Compare
this script replaces the manual workflow of: bkr workflow-tomorrow --errata <id> --tps-rhnqa bkr job-watch J:<job_id> tps-results --errata=<id> --job=<job_id> --rhnqa - fetches errata info and TPS jobs from Errata Tool API - generates Beaker XML matching workflow-tomorrow format (check-install, errata/setup, errata/tps, errata/cleanup tasks) - gubmits jobs to Beaker and monitors completion - parses Beaker job results from XML - uploads results to Errata Tool this implementation avoids the dependency on qa-tools-workstation package (workflow-tomorrow and tps-results) by using public beaker-client library(PyPI) for XML generation and job submission,calling Errata Tool REST/XML-RPC APIs directly, and reverse engineering behavior from workflow-tomorrow and tps-results
6efa56b to
8238fa0
Compare
this script replaces the manual workflow of:
bkr workflow-tomorrow --errata --tps-rhnqa
bkr job-watch J:<job_id>
tps-results --errata= --job=<job_id> --rhnqa
this implementation avoids the dependency on qa-tools-workstation package (workflow-tomorrow and tps-results) by using public beaker-client library(PyPI) for XML generation and job submission,calling Errata Tool REST/XML-RPC APIs directly, and reverse engineering behavior from workflow-tomorrow and tps-results.
resolves https://github.com/packit/jotnar/issues/266