ART-21816: Add doozer verify-image-grades command for Pyxis freshness… - #3216
ART-21816: Add doozer verify-image-grades command for Pyxis freshness…#3216tomasdavidorg wants to merge 8 commits into
Conversation
… grade check Queries the Red Hat Catalog (Pyxis) API for container image freshness grades from a shipment MR. Flags images with grade worse than B or Unknown as unhealthy. Uses standard --group + --assembly global options with config_only initialization. Resolves shipment MR URL from assembly config in releases.yml via assembly_config_struct(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
@tomasdavidorg: This pull request references ART-21816 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
[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 |
|
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 the ChangesImage Grade Verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant verify_image_grades_cli
participant resolve_shipment_mr_url
participant fetch_shipment_components
participant GitLab
participant verify_image_grades
participant query_freshness_grades
participant Pyxis
participant render_result
verify_image_grades_cli->>resolve_shipment_mr_url: Resolve shipment MR URL
resolve_shipment_mr_url->>fetch_shipment_components: Pass MR URL
fetch_shipment_components->>GitLab: Read shipment YAML and advisory files
GitLab-->>fetch_shipment_components: Return component images
fetch_shipment_components-->>verify_image_grades_cli: Return components and shipment version
verify_image_grades_cli->>verify_image_grades: Verify image digests
verify_image_grades->>query_freshness_grades: Query grades concurrently
query_freshness_grades->>Pyxis: Request freshness grades
Pyxis-->>query_freshness_grades: Return grades or failures
query_freshness_grades-->>verify_image_grades: Return image grades
verify_image_grades-->>render_result: Return verification result
render_result-->>verify_image_grades_cli: Render text or JSON output
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 7❌ Failed checks (1 warning, 6 inconclusive)
✅ Passed checks (4 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: 4
🧹 Nitpick comments (5)
doozer/doozerlib/cli/verify_image_grades.py (1)
155-184: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDistinguish "grade unavailable" from "bad grade".
query_freshness_gradesreturns[]for a non-200 status, a timeout, and a genuine empty result. All three become gradeUnknown, whichhealthytreats as unhealthy. During a Pyxis outage every image is reported unhealthy, and the operator cannot tell an actual grade regression from a failed lookup.Add a distinct state, for example an
available: boolfield onImageGradeResult, and add a small retry with backoff for 5xx and timeout responses. Report unavailable counts separately in the output.🤖 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 `@doozer/doozerlib/cli/verify_image_grades.py` around lines 155 - 184, Update ImageGradeResult and the verify_image_grades flow to distinguish unavailable freshness lookups from genuinely returned Unknown grades: have query_freshness_grades expose availability, add limited retry with backoff for 5xx and timeout failures, propagate the state through _check_grade, and report unavailable results separately from unhealthy grades.doozer/tests/cli/test_verify_image_grades.py (4)
148-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the request parameters and use a realistic digest.
The aiohttp async-context-manager mocking is correct. Two gaps remain:
- No test asserts what URL or query parameters
query_freshness_gradessends. An assertion on thefiltervalue would cover the query-construction change requested ondoozer/doozerlib/cli/verify_image_grades.pylines 139-152.- The fixture digest
"abc123"is not a 64-character hex digest. If you add digest validation, update this fixture to a valid digest and add a case that asserts a malformed digest is rejected without an HTTP call.🤖 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 `@doozer/tests/cli/test_verify_image_grades.py` around lines 148 - 186, Enhance the TestQueryFreshnessGrades tests around query_freshness_grades by replacing "abc123" with a valid 64-character hexadecimal digest, asserting the request URL and filter query parameter, and adding a malformed-digest case that verifies rejection occurs without calling the HTTP session. Keep the existing success, empty-data, and API-error expectations unchanged.
231-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the CLI exit code.
The mocking is correct here.
patchreturns anAsyncMockforquery_freshness_gradesbecause it is anasync def, so the awaited return value works.fetch_shipment_componentsis synchronous and is patched at module scope, soasyncio.to_threadresolves the patched attribute.No test covers
verify_image_grades_cli. Add aclick.testing.CliRunnertest that asserts exit code 1 for unhealthy images and exit code 0 for a passing run. That path is whereraise SystemExit(1)on line 258 takes effect.🤖 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 `@doozer/tests/cli/test_verify_image_grades.py` around lines 231 - 323, Add a CliRunner-based test class for verify_image_grades_cli covering both unhealthy and passing results, reusing the existing fetch_shipment_components and query_freshness_grades mocks. Assert that the unhealthy invocation exits with code 1 and the passing invocation exits with code 0, exercising the CLI’s SystemExit behavior.
33-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a
Z-suffixedstart_date.Every test here builds the timestamp with
datetime.now(timezone.utc).isoformat(), which always produces a+00:00offset. Pyxis also returnsstart_datevalues with a trailingZ. The current tests cannot detect the parse failure described in the comment ondoozer/doozerlib/cli/verify_image_grades.pylines 62-82.Add cases for
"2024-01-01T00:00:00Z"and for a naive timestamp without an offset.🤖 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 `@doozer/tests/cli/test_verify_image_grades.py` around lines 33 - 64, Extend TestGetCurrentGrade with cases covering a start_date formatted with a trailing Z and a naive timestamp without an offset, using fixed historical values and asserting the expected grade is returned. Keep the existing timezone-aware test coverage unchanged and ensure both formats exercise get_current_grade parsing.
189-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative-path tests for shipment parsing.
Only the happy path is covered. The
except Exceptionblock infetch_shipment_componentshides every failure, so these paths need explicit tests:
- An empty YAML file, where
yaml.safe_loadreturnsNone.- A YAML document that is not a mapping.
- A
source_project.files.getcall that raises.- An MR title with no version, which must yield
version == "".- A diff that contains no
.yamlor.ymlfiles.🤖 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 `@doozer/tests/cli/test_verify_image_grades.py` around lines 189 - 228, Add five new test methods to the TestFetchShipmentComponents class to cover the negative paths currently hidden by the broad exception handling in fetch_shipment_components: test for when yaml.safe_load returns None (empty YAML), test for non-mapping YAML documents, test for source_project.files.get raising an exception, test for MR titles without version information (expecting version == ""), and test for diffs containing no .yaml or .yml files. Each test should follow the same mocking setup pattern as test_parses_components, adjusting mock configurations to trigger the specific failure condition.
🤖 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 `@doozer/doozerlib/cli/verify_image_grades.py`:
- Around line 155-184: The grade lookup currently conflates unavailable results
with genuinely unhealthy grades. In ImageGradeResult and verify_image_grades,
add and populate an available flag, setting it false for lookup failures while
preserving graded results, and update render_result to report unavailable images
separately from graded-but-unhealthy images. In get_current_grade, normalize
trailing Z and naive timestamps before datetime.fromisoformat and log every
skipped start_date instead of silently returning Unknown; apply these changes at
the affected locations in doozer/doozerlib/cli/verify_image_grades.py (lines
155-184 and 62-82).
- Around line 119-136: Update the shipment-file parsing flow around
yaml.safe_load and the surrounding try/except to validate that the parsed
document is a mapping before accessing shipment fields, and track any read or
parse failure instead of silently continuing. Propagate the failure to the
caller so VerifyImageGradesResult.passed cannot report PASS or exit successfully
when shipment files are unreadable or invalid, while preserving normal component
extraction for valid mappings.
- Around line 139-152: Update query_freshness_grades to validate digest at the
trust boundary using Unicode normalization and an anchored allow-list for the
expected digest format, returning no grades for invalid values. Stop
interpolating digest into the URL; keep the fixed Pyxis endpoint and pass the
filter through aiohttp’s params argument so query parameters are encoded safely.
In `@doozer/tests/cli/test_verify_image_grades.py`:
- Around line 102-105: Decide the intended zero-component behavior in the
verify-image-grades result flow and update test_empty accordingly. If an empty
image set is invalid, replace the current passed assertion with assertions for
the expected error or failure outcome while preserving the total_scanned == 0
check; otherwise retain the PASS expectation and align the implementation
consistently.
---
Nitpick comments:
In `@doozer/doozerlib/cli/verify_image_grades.py`:
- Around line 155-184: Update ImageGradeResult and the verify_image_grades flow
to distinguish unavailable freshness lookups from genuinely returned Unknown
grades: have query_freshness_grades expose availability, add limited retry with
backoff for 5xx and timeout failures, propagate the state through _check_grade,
and report unavailable results separately from unhealthy grades.
In `@doozer/tests/cli/test_verify_image_grades.py`:
- Around line 148-186: Enhance the TestQueryFreshnessGrades tests around
query_freshness_grades by replacing "abc123" with a valid 64-character
hexadecimal digest, asserting the request URL and filter query parameter, and
adding a malformed-digest case that verifies rejection occurs without calling
the HTTP session. Keep the existing success, empty-data, and API-error
expectations unchanged.
- Around line 231-323: Add a CliRunner-based test class for
verify_image_grades_cli covering both unhealthy and passing results, reusing the
existing fetch_shipment_components and query_freshness_grades mocks. Assert that
the unhealthy invocation exits with code 1 and the passing invocation exits with
code 0, exercising the CLI’s SystemExit behavior.
- Around line 33-64: Extend TestGetCurrentGrade with cases covering a start_date
formatted with a trailing Z and a naive timestamp without an offset, using fixed
historical values and asserting the expected grade is returned. Keep the
existing timezone-aware test coverage unchanged and ensure both formats exercise
get_current_grade parsing.
- Around line 189-228: Add five new test methods to the
TestFetchShipmentComponents class to cover the negative paths currently hidden
by the broad exception handling in fetch_shipment_components: test for when
yaml.safe_load returns None (empty YAML), test for non-mapping YAML documents,
test for source_project.files.get raising an exception, test for MR titles
without version information (expecting version == ""), and test for diffs
containing no .yaml or .yml files. Each test should follow the same mocking
setup pattern as test_parses_components, adjusting mock configurations to
trigger the specific failure condition.
🪄 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: 2e28598e-f715-4b66-98f3-b29f0e0d565d
📒 Files selected for processing (3)
doozer/doozerlib/cli/__main__.pydoozer/doozerlib/cli/verify_image_grades.pydoozer/tests/cli/test_verify_image_grades.py
…yxis API Read images from shipment advisory internal_url (registry.stage.redhat.io pullspecs) instead of snapshot components (quay.io Konflux builds). Query catalog.stage.redhat.com via corporate proxy, matching OAR behavior. Skip FBC files. Add debug logging for individual grades. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
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 `@doozer/doozerlib/cli/verify_image_grades.py`:
- Around line 137-139: Validate advisory_url before requests.get in the advisory
verification flow: parse it and allow only HTTPS, explicitly allow-listed
advisory hosts, no user-info, and standard ports. Disable redirects or validate
every redirect target with the same policy, and reject invalid URLs before
making any request. Add tests covering disallowed hosts and redirect targets.
In `@doozer/tests/cli/test_verify_image_grades.py`:
- Line 270: Update the fetch_shipment_components unpacking at both test
locations to bind the unused second return value to _ instead of version, while
preserving the components assignment and existing behavior.
🪄 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: 22f92263-7603-442a-b15c-cc245b1d9381
📒 Files selected for processing (2)
doozer/doozerlib/cli/verify_image_grades.pydoozer/tests/cli/test_verify_image_grades.py
…andling Validate digest format and advisory URL host at trust boundaries, prevent false-positive PASS when all shipment files fail to parse, guard against non-mapping YAML documents. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doozer/tests/cli/test_verify_image_grades.py (1)
195-383: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd coverage for non-mapping YAML handling.
No test covers a shipment YAML document that parses to a list or scalar. Add a test that asserts the file is skipped, a warning is logged, and no advisory request is made.
🤖 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 `@doozer/tests/cli/test_verify_image_grades.py` around lines 195 - 383, Add a test in TestFetchShipmentComponents covering shipment YAML that parses to a non-mapping value such as a list or scalar; configure the mocked file and GitLab objects like the existing shipment tests, assert fetch_shipment_components skips the file, emits a warning through the module’s logger, and does not call requests.get.
🧹 Nitpick comments (2)
doozer/tests/cli/test_verify_image_grades.py (2)
148-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared mock scaffolding for Pyxis response tests.
test_successful_query,test_empty_data, andtest_api_erroreach rebuild the samemock_resp/mock_sessionboilerplate (AsyncMock with__aenter__/__aexit__,status, andjson). Extract a small helper to build the mocked session for a given status/JSON payload. This reduces duplication and makes adding new grade-response cases easier.♻️ Proposed helper extraction
+ def _mock_session(self, status=200, json_data=None): + mock_resp = AsyncMock() + mock_resp.status = status + if json_data is not None: + mock_resp.json = AsyncMock(return_value=json_data) + mock_resp.__aenter__ = AsyncMock(return_value=mock_resp) + mock_resp.__aexit__ = AsyncMock(return_value=False) + mock_session = AsyncMock(spec=ClientSession) + mock_session.get = MagicMock(return_value=mock_resp) + return mock_session + async def test_successful_query(self): grades = [{"start_date": "2026-01-01T00:00:00+00:00", "grade": "A"}] - mock_resp = AsyncMock() - mock_resp.status = 200 - mock_resp.json = AsyncMock(return_value={"data": [{"freshness_grades": grades}]}) - mock_resp.__aenter__ = AsyncMock(return_value=mock_resp) - mock_resp.__aexit__ = AsyncMock(return_value=False) - - mock_session = AsyncMock(spec=ClientSession) - mock_session.get = MagicMock(return_value=mock_resp) - + mock_session = self._mock_session(200, {"data": [{"freshness_grades": grades}]}) result = await query_freshness_grades(mock_session, "a" * 64) self.assertEqual(result, grades)🤖 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 `@doozer/tests/cli/test_verify_image_grades.py` around lines 148 - 193, Extract the repeated mock response/session setup from test_successful_query, test_empty_data, and test_api_error into a shared helper that accepts the HTTP status and JSON payload, configures the AsyncMock context-manager methods, and returns the mocked ClientSession. Update those tests to use the helper while preserving their existing inputs and assertions.
195-383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared GitLab mock fixture for
TestFetchShipmentComponents.Every test in this class (lines 198-258, 262-281, 285-318, 322-346, 350-382) rebuilds nearly identical
mock_diff/mock_mr/mock_glscaffolding. Move the common setup into a helper (e.g., a_build_gitlab_mocks(diff_paths)method or a fixture insetUp) that returns the configuredmock_gl_cls/mock_project. This removes roughly 60-70 lines of repeated boilerplate and keeps each test focused on the behavior it targets.🤖 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 `@doozer/tests/cli/test_verify_image_grades.py` around lines 195 - 383, Extract the repeated GitLab setup from TestFetchShipmentComponents into a shared helper such as _build_gitlab_mocks(diff_paths) or setUp, configuring the mock diff, merge request, project, and GitLab client while allowing each test to supply its changed paths. Update all five tests to reuse the helper and retain only test-specific shipment files, advisory responses, and assertions.
🤖 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 `@doozer/doozerlib/cli/verify_image_grades.py`:
- Line 146: The advisory URL is embedded in the RuntimeError and may expose
credentials when logged with exc_info. Update the validation error in the
advisory URL handling block to use a generic message or a safely redacted value,
while preserving the rejected-host validation behavior.
---
Outside diff comments:
In `@doozer/tests/cli/test_verify_image_grades.py`:
- Around line 195-383: Add a test in TestFetchShipmentComponents covering
shipment YAML that parses to a non-mapping value such as a list or scalar;
configure the mocked file and GitLab objects like the existing shipment tests,
assert fetch_shipment_components skips the file, emits a warning through the
module’s logger, and does not call requests.get.
---
Nitpick comments:
In `@doozer/tests/cli/test_verify_image_grades.py`:
- Around line 148-193: Extract the repeated mock response/session setup from
test_successful_query, test_empty_data, and test_api_error into a shared helper
that accepts the HTTP status and JSON payload, configures the AsyncMock
context-manager methods, and returns the mocked ClientSession. Update those
tests to use the helper while preserving their existing inputs and assertions.
- Around line 195-383: Extract the repeated GitLab setup from
TestFetchShipmentComponents into a shared helper such as
_build_gitlab_mocks(diff_paths) or setUp, configuring the mock diff, merge
request, project, and GitLab client while allowing each test to supply its
changed paths. Update all five tests to reuse the helper and retain only
test-specific shipment files, advisory responses, and assertions.
🪄 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: 0d72381d-690a-48a4-a7b7-49a8cba41ce1
📒 Files selected for processing (2)
doozer/doozerlib/cli/verify_image_grades.pydoozer/tests/cli/test_verify_image_grades.py
Log only the hostname instead of the full URL to avoid leaking credentials in exception traces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
Empty image set no longer reports PASS — a shipment must contain at least one image. Unparseable or missing start_date values are now logged as warnings instead of silently skipped. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
…ages Add available field to ImageGradeResult to separate Pyxis lookup failures from real grade regressions. Report unavailable images separately in both text and JSON output. Normalize Z-suffix and naive timestamps in grade date parsing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
tomasdavidorg
added
the
tide/merge-method-squash
Konflux advisory YAMLs are hosted on GitLab, not Errata Tool. The internal_url field in shipment files legitimately points to gitlab.cee.redhat.com/rhtap-release/advisories/ or gitlab.cee.redhat.com/releng/advisories/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
Instead of checking only the hostname (which allows any path on gitlab.cee.redhat.com), validate that the full URL starts with one of the known advisory origins: errata hosts or specific GitLab advisory repos (rhtap-release/advisories, releng/advisories). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
@tomasdavidorg: The following test 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. |
2 participants
… grade check
Queries the Red Hat Catalog (Pyxis) API for container image freshness grades from a shipment MR. Flags images with grade worse than B or Unknown as unhealthy.
Uses standard --group + --assembly global options with config_only initialization. Resolves shipment MR URL from assembly config in releases.yml via assembly_config_struct().
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
rh-pre-commit.version: 2.4.0
rh-pre-commit.check-secrets: ENABLED
Summary by CodeRabbit
New Features
verify-image-gradescommand to assess shipment container image freshness.Tests