USHIFT-7260: add advisory promotion skill (Phase 3) for Konflux bootc#201
Open
agullon wants to merge 8 commits into
Open
USHIFT-7260: add advisory promotion skill (Phase 3) for Konflux bootc#201agullon wants to merge 8 commits into
agullon wants to merge 8 commits into
Conversation
USHIFT-7260 Add advisory-promotion skill that validates Konflux bootc advisories for QE sign-off before shipping. Checks are atomic, per-variant (arch + RHEL version), and grouped by data source: Advisory YAML checks: - Image presence, repository, SHA per arch/RHEL variant - Advisory type (RHBA/RHSA for z-stream, RHEA for X.Y.0) - SHA distinctness across architectures Pyxis catalog checks (via GraphQL): - Stage and prod catalog presence - Assembly tag commit, build date, no-XY0-tag for z-streams Shipment MR checks: - YAML filename validation - NVR commit vs Brew RPM build commit - releaseNotes.type matches advisory type - MR approval status Supports 4.18+ versions. For 4.22.2+ also checks el10 bootc images. EC/RC prod catalog checks are skipped (not yet shipped). Repository names are version-aware (openshift4 vs openshift5). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
|
[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
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:
WalkthroughAdds a Phase 3 advisory-promotion validator with Pyxis, advisory, and shipment MR checks, plus a Bash wrapper, tests, and skill documentation. ChangesPhase 3 advisory promotion validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/microshift-release/scripts/lib/pyxis.py (1)
559-563: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStage→prod fallback drops
archandrhel.On a stage 403, the recursive retry reverts to the defaults (
arch="amd64",rhel=9), so an arm64/el10 query silently checks the wrong prod variant and can return a misleading result.🐛 Proposed fix
- return check_catalog_image(version, catalog="prod") + return check_catalog_image(version, catalog="prod", + arch=arch, rhel=rhel)🤖 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/pyxis.py` around lines 559 - 563, In the stage catalog 403 fallback within the check_catalog_image function, the recursive call to check_catalog_image is missing the arch and rhel parameters from the original function invocation. Update the recursive call that falls back to prod to explicitly pass the arch and rhel parameters that were provided to the current function call, so that the fallback checks the same variant rather than silently reverting to the default architecture and RHEL version.
🧹 Nitpick comments (3)
plugins/microshift-release/skills/advisory-promotion/SKILL.md (1)
59-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCo-locate error handling with the script invocation step.
Move the VPN/token/version failure policy directly under Step 2 (where the command is run) to keep execution guidance in one place and reduce instruction drift during tool use.
Based on learnings, SKILL.md files should keep “CRITICAL”/error-handling rules inline with the relevant step and tool invocation rather than in a separate top-level edge-case section.
🤖 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/advisory-promotion/SKILL.md` around lines 59 - 78, The error handling guidance in Step 4 should be moved to immediately follow Step 2 (the script invocation) rather than appearing as a separate section at the end. Restructure the document so that after the bash command example in Step 2, you add subsections covering the VPN errors, GITLAB_API_TOKEN requirement, and version constraint handling directly. Then move Step 3 (Display Output) to follow the error handling guidance. This keeps the execution context and potential failure modes together in one cohesive place rather than split across multiple steps.Source: Learnings
plugins/microshift-release/scripts/advisory_promotion.py (2)
61-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_EL10_BOOTC_MINis defined but unused; the same threshold is hardcoded here.Lines 34-35 declare
_EL10_BOOTC_MIN = (4, 22, 2), but_rhel_versionsre-encodes that threshold inline as(4, 22)/z >= 2. The two can drift independently. Either consume the constant or drop it.♻️ Use the named threshold
def _rhel_versions(version_info): """Return RHEL versions to check based on the MicroShift version.""" - minor = _minor_tuple(version_info["minor"]) - z = version_info["z"] - if minor > (4, 22) or (minor == (4, 22) and z >= 2): + minor = _minor_tuple(version_info["minor"]) + full = (minor[0], minor[1], version_info["z"]) + if full >= _EL10_BOOTC_MIN: return [9, 10] return [9]🤖 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/advisory_promotion.py` around lines 61 - 67, The _rhel_versions function has hardcoded the threshold values (4, 22) and z >= 2 that are already defined in the _EL10_BOOTC_MIN constant at the top of the file. Refactor the conditional logic in _rhel_versions to use the _EL10_BOOTC_MIN constant instead of duplicating these magic numbers. Extract the major and minor version components from _EL10_BOOTC_MIN and compare them against the version_info parameters to determine if RHEL versions 9 and 10 should be returned, eliminating the hardcoded threshold values.
471-510: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCatalog images are queried twice per variant.
The first executor (Lines 471-484) prefetches
pyxis.check_catalog_image_graphqlfor every(arch, rhel, env)intocatalog. Butcheck_in_catalog(Lines 501-504) callscheck_catalog_image_graphqlagain for the stage/prod presence checks instead of reusing the prefetched results. That doubles the GraphQL calls (~2 extra per variant). Only the tag/date/no-xy0 checks consume the prefetchedcatalog; the presence checks should too.Consider passing the prefetched result into
check_in_catalog(keeping the EC/RC skip guard) so each(arch, rhel, env)is fetched once.🤖 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/advisory_promotion.py` around lines 471 - 510, The issue is that check_in_catalog is making redundant GraphQL API calls by querying pyxis.check_catalog_image_graphql internally instead of reusing the prefetched catalog results already stored in the catalog dictionary from the first executor. To fix this, modify the two check_in_catalog function calls (for stage and prod) to pass the prefetched stage_cat and prod_cat variables as parameters to check_in_catalog, allowing it to reuse these cached results instead of making duplicate GraphQL queries. This ensures each catalog image is fetched only once per (arch, rhel, env) combination while preserving any EC/RC skip guard logic.
🤖 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/advisory_promotion.py`:
- Around line 552-587: The `format_text_short` function's docstring claims SKIPs
are hidden, but the results filtering does not exclude entries with status
"SKIP". When iterating through results to populate the by_section dictionary,
add a condition to skip any results where the status equals "SKIP" before
appending them to the section. This will ensure SKIP status checks are not
included in the output for EC/RC releases as the docstring indicates.
- Around line 345-347: The brew_type variable built at line 346 can be set to
"XY" when vtype equals "XY", but this value is then passed to get_build_info()
which only documents release_type as one of "Z"/"X"/"Y"/"RC"/"EC"/"nightly" in
its docstring. Either remove the "XY" logic from the conditional and use one of
the documented release types instead, or update the get_build_info() function's
docstring to include "XY" as a valid documented release_type value.
In `@plugins/microshift-release/scripts/lib/artifacts.py`:
- Around line 714-720: The `_yaml.safe_load()` call can return `None` or
non-dict types (list, string, etc.), but the line `spec = content.get("spec",
{})` is executed outside the try-except block without validation. If `safe_load`
returns a non-dict value, calling `.get()` on it will raise an unhandled
`AttributeError` instead of the intended `None` return. Move the `spec =
content.get("spec", {})` assignment inside the try block immediately after the
`_yaml.safe_load()` call, or add a type check to ensure `content` is a
dictionary before calling `.get()` on it.
In `@plugins/microshift-release/scripts/lib/pyxis.py`:
- Around line 681-686: The chained `.get()` calls in the repo_data and
images_data assignments do not handle the case where GraphQL returns `data:
null` values. When result.get("data", {}) encounters an actual null value in the
response, it returns None instead of the default empty dict, causing subsequent
.get() calls to raise AttributeError. Fix this by wrapping each level with `or
{}` to ensure all intermediate values are dictionaries before calling chained
.get() methods, such as changing `result.get("data", {})` to
`(result.get("data") or {})` and applying the same pattern throughout the
repo_data and images_data assignment chain.
- Around line 30-51: The `_BOOTC_REPO_TEMPLATE` dictionary currently hardcodes
`openshift4` in both "prod" and "stage" REST API paths, while the GraphQL
equivalent correctly uses `openshift{major}`. Replace the hardcoded `openshift4`
with the placeholder `openshift{major}` in both entries of
`_BOOTC_REPO_TEMPLATE`. Additionally, update the `_catalog_url()` function to
accept the major version as a parameter and properly format the template with
that major version before returning the URL, ensuring the REST path matches the
GraphQL path behavior.
In `@plugins/microshift-release/scripts/unit_tests/test_advisory_promotion.py`:
- Around line 200-210: The TestTagBuildDate class currently tests only the PASS
and WARN branches of check_tag_build_date but lacks a test for the FAIL branch
with an invalid or malformed timestamp. Add a new test method to
TestTagBuildDate that creates a tag with an invalid/malformed build date (e.g.,
a tag missing the timestamp component or with corrupted format) and verify that
check_tag_build_date returns a FAIL status with appropriate error messaging.
Additionally, add a test method to check_shipment_mr_approved that tests the
PASS branch (when the MR is actually approved), as currently only non-PASS cases
appear to be tested.
In `@plugins/microshift-release/skills/advisory-promotion/SKILL.md`:
- Around line 3-4: The argument-hint on the first line documents only
`[--verbose]` but the step instructions reference passing through a `--json`
flag. Update the argument-hint to include both supported flags as `<version>
[--json] [--verbose]`, and ensure all documentation sections including the
synopsis/arguments and command examples consistently reflect both flags as
available options throughout the document. This inconsistency appears in
multiple locations at lines 14, 39-45, 57, and 62 as well.
- Around line 123-163: The markdown output examples in the SKILL.md file contain
emoji status symbols (✅, ❌, ⏭️) that violate the repository's markdown standards
as defined in CONTRIBUTING.md, which requires professional, terse,
customer-centric markdown without emojis or filler. Replace all emoji symbols
throughout the code example blocks (the Short/default output section, the On
failure section, and the EC/RC section) with text-based equivalents such as
PASS/FAIL/SKIP or remove them entirely, ensuring the output examples remain
clear and professional while conforming to the markdown guidelines.
---
Outside diff comments:
In `@plugins/microshift-release/scripts/lib/pyxis.py`:
- Around line 559-563: In the stage catalog 403 fallback within the
check_catalog_image function, the recursive call to check_catalog_image is
missing the arch and rhel parameters from the original function invocation.
Update the recursive call that falls back to prod to explicitly pass the arch
and rhel parameters that were provided to the current function call, so that the
fallback checks the same variant rather than silently reverting to the default
architecture and RHEL version.
---
Nitpick comments:
In `@plugins/microshift-release/scripts/advisory_promotion.py`:
- Around line 61-67: The _rhel_versions function has hardcoded the threshold
values (4, 22) and z >= 2 that are already defined in the _EL10_BOOTC_MIN
constant at the top of the file. Refactor the conditional logic in
_rhel_versions to use the _EL10_BOOTC_MIN constant instead of duplicating these
magic numbers. Extract the major and minor version components from
_EL10_BOOTC_MIN and compare them against the version_info parameters to
determine if RHEL versions 9 and 10 should be returned, eliminating the
hardcoded threshold values.
- Around line 471-510: The issue is that check_in_catalog is making redundant
GraphQL API calls by querying pyxis.check_catalog_image_graphql internally
instead of reusing the prefetched catalog results already stored in the catalog
dictionary from the first executor. To fix this, modify the two check_in_catalog
function calls (for stage and prod) to pass the prefetched stage_cat and
prod_cat variables as parameters to check_in_catalog, allowing it to reuse these
cached results instead of making duplicate GraphQL queries. This ensures each
catalog image is fetched only once per (arch, rhel, env) combination while
preserving any EC/RC skip guard logic.
In `@plugins/microshift-release/skills/advisory-promotion/SKILL.md`:
- Around line 59-78: The error handling guidance in Step 4 should be moved to
immediately follow Step 2 (the script invocation) rather than appearing as a
separate section at the end. Restructure the document so that after the bash
command example in Step 2, you add subsections covering the VPN errors,
GITLAB_API_TOKEN requirement, and version constraint handling directly. Then
move Step 3 (Display Output) to follow the error handling guidance. This keeps
the execution context and potential failure modes together in one cohesive place
rather than split across multiple steps.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1468ca94-1e3d-46a5-be43-6bc3026275c2
📒 Files selected for processing (6)
plugins/microshift-release/scripts/advisory_promotion.pyplugins/microshift-release/scripts/advisory_promotion.shplugins/microshift-release/scripts/lib/artifacts.pyplugins/microshift-release/scripts/lib/pyxis.pyplugins/microshift-release/scripts/unit_tests/test_advisory_promotion.pyplugins/microshift-release/skills/advisory-promotion/SKILL.md
agullon
changed the title
- Fix stage→prod fallback dropping arch/rhel params in check_catalog_image
- Guard against yaml.safe_load returning None in fetch_advisory_details
- Add pagination to check_catalog_image_graphql (up to 5 pages, matching REST)
- Use `or {}` pattern for null-safe GraphQL response chaining
- Add missing 'catalog' key in exception-path fallback dict
- Use .get() instead of direct dict access for tag names (KeyError prevention)
- Guard against None mr_iid in check_shipment_mr_approved
- Eliminate double GraphQL queries by passing prefetched catalog results
- Align format_text_short/format_text_full parameter order
- Fix emoji display-width padding for FAIL detail lines
- Correct misleading "SKIPs hidden" docstring
- Deduplicate advisory YAML parsing (_fetch_advisory_data delegates to fetch_advisory_details)
- Unify _catalog_url to always use _BOOTC_REPO_TEMPLATE (remove redundant constants)
- Remove unused _EL10_BOOTC_MIN constant
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pre-commit.check-secrets: ENABLED
coderabbitai
Bot
added
the
ready-for-human-review
Contributor
Author
|
Addressed CodeRabbit review-body and summary findings:
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/microshift-release/scripts/lib/pyxis.py (2)
637-647: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not cap GraphQL pagination before the catalog is exhausted.
Because tag matching is client-side,
range(5)can falsely report “not found” when the image exists after the first 1,250 rows. Continue until a short page is returned, or add a server-side tag/architecture filter before keeping the cap.Proposed fix
page_size = 250 + page = 0 - for page in range(5): + while True: variables = { "id": repo_id, "page": page, @@ if len(images_data) < page_size: break + page += 1Also applies to: 715-716
🤖 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/pyxis.py` around lines 637 - 647, The tag lookup in pyxis.py is incorrectly capped by the fixed pagination loop in the tag search logic, which can stop before the catalog is fully scanned. Update the pagination in the version-matching flow that builds tag_patterns and iterates over pages to continue fetching GraphQL results until a short page is returned, or add a server-side tag/architecture filter before keeping any cap, so the “not found” result is based on an exhausted catalog rather than range(5).
530-531: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid matching GA assembly tags for EC/RC lookups.
For
5.0.0-ec.3, the strippedbase_versionalso enables matchingassembly.5.0.0, so a GA image can satisfy a prerelease validation before the exactv5.0.0-ec.3tag is found.Proposed fix
- tag = re.sub(r"-(ec|rc)\.\d+$", "", version) - assembly_pattern = re.compile(rf"\bassembly\.{re.escape(tag)}\b") + prerelease = re.search(r"-(ec|rc)\.\d+$", version) + tag_patterns = ( + [re.compile(rf"^v{re.escape(version)}$")] + if prerelease else + [re.compile(rf"\bassembly\.{re.escape(version)}\b")] + )- base_version = re.sub(r"-(ec|rc)\.\d+$", "", version) - tag_patterns = [ - re.compile(rf"\bassembly\.{re.escape(base_version)}\b"), - re.compile(rf"^v{re.escape(version)}$"), - ] + prerelease = re.search(r"-(ec|rc)\.\d+$", version) + tag_patterns = ( + [re.compile(rf"^v{re.escape(version)}$")] + if prerelease else + [re.compile(rf"\bassembly\.{re.escape(version)}\b")] + )Also applies to: 641-645
🤖 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/pyxis.py` around lines 530 - 531, The EC/RC lookup in pyxis.py is too broad because the stripped base version lets `assembly_pattern` match GA tags like `assembly.5.0.0` before the exact prerelease tag is checked. Update the tag-matching logic in the `base_version`/`assembly_pattern` flow so prerelease versions such as `5.0.0-ec.3` only validate against the exact `v5.0.0-ec.3`-style tag and do not fall back to matching the GA assembly tag. Apply the same tightening anywhere the same lookup logic is used, including the related code around the later matching block referenced by the comment.
🤖 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.
Outside diff comments:
In `@plugins/microshift-release/scripts/lib/pyxis.py`:
- Around line 637-647: The tag lookup in pyxis.py is incorrectly capped by the
fixed pagination loop in the tag search logic, which can stop before the catalog
is fully scanned. Update the pagination in the version-matching flow that builds
tag_patterns and iterates over pages to continue fetching GraphQL results until
a short page is returned, or add a server-side tag/architecture filter before
keeping any cap, so the “not found” result is based on an exhausted catalog
rather than range(5).
- Around line 530-531: The EC/RC lookup in pyxis.py is too broad because the
stripped base version lets `assembly_pattern` match GA tags like
`assembly.5.0.0` before the exact prerelease tag is checked. Update the
tag-matching logic in the `base_version`/`assembly_pattern` flow so prerelease
versions such as `5.0.0-ec.3` only validate against the exact
`v5.0.0-ec.3`-style tag and do not fall back to matching the GA assembly tag.
Apply the same tightening anywhere the same lookup logic is used, including the
related code around the later matching block referenced by the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b3667c1d-e974-4a6f-831c-8640749bb404
📒 Files selected for processing (4)
plugins/microshift-release/scripts/advisory_promotion.pyplugins/microshift-release/scripts/lib/artifacts.pyplugins/microshift-release/scripts/lib/pyxis.pyplugins/microshift-release/scripts/unit_tests/test_advisory_promotion.py
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/microshift-release/scripts/unit_tests/test_advisory_promotion.py
- plugins/microshift-release/scripts/lib/artifacts.py
- plugins/microshift-release/scripts/advisory_promotion.py
- Log tracebacks on unexpected exceptions in ThreadPoolExecutor futures - Bump advisory YAML fetch/parse logging from debug to warning level Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
Contributor
|
@agullon couple of suggestions:
|
- Stage Pyxis at catalog.stage.redhat.com is behind preprod lockdown - Switch to pyxis.stage.engineering.redhat.com/v1 (requires kinit) - Add requests-gssapi for Kerberos/SPNEGO authentication - Disable SSL verify for stage (internal CA) - Stage has no GraphQL — delegate to REST for stage catalog checks - Remove stage→prod fallback on 403 (always check stage directly) - Fix trailing space in SKIP emoji 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/microshift-release/skills/advisory-promotion/SKILL.md (1)
31-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify whether VPN and
GITLAB_API_TOKENare hard prerequisites or WARN-only inputs.The table marks both as mandatory, but the later steps/notes say missing values only downgrade those checks to WARN. That makes the prerequisites section ambiguous about whether the run should fail fast or continue with partial validation.
Per CONTRIBUTING.md: documentation should describe concrete invocation and expected failures clearly.
♻️ Proposed fix
-| VPN | GitLab API, Brew RPM lookup | Yes — shipment and NVR checks WARN without it | -| `GITLAB_API_TOKEN` | Shipment MR fetch, MR approval check | Yes — shipment checks WARN without it | +| VPN | GitLab API, Brew RPM lookup | Required for full validation | +| `GITLAB_API_TOKEN` | Shipment MR fetch, MR approval check | Required for full validation |🤖 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/advisory-promotion/SKILL.md` around lines 31 - 35, The prerequisites table in the advisory-promotion skill is ambiguous because VPN and GITLAB_API_TOKEN are marked mandatory even though the workflow only downgrades those checks to WARN when missing. Update the requirement wording in SKILL.md to clearly distinguish hard prerequisites from optional/WARN-only inputs, and make sure the descriptions for shipment-related checks and catalog queries match the behavior described later in the skill. Use the table entries for VPN and GITLAB_API_TOKEN as the primary symbols to align the documented expectations with the actual run behavior.Source: Path instructions
🤖 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.
Outside diff comments:
In `@plugins/microshift-release/skills/advisory-promotion/SKILL.md`:
- Around line 31-35: The prerequisites table in the advisory-promotion skill is
ambiguous because VPN and GITLAB_API_TOKEN are marked mandatory even though the
workflow only downgrades those checks to WARN when missing. Update the
requirement wording in SKILL.md to clearly distinguish hard prerequisites from
optional/WARN-only inputs, and make sure the descriptions for shipment-related
checks and catalog queries match the behavior described later in the skill. Use
the table entries for VPN and GITLAB_API_TOKEN as the primary symbols to align
the documented expectations with the actual run behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0c00d4ff-6dd8-42e7-98b7-8a80389fff85
📒 Files selected for processing (4)
plugins/microshift-release/scripts/advisory_promotion.pyplugins/microshift-release/scripts/lib/pyxis.pyplugins/microshift-release/scripts/requirements.txtplugins/microshift-release/skills/advisory-promotion/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- plugins/microshift-release/scripts/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/microshift-release/scripts/lib/pyxis.py
- plugins/microshift-release/scripts/advisory_promotion.py
Contributor
Author
|
/restart |
Contributor
Author
|
/retest |
- Default to stage mode: prod catalog checks skipped (N/A), reducing noise during pre-promotion validation - --prod flag enables both stage and prod catalog checks - New per-variant check: catalog_chi validates Container Health Index grade from Pyxis freshness_grades (grade A = PASS) - Extend _parse_image_metadata with freshness_grade and container_grade_status fields - Add 8 new tests (44 total): TestCHIFreshness, TestCheckInCatalog Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
Author
|
Both implemented in fc2a65b:
CHI check — New |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
# Conflicts: # .claude-plugin/marketplace.json pre-commit.check-secrets: ENABLED
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/microshift-release/scripts/lib/pyxis.py (1)
216-233: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a direct test for the new Pyxis metadata mapping.
check_chi_freshness()now depends on this parser emittingfreshness_grade, but the added tests only stub the post-parsed shape via_catalog(...)and never exercise_parse_image_metadata()itself. If the Pyxisfreshness_gradespayload shape or ordering differs, this regresses silently. Per CONTRIBUTING.md: “regex patterns, parsing rules, and validation functions require tests with positive and negative cases.” Please add a focused unit test that feeds raw Pyxis image data through_parse_image_metadata()and asserts both the present and missing-grade cases.🤖 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/pyxis.py` around lines 216 - 233, Add a focused unit test for _parse_image_metadata() to directly cover the new freshness_grade mapping instead of only mocking the parsed catalog shape. Feed raw Pyxis image data through parse logic and assert it returns the expected freshness_grade when freshness_grades is present, and None when it is missing or empty. Include both positive and negative cases so check_chi_freshness() stays protected if the Pyxis payload shape or ordering changes.Source: Path instructions
🤖 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/advisory_promotion.py`:
- Line 271: The list entry in advisory_promotion.py uses an unnecessary f-string
prefix with no interpolation, which will trigger Ruff F541. Update the message
in the promotion-related logic to use a plain string instead, and verify the
surrounding code still passes ruff validation in the same area where the
promotion warning text is assembled.
In `@plugins/microshift-release/scripts/unit_tests/test_advisory_promotion.py`:
- Around line 263-280: The TestCheckInCatalog coverage only exercises PASS and
SKIP paths, so add a negative case for check_in_catalog() that passes {"valid":
False, "reason": ...} and asserts the FAIL status plus preservation of the
backend reason. Place it alongside the existing
test_stage_mode_skips_prod/test_prod_mode_checks_prod cases so the function’s
failure contract is verified without relying on line numbers.
---
Outside diff comments:
In `@plugins/microshift-release/scripts/lib/pyxis.py`:
- Around line 216-233: Add a focused unit test for _parse_image_metadata() to
directly cover the new freshness_grade mapping instead of only mocking the
parsed catalog shape. Feed raw Pyxis image data through parse logic and assert
it returns the expected freshness_grade when freshness_grades is present, and
None when it is missing or empty. Include both positive and negative cases so
check_chi_freshness() stays protected if the Pyxis payload shape or ordering
changes.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f04f34f2-608b-48f3-b7fd-b9a311c8c873
📒 Files selected for processing (4)
plugins/microshift-release/scripts/advisory_promotion.pyplugins/microshift-release/scripts/lib/pyxis.pyplugins/microshift-release/scripts/unit_tests/test_advisory_promotion.pyplugins/microshift-release/skills/advisory-promotion/SKILL.md
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
Contributor
|
amd64_el9_catalog_stage_chi CHI grade A -> can we update as |
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
2 participants
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
microshift-release:advisory-promotionskill — validates Konflux bootc advisory readiness for QE sign-offTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit