Query dev-preview stream in release controller for EC releases#3197
Query dev-preview stream in release controller for EC releases#3197joepvd wants to merge 1 commit into
Conversation
The release controller query only checked {major}-stable, but EC releases
(e.g. 5.0.0-ec.5) are promoted to {major}-dev-preview. This caused
gen-assembly to crash for 5.0 EC assemblies because 5-stable returns
{"tags": null}, and data.get('tags', []) doesn't guard against explicit
null values.
Fix both issues:
- Use `data.get('tags') or []` to handle null tags without crashing
- Query both {major}-stable and {major}-dev-preview streams, unioning
the results
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Walkthrough
ChangesRelease controller versions
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant get_release_controller_versions_async
participant ReleaseController
Caller->>get_release_controller_versions_async: request major.minor versions
get_release_controller_versions_async->>ReleaseController: GET stable tags
ReleaseController-->>get_release_controller_versions_async: stable tag data
get_release_controller_versions_async->>ReleaseController: GET dev-preview tags
ReleaseController-->>get_release_controller_versions_async: dev-preview tag data
get_release_controller_versions_async-->>Caller: merged sorted versions
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@artcommon/artcommonlib/ocp_version_ancestry.py`:
- Around line 376-385: Update the release-controller response parsing around the
tags/version extraction loop to validate each stream’s decoded payload before
accessing it: handle invalid JSON, non-object payloads, non-list tags, and
malformed tag entries by returning [] for that stream. Keep HTTP errors handled
as currently, and ensure parsing/shape failures are contained so the second
stream is still queried.
🪄 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: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9e1ed082-7968-412c-956e-35083cc387dc
📒 Files selected for processing (2)
artcommon/artcommonlib/ocp_version_ancestry.pyartcommon/tests/test_ocp_version_ancestry.py
| tags = data.get('tags') or [] | ||
|
|
||
| # Filter tags to only those matching the requested major.minor. | ||
| # Tag names are version strings like "4.18.3" or "4.18.0-rc.0". | ||
| version_pattern = re.compile(rf'^{major}\.{minor}\.') | ||
| versions = [] | ||
| for tag in tags: | ||
| name = tag.get('name', '') | ||
| if version_pattern.match(name): | ||
| # Validate it's a proper semver string before including | ||
| try: | ||
| semver.VersionInfo.parse(name) | ||
| versions.append(name) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="artcommon/artcommonlib/ocp_version_ancestry.py"
echo "== Line count =="
wc -l "$FILE"
echo
echo "== Outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== Relevant section =="
sed -n '320,450p' "$FILE" | cat -nRepository: openshift-eng/art-tools
Length of output: 7535
Make release-controller parsing non-fatal. Invalid JSON, non-object payloads, non-list tags, or malformed tag entries can raise outside the httpx.HTTPError handler and stop the second stream from being queried. Validate the decoded payload per stream and return [] on parse/shape errors instead.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 377-377: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.compile(rf'^{major}.{minor}.')
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🤖 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 `@artcommon/artcommonlib/ocp_version_ancestry.py` around lines 376 - 385,
Update the release-controller response parsing around the tags/version
extraction loop to validate each stream’s decoded payload before accessing it:
handle invalid JSON, non-object payloads, non-list tags, and malformed tag
entries by returning [] for that stream. Keep HTTP errors handled as currently,
and ensure parsing/shape failures are contained so the second stream is still
queried.
|
@joepvd: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary
TypeError: 'NoneType' object is not iterablecrash inget_release_controller_versions_async()when the release controller returns{"tags": null}(e.g. for5-stablewhich has no releases yet){major}-stableand{major}-dev-previewrelease controller streams, since EC releases (e.g.5.0.0-ec.5) are promoted to dev-preview, not stable_fetch_release_controller_tags()helper to avoid duplicationTest plan
test_null_tags_returns_empty— verifies{"tags": null}returns[]test_dev_preview_versions_included— verifies EC releases from dev-preview are returnedtest_union_of_stable_and_dev_preview— verifies deduplication across streamsmake unit)🤖 Generated with Claude Code
Summary by CodeRabbit