CLID-690: Automate invalid related image catalog test (OCP-73784) - #1493
CLID-690: Automate invalid related image catalog test (OCP-73784)#1493adolfo-ab wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@adolfo-ab: This pull request references CLID-690 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 task to target either version "5.0." or "openshift-5.0.", but it targets "openshift-5.1" instead. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adolfo-ab 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis change adds an invalid related-image operator catalog and integration coverage. It also updates mirror-to-disk archiving to stop the local registry before archiving the working directory and registry log. ChangesInvalid related-image catalog validation
Archive registry shutdown
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RunMirrorToDisk
participant BuildArchive
participant LocalRegistry
participant Archive
RunMirrorToDisk->>BuildArchive: provide onBlobsGathered callback
BuildArchive->>BuildArchive: gather image blobs
BuildArchive->>RunMirrorToDisk: invoke callback
RunMirrorToDisk->>LocalRegistry: stopLocalRegistry
BuildArchive->>Archive: archive working directory and registry log
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 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 |
![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
🧹 Nitpick comments (1)
tests/integration/operators_test.go (1)
130-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBind the error assertion to the invalid image.
Line 130 checks generic collection-error text. It does not prove that
registry.example.com/foo/operand-missing-tagorfoo.v0.9.9-invalid-related-imagecaused the failure. Include a fixture-specific token in the expected output, or inspect the collection error directly, so the test cannot pass after it stops exercising this invalid-related-image path.🤖 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 `@tests/integration/operators_test.go` at line 130, Strengthen the assertion in the test around expectOcMirrorExitCode so the expected error output includes a token uniquely identifying the invalid image fixture, such as its repository or tag, alongside the existing collection-error and empty tag checks. Ensure the test fails if the invalid-related-image path is no longer exercised.
🤖 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 `@tests/integration/image-builders/operator/catalogs/README.md`:
- Around line 149-154: Align the fixture documentation with the behavior
asserted by the integration test in operators_test.go: state that this case
reproduces the whole-catalog failure, including exit code 4 and no mirrored
repositories. Do not describe graceful skipping as the expected outcome unless
you also update the corresponding test expectation.
---
Nitpick comments:
In `@tests/integration/operators_test.go`:
- Line 130: Strengthen the assertion in the test around expectOcMirrorExitCode
so the expected error output includes a token uniquely identifying the invalid
image fixture, such as its repository or tag, alongside the existing
collection-error and empty tag checks. Ensure the test fails if the
invalid-related-image path is no longer exercised.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ceb4e9b-4b48-4f1e-a127-4fecd63e7152
📒 Files selected for processing (6)
tests/integration/image-builders/operator/catalogs/README.mdtests/integration/image-builders/operator/catalogs/test-catalog-invalid-images/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-invalid-images/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-invalid-images/foo/operator.yamltests/integration/operators_test.gotests/integration/testdata/imagesetconfigs/operators/isc-operator-invalid-images.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/pkg/archive/archive_test.go (1)
84-84: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-nil callback path.
Every changed call passes
nil, so these tests verify only the new signature. They do not verify that the callback runs once afteraddImagesDiffand beforeworking-diris archived. Add a focused ordering test because this boundary controls registry-log completeness.Also applies to: 107-107, 136-136, 161-161, 185-185
🤖 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 `@internal/pkg/archive/archive_test.go` at line 84, The archive tests currently cover only a nil callback; add a focused non-nil callback test around BuildArchive that records callback execution and verifies it runs exactly once after addImagesDiff completes but before the working directory is archived. Update the relevant BuildArchive test setup to pass the callback and assert this ordering while preserving existing archive behavior.internal/pkg/cli/executor.go (1)
944-948: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the global
logrusoutput after registry shutdown.The callback now runs before
BuildArchivefinishes.stopLocalRegistrychanges the globallogrusoutput toio.Discardand never restores it. Laterlogrusmessages from archive work or other in-process work are silently lost. Restore the previous output after closingregistryLogFile, or use a registry-specific logger.🤖 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 `@internal/pkg/cli/executor.go` around lines 944 - 948, Update stopLocalRegistry to preserve and restore the global logrus output after shutting down the registry. Capture the previous output before switching to io.Discard, then restore it after closing registryLogFile so subsequent BuildArchive and in-process logging remains visible.
🤖 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 `@internal/pkg/cli/executor.go`:
- Around line 763-771: The stopLocalRegistry method must only set
registryStopped after LocalStorageService.Shutdown and log-file cleanup complete
successfully. Propagate any shutdown error to the BuildArchive callback and Run
cleanup so archiving stops instead of continuing with an active registry; retain
idempotent behavior for successful prior cleanup.
---
Nitpick comments:
In `@internal/pkg/archive/archive_test.go`:
- Line 84: The archive tests currently cover only a nil callback; add a focused
non-nil callback test around BuildArchive that records callback execution and
verifies it runs exactly once after addImagesDiff completes but before the
working directory is archived. Update the relevant BuildArchive test setup to
pass the callback and assert this ordering while preserving existing archive
behavior.
In `@internal/pkg/cli/executor.go`:
- Around line 944-948: Update stopLocalRegistry to preserve and restore the
global logrus output after shutting down the registry. Capture the previous
output before switching to io.Discard, then restore it after closing
registryLogFile so subsequent BuildArchive and in-process logging remains
visible.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eea6d654-726f-4d5d-a60f-af654acf94d2
📒 Files selected for processing (5)
internal/pkg/archive/archive.gointernal/pkg/archive/archive_test.gointernal/pkg/archive/interface.gointernal/pkg/cli/executor.gointernal/pkg/cli/executor_test.go
…e archiving working-dir
|
@adolfo-ab: all tests passed! 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. |
| if o.registryStopped { | ||
| return | ||
| } | ||
| o.registryStopped = true |
There was a problem hiding this comment.
Could we use sync.Once to run the function contents only once via a synchronization primitive instead of using a non-mutexed mutable variable?
3 participants
Description
Add an integration test for OCP-73784 / OCPBUGS-33081, covering an operator catalog whose declarative config contains a bundle with an invalid related image (missing tag/digest, missing name, or unsupported oci:// scheme).
The test mirrors a catalog with one valid bundle and one bundle whose related image has no tag or digest, and documents oc-mirror's current behavior: the whole catalog collection fails (exit code 4) instead of
skipping just the invalid bundle and mirroring the rest. Fixing that behavior is tracked as separate follow-up work; this PR only adds test coverage for the current state.
Adds a new test-catalog-invalid-images fixture under image-builders/operator/catalogs, built and pushed to quay.io/oc-mirror/oc-mirror-dev the same way as the other test catalogs.
Github / Jira issue:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Expected Outcome
Please describe the outcome expected from the tests.
Summary by CodeRabbit
Bug Fixes
Tests