CLID-614: Test for Operator incremental mirroring#1413
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@nidangavali: This pull request references CLID-614 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 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds integration test for operator config-based incremental mirroring (CLID-614). Two ISC YAML fixtures define initial and update mirror configurations for operator ChangesOperator Incremental Mirroring Integration Test (CLID-614)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 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 `@tests/integration/incremental_test.go`:
- Around line 36-38: The test currently hardcodes "mirror_000001.tar" (assigned
to initialTar) which can break if sequence naming changes; replace the hardcoded
lookup by discovering produced mirror tar files in workDir with a glob like
"mirror_*.tar", assert the glob returns at least one match, sort the matches (by
filename or modtime) to pick the intended archive (first/earliest for initial,
next for subsequent), assign that path to initialTar (and the later tar variable
used around logTarSummary), and then call logTarSummary with the discovered
path; update any other hardcoded uses of "mirror_00000X.tar" similarly.
- Around line 68-70: The current assertion using
Expect(incrementalBlobs).NotTo(ConsistOf(initialBlobs)) only proves the sets
differ; change the test to assert the incremental tar contains none of the
previously mirrored blobs by verifying there is no intersection between
incrementalBlobs and initialBlobs. Replace the ConsistOf-based assertion with
one that checks incrementalBlobs does not contain any element from initialBlobs
(e.g., assert the intersection length is zero or use a Gomega matcher that
ensures no elements from initialBlobs appear in incrementalBlobs) referencing
the incrementalBlobs and initialBlobs variables and the Expect call.
🪄 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/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d60a918f-b59b-49dd-aa3e-496c58791e63
📒 Files selected for processing (4)
tests/integration/helpers_test.gotests/integration/incremental_test.gotests/integration/testdata/imagesetconfigs/operators/isc-operator-incremental-initial.yamltests/integration/testdata/imagesetconfigs/operators/isc-operator-incremental-update.yaml
|
/retest |
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks for the PR, I just added few comments to it.
Please add a PR description following the template on .github directory and if possible also a commit message with context about the changes for future reference.
| for _, b := range incrementalBlobs { | ||
| _, alreadyMirrored := initialBlobSet[b] | ||
| Expect(alreadyMirrored).To(BeFalse(), | ||
| "incremental tar re-included previously mirrored blob: %s", b) | ||
| } |
There was a problem hiding this comment.
It would be good if we could check if the blobs of the incremental run were included correctly. Currently we are only checking if the initial blobs were not included.
| By("moving the initial tar outside the working directory to preserve it") | ||
| preserveDir, err := os.MkdirTemp("", "oc-mirror-preserved-tar-*") | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| defer os.RemoveAll(preserveDir) | ||
| preservedTar := filepath.Join(preserveDir, "mirror_initial.tar") | ||
| err = os.Rename(initialTar, preservedTar) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to move initial tar") |
There was a problem hiding this comment.
It seems we're not using the initial tar later, so this is dead code that can be removed. Or am I missing anything?
There was a problem hiding this comment.
I tried to remove the initial tar from workDir since the second M2D overwrites the initial tar, moving it to a temp directory is the only way to keep the initial tar's content available for the later blob comparison.
There was a problem hiding this comment.
In which line are we using the initial tar?
| logTarSummary("initial", initialTar) | ||
|
|
||
| By("verifying the initial tar contains expected content") | ||
| expectCorrectTarArchiveContents(iscInitialPath, workDir) |
There was a problem hiding this comment.
Are we also checking the blobs here? Only only the directory path?
There was a problem hiding this comment.
Just the directory path.
There was a problem hiding this comment.
It's important we check also the blobs, because this is the most important thing to check in the incremental feature.
|
|
||
| By("comparing blob paths between the two tars") | ||
| const blobPrefix = "docker/registry/v2/blobs/sha256" | ||
| initialBlobs := collectTarBlobPaths(preservedTar, blobPrefix) |
There was a problem hiding this comment.
@aguidirh this is the line where it is used to compare the blobs from both the tar files.
There was a problem hiding this comment.
I will be refactoring the code since there is already a incremental test file now.
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 `@tests/integration/incremental_mirroring_test.go`:
- Around line 147-150: The deferred cleanup call to os.RemoveAll(preserveDir)
does not capture or handle its error return value. Modify the defer statement to
use a closure that captures the error returned by os.RemoveAll and logs or
handles it appropriately, ensuring that any cleanup failures are not silently
ignored per the coding guidelines that require never ignoring error returns.
- Line 122: The test "should produce a second tar with only incremental blob
content" is a long-running integration test that lacks a timeout and needs to
pass SpecContext to its mirroring operations. Add a SpecTimeout parameter to the
It() function call to prevent test suite hangs, and update all MirrorToDisk
operation calls within this test (and the related calls at lines 134-135 and
155-156) to accept and use the SpecContext parameter instead of the outer ctx
variable to ensure proper test context handling within Ginkgo.
🪄 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/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5907f313-9121-42bd-a4c1-62b6c40967ac
📒 Files selected for processing (4)
tests/integration/helpers_test.gotests/integration/incremental_mirroring_test.gotests/integration/testdata/imagesetconfigs/operators/isc-operator-incremental-initial.yamltests/integration/testdata/imagesetconfigs/operators/isc-operator-incremental-update.yaml
✅ Files skipped from review due to trivial changes (1)
- tests/integration/testdata/imagesetconfigs/operators/isc-operator-incremental-initial.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/testdata/imagesetconfigs/operators/isc-operator-incremental-update.yaml
- tests/integration/helpers_test.go
| iscInitial := filepath.Join("operators", "isc-operator-incremental-initial.yaml") | ||
| iscUpdate := filepath.Join("operators", "isc-operator-incremental-update.yaml") | ||
|
|
||
| It("should produce a second tar with only incremental blob content", func() { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this spec currently lacks SpecTimeout and uses non-spec context in MirrorToDisk calls.
rg -nP --type=go -C2 'It\("should produce a second tar with only incremental blob content",\s*func\(' tests/integration/incremental_mirroring_test.go
rg -nP --type=go -C2 'runner\.MirrorToDisk\(' tests/integration/incremental_mirroring_test.goRepository: openshift/oc-mirror
Length of output: 1647
Add timeout to long-running incremental mirroring test and pass SpecContext to mirroring calls.
Line 122 defines a long-running integration test with two MirrorToDisk operations but no SpecTimeout, creating risk of suite hangs. Additionally, the test should pass Ginkgo's SpecContext to the mirroring operations instead of the outer ctx.
Suggested fix
- It("should produce a second tar with only incremental blob content", func() {
+ It("should produce a second tar with only incremental blob content", SpecTimeout(10*time.Minute), func(specCtx SpecContext) {
@@
- result, err := runner.MirrorToDisk(ctx, iscInitialPath, workDir, "--remove-signatures=true")
+ result, err := runner.MirrorToDisk(specCtx, iscInitialPath, workDir, "--remove-signatures=true")
@@
- result, err = runner.MirrorToDisk(ctx, iscUpdatePath, workDir, "--remove-signatures=true")
+ result, err = runner.MirrorToDisk(specCtx, iscUpdatePath, workDir, "--remove-signatures=true")Also applies to lines 134–135, 155–156.
🤖 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/incremental_mirroring_test.go` at line 122, The test
"should produce a second tar with only incremental blob content" is a
long-running integration test that lacks a timeout and needs to pass SpecContext
to its mirroring operations. Add a SpecTimeout parameter to the It() function
call to prevent test suite hangs, and update all MirrorToDisk operation calls
within this test (and the related calls at lines 134-135 and 155-156) to accept
and use the SpecContext parameter instead of the outer ctx variable to ensure
proper test context handling within Ginkgo.
Source: Coding guidelines
|
@nidangavali: 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adolfo-ab, nidangavali 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 |
4 participants
Description
Add an integration test for operator incremental mirroring (mirrorToDisk). The test verifies that oc-mirror produces archives containing only new content on subsequent runs when the ImageSetConfig changes.
The test runs mirrorToDisk twice against the same workspace with different ISCs:
It then validates that the second archive contains only the incremental data,new blobs from the expanded version range and the newly added package with no overlap from the first archive.
Github / Jira issue: CLID-614
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
Summary by CodeRabbit
Summary by CodeRabbit