USHIFT-6804: AI Skill: Automated Testing - Prow (Phase 2)#133
Open
agullon wants to merge 4 commits into
Open
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a Prow-focused release-testing feature: a new ChangesProw Release Testing Workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels: ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)plugins/microshift-release/skills/automated-testing/SKILL.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) ... [truncated 1108 characters] ... node:internal/modules/esm/resolve:271:11) Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
agullon
force-pushed
the
USHIFT-6804
branch
2 times, most recently
from
8b53bb6 to
5342bf0
Compare
Contributor
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
plugins/microshift-release/scripts/unit_tests/test_prow.py (1)
47-49: ⚡ Quick winRemove redundant test.
test_rejects_bare_minoris redundant withtest_rejects_invalid_format(lines 42-45): both test the same input"4.21"expecting aValueError. One test already validates the error message; the duplicate adds no value.♻️ Proposed fix
- def test_rejects_bare_minor(self): - with self.assertRaises(ValueError): - prow.parse_version("4.21") -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/microshift-release/scripts/unit_tests/test_prow.py` around lines 47 - 49, Remove the redundant unit test function test_rejects_bare_minor which duplicates test_rejects_invalid_format; delete the entire test_rejects_bare_minor function so only test_rejects_invalid_format remains to assert prow.parse_version("4.21") raises ValueError (and preserves its existing message assertion).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/microshift-release/scripts/lib/prow.py`:
- Around line 108-117: Create a centralized helper (e.g., get_gh_runner or
run_gh_command) that first resolves the "gh" executable with shutil.which and
raises a clear error if not found, then calls subprocess.run with that resolved
path, sets capture_output=True, text=True, and enforces a sensible timeout
(e.g., timeout=30). Replace direct subprocess.run invocations that call "gh"
(the call building ["gh", "pr", "list", ...] and the other occurrences around
the current pr-list and other gh uses) to use this helper so every GH invocation
uses the resolved executable and a timeout; preserve passing GH_REPO and other
args through to the helper and propagate subprocess.CalledProcessError or return
subprocess.CompletedProcess as before. Ensure the helper is imported/defined
near other utility functions in prow.py so all gh subprocess uses (including the
blocks around the existing subprocess.run calls) are centralized.
In `@plugins/microshift-release/scripts/prow_testing.py`:
- Around line 145-185: The subprocess.run invocations (e.g., the git
checkout/commit/push and gh pr create calls involving variables like
head_branch, repo_dir, v, push_result, pr_result) must be hardened the same way
as in prow.py: resolve executables with shutil.which (use resolved paths for
"git" and "gh") and pass an explicit timeout (and check=True where appropriate)
to avoid indefinite hangs and satisfy Ruff S603/S607; update these calls in
prow_testing.py and the other noted regions (around lines 248-255, 333-353,
484-487, 574-577) to use the resolved command paths and a sensible timeout
value, preserving current arguments and capturing output/text as before.
- Around line 496-501: The output dictionary currently always sets "status":
"completed" regardless of job outcomes; change the logic that builds output (the
variable named output near the end of prow_testing.py and the code that
populates results) to inspect results for any failed gsutil copy (e.g., a
non-zero return code or an explicit failure flag in each job entry) and set
"status" to "failed" (or "partial" per your convention) if any job failed,
otherwise "completed"; update both the block producing output at lines ~496-501
and the similar block at ~508-513 so download_dir and jobs remain the same but
status reflects the aggregated success/failure of results.
- Around line 145-154: The git checkout/commit subprocess.run calls in main()
(the ones using head_branch, v['branch'], v['version'], and repo_dir) must be
wrapped in try/except that catches subprocess.CalledProcessError and emits the
same structured JSON error used elsewhere in this script (preserve exit code and
stderr) instead of letting a traceback print; update both the checkout block and
the commit block (and the similar calls around lines 626-633) to catch
CalledProcessError and call the existing JSON error-emission routine (or the
same logic used elsewhere for structured errors) before exiting.
In `@plugins/microshift-release/scripts/prow_testing.sh`:
- Line 1: Replace the current shebang in
plugins/microshift-release/scripts/prow_testing.sh from #!/usr/bin/env bash to
the repository-required #!/usr/bin/bash and ensure the script enables strict
mode by adding set -euo pipefail near the top (immediately after the shebang) so
the script follows the CONTRIBUTING.md shell script standard.
In `@plugins/microshift-release/skills/automated-testing/SKILL.md`:
- Around line 89-93: Update Step 5 of SKILL.md (the description around running
`bash ${SCRIPTS_DIR}/prow_testing.sh merge-status <version>`) to explicitly
document the `"closed"` outcome as a stop condition: state that if the JSON
returns `"status": "closed"` (closed without merge) the workflow should display
the message and stop the workflow (same behavior as `"open"`), and include this
edge case as a clear safety/stop condition per SKILL-GUIDELINES.md.
- Around line 23-25: Update the example invocation in SKILL.md to include the
required <action> positional argument for the CLI; change the shown command that
references prow_testing.sh to pass <action> before <version> (e.g.,
prow_testing.sh <action> <version>) so the documentation matches the script's
expected arguments and users aren't misled into omitting the action parameter.
---
Nitpick comments:
In `@plugins/microshift-release/scripts/unit_tests/test_prow.py`:
- Around line 47-49: Remove the redundant unit test function
test_rejects_bare_minor which duplicates test_rejects_invalid_format; delete the
entire test_rejects_bare_minor function so only test_rejects_invalid_format
remains to assert prow.parse_version("4.21") raises ValueError (and preserves
its existing message assertion).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b30973c4-3bde-42cf-a1f0-45b48f1f84b8
📒 Files selected for processing (8)
plugins/microshift-release/.claude-plugin/plugin.jsonplugins/microshift-release/README.mdplugins/microshift-release/scripts/lib/prow.pyplugins/microshift-release/scripts/prow_testing.pyplugins/microshift-release/scripts/prow_testing.shplugins/microshift-release/scripts/run_unit_tests.shplugins/microshift-release/scripts/unit_tests/test_prow.pyplugins/microshift-release/skills/automated-testing/SKILL.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/microshift-release/scripts/lib/prow.py (1)
222-226: 💤 Low valueDead exception handlers around
_http_getcalls.
_http_getalready catchesrequests.RequestExceptioninternally and returnsNoneon failure (line 166). Thetry/except RequestExceptionblocks here will never catch anything since exceptions are already handled.The
Nonechecks at lines 228 and 242-247 correctly handle failures, so these try/except blocks are just dead code.♻️ Simplify by removing dead exception handlers
- try: - resp = _http_get(f"{base}/latest-build.txt") - except requests.RequestException as e: - logger.warning("Failed to fetch latest-build.txt for %s: %s", full_job_name, e) - return {"job": full_job_name, "status": "ERROR", "url": None, "build_id": None} + resp = _http_get(f"{base}/latest-build.txt") if resp is None or resp.status_code != 200: return {"job": full_job_name, "status": "ERROR", "url": None, "build_id": None}- try: - finished_resp = _http_get(f"{base}/{build_id}/finished.json") - except requests.RequestException: - return {"job": full_job_name, "status": "PENDING", "url": prow_url, "build_id": build_id} + finished_resp = _http_get(f"{base}/{build_id}/finished.json")Also applies to: 237-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/microshift-release/scripts/lib/prow.py` around lines 222 - 226, The try/except blocks around the _http_get calls are dead because _http_get already catches requests.RequestException and returns None; remove the outer try/except that wraps _http_get(f"{base}/latest-build.txt") and the similar handler around the other _http_get call, and rely on the existing None checks (e.g., the subsequent "if resp is None" logic) to handle failures; update any logger messages to the existing None-path behavior rather than catching exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/microshift-release/scripts/lib/prow.py`:
- Around line 222-226: The try/except blocks around the _http_get calls are dead
because _http_get already catches requests.RequestException and returns None;
remove the outer try/except that wraps _http_get(f"{base}/latest-build.txt") and
the similar handler around the other _http_get call, and rely on the existing
None checks (e.g., the subsequent "if resp is None" logic) to handle failures;
update any logger messages to the existing None-path behavior rather than
catching exceptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 87b9ae2c-ab2f-452a-b902-88e266362b28
📒 Files selected for processing (2)
plugins/microshift-release/scripts/lib/prow.pyplugins/microshift-release/scripts/prow_testing.py
Add lib/prow.py with utilities for the release testing workflow: - Version parsing and validation (4.21+ only, rejects Jenkins-era versions) - PR lookup via gh CLI (open and any-state searches) - GCS job listing and short-name matching for CI jobs - Parallel build status fetching with ThreadPoolExecutor - GitHub check status cross-referencing to detect stale GCS results - HTTP GET with retry logic Includes unit tests covering version parsing, job selection by minor version, GCS name matching (including false-positive prevention), and the GitHub status override behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Add prow_testing.py with 7 commands for the release testing lifecycle: - create-pr: Create a draft PR with an empty commit - trigger: Post /test comments to retrigger failed jobs - status: Display CI job statuses as a formatted table - merge: Undraft PR and post merge labels (no /lgtm — requires human) - merge-status: Check if Tide has merged the PR - download: Download artifacts from GCS (plan/execute pattern) - upload: Compress artifacts to tar.gz and upload to S3 All mutating commands use a dry-run/--execute pattern. Output is structured JSON for programmatic consumption by the Claude skill. Includes prow_testing.sh venv wrapper that manages Python dependencies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Add SKILL.md defining the 7-step workflow that Claude Code follows when running /microshift-release:automated-testing <version>. Update plugin.json version to 1.2.0 and README roadmap to reflect the new skill name (automated-testing, replacing prow-testing). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
plugins/microshift-release/skills/automated-testing/SKILL.md (2)
23-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix manual invocation to include the required action argument.
Line 24 currently shows
prow_testing.sh <version>, but manual mode requires an action first; this example will fail or mislead users.Suggested doc fix
-bash plugins/microshift-release/scripts/prow_testing.sh <version> +bash plugins/microshift-release/scripts/prow_testing.sh <action> <version> [--execute]As per coding guidelines, CONTRIBUTING.md says to “ensure the user-facing workflow docs (e.g., SKILL.md steps and prerequisites) match actual script behavior.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/microshift-release/skills/automated-testing/SKILL.md` around lines 23 - 25, The example invocation in SKILL.md is missing the required action argument for the prow_testing.sh script; update the example line that currently shows "bash plugins/microshift-release/scripts/prow_testing.sh <version>" to include the action parameter (e.g., "manual" or the appropriate action your script expects) so it correctly demonstrates calling prow_testing.sh with both the action and the version arguments (refer to the script name prow_testing.sh and the example line in SKILL.md).
89-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument
closedas an explicit merge-status stop condition.Step 5 omits the
closedoutcome (PR closed without merge). Add it as a stop condition so operators don’t continue to artifact steps incorrectly.Suggested doc fix
- If `"status": "merged"` — display the message and continue to Step 6. - If `"status": "open"` — display the message and stop the workflow (waiting for human `/lgtm` and Tide merge). +- If `"status": "closed"` — display the message and stop the workflow (PR was closed without merge).As per coding guidelines, plugins/docs/SKILL-GUIDELINES.md requires documenting “relevant safety/edge cases” with “clear stop/error conditions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/microshift-release/skills/automated-testing/SKILL.md` around lines 89 - 93, Step 5's merge-status handling omits the `"closed"` outcome; update the documentation for the command `bash ${SCRIPTS_DIR}/prow_testing.sh merge-status <version>` to parse the JSON and treat `"status": "closed"` as an explicit stop condition (same behavior as `"open"`) so operators do not proceed to artifact steps; modify the SKILL.md text where the three cases are listed to add a bullet explaining that `"status": "closed"` should display a clear message and halt the workflow until human intervention.
🧹 Nitpick comments (1)
plugins/microshift-release/scripts/prow_testing.py (1)
587-590: ⚡ Quick winDerive public URL from
prow.S3_BUCKETto avoid silent breakage.The replacement hardcodes
"s3://release-testing-results"and the region. Ifprow.S3_BUCKETchanges,replace()will silently produce an incorrect URL (the original s3:// URI unchanged or partially mangled).Proposed fix
- public_url = s3_dest.replace( - "s3://release-testing-results", - "https://release-testing-results.s3.us-west-2.amazonaws.com", - ) + # Parse bucket name from S3_BUCKET constant + bucket_name = prow.S3_BUCKET.replace("s3://", "") + s3_region = "us-west-2" # or add S3_REGION constant to prow module + public_url = s3_dest.replace( + prow.S3_BUCKET, + f"https://{bucket_name}.s3.{s3_region}.amazonaws.com", + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/microshift-release/scripts/prow_testing.py` around lines 587 - 590, The current public_url computation hardcodes "s3://release-testing-results" and the region, which breaks if prow.S3_BUCKET changes; update the logic that sets public_url (derived from s3_dest) to base the replacement on prow.S3_BUCKET instead of the hardcoded string and construct the HTTPS host from that bucket value (e.g., use prow.S3_BUCKET to determine the bucket name and build the https://{bucket}.s3.{region}.amazonaws.com form or a safer transformation of s3:// to https://), ensuring s3_dest is left unchanged on mismatch and avoiding silent incorrect URLs; update references in this block (variables public_url, s3_dest and prow.S3_BUCKET) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/microshift-release/scripts/lib/prow.py`:
- Around line 41-46: ci_jobs currently maps any minor >21 to _CI_JOBS_422;
change it to explicitly accept only supported minors and raise on others: in
ci_jobs(minor) parse minor_num as now, then if minor_num == 21 return
_CI_JOBS_421, elif minor_num == 22 return _CI_JOBS_422, else raise a ValueError
(or custom exception) with a clear message about unsupported minor; update/ add
unit tests to assert behavior for 21, 22 and that e.g. 23 raises.
In `@plugins/microshift-release/scripts/prow_testing.py`:
- Around line 59-60: Guard against empty statuses before calling max: replace
the current unconditional max() on the generator with either a check (if
statuses: col_job = max(len(s["short_name"]) for s in statuses) else: col_job =
len("Job")) or use max(..., default=0) and then ensure col_job = max(col_job,
len("Job")). Target the existing code that sets col_job (the two lines
referencing col_job and statuses) so it won't call max on an empty iterable.
---
Duplicate comments:
In `@plugins/microshift-release/skills/automated-testing/SKILL.md`:
- Around line 23-25: The example invocation in SKILL.md is missing the required
action argument for the prow_testing.sh script; update the example line that
currently shows "bash plugins/microshift-release/scripts/prow_testing.sh
<version>" to include the action parameter (e.g., "manual" or the appropriate
action your script expects) so it correctly demonstrates calling prow_testing.sh
with both the action and the version arguments (refer to the script name
prow_testing.sh and the example line in SKILL.md).
- Around line 89-93: Step 5's merge-status handling omits the `"closed"`
outcome; update the documentation for the command `bash
${SCRIPTS_DIR}/prow_testing.sh merge-status <version>` to parse the JSON and
treat `"status": "closed"` as an explicit stop condition (same behavior as
`"open"`) so operators do not proceed to artifact steps; modify the SKILL.md
text where the three cases are listed to add a bullet explaining that `"status":
"closed"` should display a clear message and halt the workflow until human
intervention.
---
Nitpick comments:
In `@plugins/microshift-release/scripts/prow_testing.py`:
- Around line 587-590: The current public_url computation hardcodes
"s3://release-testing-results" and the region, which breaks if prow.S3_BUCKET
changes; update the logic that sets public_url (derived from s3_dest) to base
the replacement on prow.S3_BUCKET instead of the hardcoded string and construct
the HTTPS host from that bucket value (e.g., use prow.S3_BUCKET to determine the
bucket name and build the https://{bucket}.s3.{region}.amazonaws.com form or a
safer transformation of s3:// to https://), ensuring s3_dest is left unchanged
on mismatch and avoiding silent incorrect URLs; update references in this block
(variables public_url, s3_dest and prow.S3_BUCKET) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 880f8926-8cee-49b0-99f2-c9038f8f5e0c
📒 Files selected for processing (8)
plugins/microshift-release/.claude-plugin/plugin.jsonplugins/microshift-release/README.mdplugins/microshift-release/scripts/lib/prow.pyplugins/microshift-release/scripts/prow_testing.pyplugins/microshift-release/scripts/prow_testing.shplugins/microshift-release/scripts/run_unit_tests.shplugins/microshift-release/scripts/unit_tests/test_prow.pyplugins/microshift-release/skills/automated-testing/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- plugins/microshift-release/scripts/prow_testing.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/microshift-release/README.md
- plugins/microshift-release/.claude-plugin/plugin.json
- plugins/microshift-release/scripts/run_unit_tests.sh
- plugins/microshift-release/scripts/unit_tests/test_prow.py
- Fix manual script invocation example to include <action> argument - Document closed merge-status outcome as a stop condition Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the Phase 2 (Prow CI) release testing skill for MicroShift 4.21+. This replaces the manual workflow of creating PRs, triggering jobs, checking statuses, downloading artifacts, and uploading them to S3.
The skill runs all steps sequentially, asking for confirmation before each mutating action and skipping steps that are already complete. It exposes 7 commands via
prow_testing.py, all consumable by/microshift-release:automated-testing <version>:create-prtrigger/testcomments to retrigger failed/not-started jobsstatusmerge/lgtm(requires human)merge-statusdownloaduploadAll mutating commands use a dry-run/
--executepattern. Output is structured JSON for programmatic consumption.Commit breakdown
lib/prow.pywith GCS job listing, parallel status fetching, version parsing + unit testsprow_testing.pywith the 7 commands +prow_testing.shvenv wrapperSKILL.mddefining the 7-step workflow + plugin version bump to 1.2.0Example output
Test it with [release-4.22] Release Testing 4.22.0-rc.3 PR and results pushed into S3 directory