add script to generate prow status for dev jobs - #67
Open
weshayutin wants to merge 1 commit into
Open
Conversation
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: weshayutin 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 |
📝 WalkthroughWalkthroughAdds a standalone script that discovers Medik8s CI configurations, generates presubmit job records, retrieves results from GCS concurrently, and writes ChangesCI status generation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd script to generate durable CI status data for dev Prow jobs
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. Wrong GCS header lookup
|
![qodo-2-for-medik8s[bot]](https://avatars.githubusercontent.com/in/4026794?s=60&v=4){kind=small}
Comment on lines
+142
to
+145
| link = next( | ||
| (v for k, v in headers.items() if k.lower() == "x-goog-meta-x-goog-meta-link"), | ||
| None, | ||
| ) |
There was a problem hiding this comment.
1. Wrong gcs header lookup 🐞 Bug ≡ Correctness
fetch_status() documents reading the x-goog-meta-link header but the code looks up x-goog-meta-x-goog-meta-link, so it will typically fail to find the link and return state=unknown instead of reading finished.json. This undermines the script’s primary purpose by producing mostly unknown statuses.
Agent Prompt
### Issue description
`fetch_status()` expects to retrieve a GCS metadata header named `x-goog-meta-link`, but the implementation searches for `x-goog-meta-x-goog-meta-link`. This mismatch prevents resolving the `gs://...` link to the build directory, leading to `state: unknown` and skipping `finished.json`.
### Issue Context
The docstring explicitly describes the header as `x-goog-meta-link`, and the code’s fallback path returns `unknown` when the lookup fails.
### Fix Focus Areas
- scripts/ci-status/generate_data.py[128-147]
### Suggested fix
- Change the header comparison to `k.lower() == "x-goog-meta-link"`.
- Optionally support both keys (try `x-goog-meta-link` first, then `x-goog-meta-x-goog-meta-link`) to be resilient if the upstream metadata key differs.
- Consider including the available header keys in the `unknown` state (or stderr log) to simplify debugging.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+106
to
+111
| def _get_text(url): | ||
| try: | ||
| with urllib.request.urlopen(url, timeout=15) as resp: | ||
| return resp.read().decode() | ||
| except urllib.error.HTTPError: | ||
| return None |
There was a problem hiding this comment.
2. Http errors misclassified 🐞 Bug ☼ Reliability
_get_text()/_head() convert all HTTPError responses into None, which fetch_status() then treats as normal states like no-history or pending. This will silently misreport statuses on non-404 HTTP failures (e.g., 403/429/5xx) instead of surfacing an error state.
Agent Prompt
### Issue description
The helpers `_get_text()` and `_head()` catch any `urllib.error.HTTPError` and return `None`. Callers treat `None` as meaningful absence ("no-history" / "pending"), which is only valid for 404-like "not found" situations. Non-404 HTTP errors (rate limiting, permission issues, transient 5xx) should not be silently reinterpreted as normal job states.
### Issue Context
- `_get_text()` returns `None` for any `HTTPError`.
- `fetch_status()` maps `latest-build.txt`/HEAD failures to `state: no-history` and `finished.json` failure to `state: pending`.
### Fix Focus Areas
- scripts/ci-status/generate_data.py[106-112]
- scripts/ci-status/generate_data.py[119-126]
- scripts/ci-status/generate_data.py[134-152]
### Suggested fix
- In `_get_text()` / `_head()`, catch `HTTPError as e` and:
- return `None` only when `e.code == 404` (and possibly 410),
- otherwise re-raise so `fetch_status()` returns `state: error` with details, or explicitly return a structured error object.
- Optionally capture and include `e.code` and a short body snippet in the returned error to improve debuggability.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+166
to
+167
| with ThreadPoolExecutor(max_workers=workers) as ex: | ||
| futs = {ex.submit(fetch_status, j): j for j in jobs} |
There was a problem hiding this comment.
3. Workers arg can crash 🐞 Bug ☼ Reliability
--workers accepts zero/negative values and is passed directly to ThreadPoolExecutor(max_workers=workers), which raises ValueError and terminates with a traceback. This makes the script fragile in automation if the argument is mis-set.
Agent Prompt
### Issue description
`ThreadPoolExecutor` requires `max_workers >= 1`, but the CLI allows `--workers 0` or negative values, which will crash the script.
### Issue Context
`--workers` is parsed as an int and passed unvalidated into `ThreadPoolExecutor(max_workers=workers)`.
### Fix Focus Areas
- scripts/ci-status/generate_data.py[164-170]
- scripts/ci-status/generate_data.py[173-182]
### Suggested fix
- Validate after parsing and call `parser.error("--workers must be >= 1")` if invalid, or
- Use a custom `type=` function in argparse that enforces positivity.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/ci-status/generate_data.py`:
- Around line 49-51: Update FNAME_RE to accept operator release branch names
such as release-0.10 by matching the known dirname prefix and capturing the
branch token through the __ delimiter, while preserving main and existing
release branch matches and the repo, variant, and YAML filename structure.
- Around line 180-181: Validate args.workers immediately after
parser.parse_args() in the argument-parsing flow, and call parser.error when the
value is less than 1 so only positive worker counts reach ThreadPoolExecutor.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e9b1332e-4a1f-4db6-8d39-24014fcd1668
📒 Files selected for processing (1)
scripts/ci-status/generate_data.py
Comment on lines
+49
to
+51
| FNAME_RE = re.compile( | ||
| r"^medik8s-(?P<repo>.+)-(?P<branch>main|release-[0-9]+\.[0-9]+)__(?P<variant>[0-9]+\.[0-9]+)\.yaml$" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Accept operator release branch names.
FNAME_RE accepts only main and release-<OCP version>. It rejects operator branches such as release-0.10. Current Prow history includes pull-ci-medik8s-node-healthcheck-operator-release-0.10-4.16-openshift-e2e. (deck-ci.apps.ci.l2s4.p1.openshiftapps.com)
Lines 83-86 then skip these configurations. The generated matrix omits valid E2E jobs. Match the known dirname prefix and capture the branch token up to __.
🤖 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 `@scripts/ci-status/generate_data.py` around lines 49 - 51, Update FNAME_RE to
accept operator release branch names such as release-0.10 by matching the known
dirname prefix and capturing the branch token through the __ delimiter, while
preserving main and existing release branch matches and the repo, variant, and
YAML filename structure.
Comment on lines
+180
to
+181
| parser.add_argument("--workers", type=int, default=16, help="concurrent GCS status lookups (default: 16)") | ||
| args = parser.parse_args() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from concurrent.futures import ThreadPoolExecutor
try:
ThreadPoolExecutor(max_workers=0)
except ValueError:
print("confirmed: non-positive max_workers is invalid")
else:
raise SystemExit("expected ValueError for max_workers=0")
PYRepository: medik8s/system-tests
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant parser/workers setup and any existing validation/usages.
sed -n '150,210p' scripts/ci-status/generate_data.py
printf '\n--- occurrences of workers/max_workers ---\n'
rg -n "\b(workers|max_workers)\b|ThreadPoolExecutor" scripts/ci-status/generate_data.pyRepository: medik8s/system-tests
Length of output: 2721
Reject non-positive worker counts.
--workers 0 and negative values parse successfully, but ThreadPoolExecutor(max_workers=...) raises ValueError for non-positive values. Validate args.workers after parsing and call parser.error for values below 1.
🤖 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 `@scripts/ci-status/generate_data.py` around lines 180 - 181, Validate
args.workers immediately after parser.parse_args() in the argument-parsing flow,
and call parser.error when the value is less than 1 so only positive worker
counts reach ThreadPoolExecutor.
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.
update the wiki page for the status of prow jobs in dev e2e
Summary by CodeRabbit