CLID-655: Add test for incremental mirroring using archives by date#1416
Conversation
|
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 (1)
WalkthroughAdds a new Ginkgo integration test and helpers that locate mirror_*.tar archives, count docker/registry/v2/blobs/ entries across those archives, and verify that running MirrorToDisk with a future --since produces an incremental archive containing no image layer blobs. ChangesIncremental Mirroring Integration Test
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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/helpers_test.go`:
- Around line 798-833: The blob checks currently iterate all files matching
"mirror_*.tar", which can include the initial archive; update the helpers (the
blob-counting function that returns blobCount and expectNoImageBlobsInTar) to
scope to the incremental archive by selecting only the incremental tar file
instead of all matches—e.g. from matches choose the single most recently
modified tar (use os.Stat on each tarFile and pick the highest ModTime) or
filter by an "incremental" naming convention if present, then call
listTarEntries only on that selected tarFile and perform the blob
checks/assertions on that one file.
In `@tests/integration/incremental_test.go`:
- Around line 26-68: This spec lacks an explicit Ginkgo timeout and can hang;
add a SpecTimeout option to the It block that wraps the test (the It that starts
with "should produce a smaller archive on incremental run with --since") e.g.
append SpecTimeout(10*time.Minute) (or another appropriate duration) to the It
declaration so the whole spec (including calls to runner.MirrorToDisk,
runner.DiskToMirror, expectOcMirrorCommandSuccess,
expectSuccessfulMirrorInRegistry, etc.) will fail fast on timeout.
🪄 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: f5a0866b-1df3-4598-91b6-0990863f1889
📒 Files selected for processing (3)
tests/integration/helpers_test.gotests/integration/incremental_test.gotests/integration/testdata/imagesetconfigs/incremental/isc-incremental-since.yaml
| matches, err := filepath.Glob(filepath.Join(workDir, "mirror_*.tar")) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(matches).NotTo(BeEmpty(), "no tar archive found in %s", workDir) | ||
|
|
||
| blobCount := 0 | ||
| for _, tarFile := range matches { | ||
| entries := listTarEntries(tarFile) | ||
| for _, entry := range entries { | ||
| if strings.Contains(entry, "docker/registry/v2/blobs") { | ||
| blobCount++ | ||
| } | ||
| } | ||
| } | ||
| return blobCount | ||
| } | ||
|
|
||
| // expectNoImageBlobsInTar verifies that the tar archive(s) in workDir contain no image layer | ||
| // blobs (entries under docker/registry/v2/blobs/). Only working-dir metadata entries should remain. | ||
| func expectNoImageBlobsInTar(workDir string) { | ||
| matches, err := filepath.Glob(filepath.Join(workDir, "mirror_*.tar")) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(matches).NotTo(BeEmpty(), "no tar archive found in %s", workDir) | ||
|
|
||
| var blobEntries []string | ||
| for _, tarFile := range matches { | ||
| entries := listTarEntries(tarFile) | ||
| for _, entry := range entries { | ||
| if strings.Contains(entry, "docker/registry/v2/blobs") { | ||
| blobEntries = append(blobEntries, entry) | ||
| } | ||
| } | ||
| } | ||
| Expect(blobEntries).To(BeEmpty(), | ||
| "expected no image blob entries in incremental archive, but found %d:\n%s", | ||
| len(blobEntries), strings.Join(blobEntries, "\n")) | ||
| } |
There was a problem hiding this comment.
Scope blob checks to the incremental archive, not all mirror_*.tar files.
These helpers currently aggregate every tar in workDir. In a two-phase test (initial + incremental), this can count blobs from the initial archive and invalidate the incremental assertions.
Suggested fix
func countTarBlobEntries(workDir string) int {
matches, err := filepath.Glob(filepath.Join(workDir, "mirror_*.tar"))
Expect(err).NotTo(HaveOccurred())
Expect(matches).NotTo(BeEmpty(), "no tar archive found in %s", workDir)
+ sort.Strings(matches)
+ latestTar := matches[len(matches)-1]
blobCount := 0
- for _, tarFile := range matches {
- entries := listTarEntries(tarFile)
- for _, entry := range entries {
- if strings.Contains(entry, "docker/registry/v2/blobs") {
- blobCount++
- }
- }
+ entries := listTarEntries(latestTar)
+ for _, entry := range entries {
+ if strings.Contains(entry, "docker/registry/v2/blobs/") {
+ blobCount++
+ }
}
return blobCount
}
func expectNoImageBlobsInTar(workDir string) {
matches, err := filepath.Glob(filepath.Join(workDir, "mirror_*.tar"))
Expect(err).NotTo(HaveOccurred())
Expect(matches).NotTo(BeEmpty(), "no tar archive found in %s", workDir)
+ sort.Strings(matches)
+ latestTar := matches[len(matches)-1]
var blobEntries []string
- for _, tarFile := range matches {
- entries := listTarEntries(tarFile)
- for _, entry := range entries {
- if strings.Contains(entry, "docker/registry/v2/blobs") {
- blobEntries = append(blobEntries, entry)
- }
- }
+ entries := listTarEntries(latestTar)
+ for _, entry := range entries {
+ if strings.Contains(entry, "docker/registry/v2/blobs/") {
+ blobEntries = append(blobEntries, entry)
+ }
}
Expect(blobEntries).To(BeEmpty(),
"expected no image blob entries in incremental archive, but found %d:\n%s",
len(blobEntries), strings.Join(blobEntries, "\n"))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| matches, err := filepath.Glob(filepath.Join(workDir, "mirror_*.tar")) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(matches).NotTo(BeEmpty(), "no tar archive found in %s", workDir) | |
| blobCount := 0 | |
| for _, tarFile := range matches { | |
| entries := listTarEntries(tarFile) | |
| for _, entry := range entries { | |
| if strings.Contains(entry, "docker/registry/v2/blobs") { | |
| blobCount++ | |
| } | |
| } | |
| } | |
| return blobCount | |
| } | |
| // expectNoImageBlobsInTar verifies that the tar archive(s) in workDir contain no image layer | |
| // blobs (entries under docker/registry/v2/blobs/). Only working-dir metadata entries should remain. | |
| func expectNoImageBlobsInTar(workDir string) { | |
| matches, err := filepath.Glob(filepath.Join(workDir, "mirror_*.tar")) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(matches).NotTo(BeEmpty(), "no tar archive found in %s", workDir) | |
| var blobEntries []string | |
| for _, tarFile := range matches { | |
| entries := listTarEntries(tarFile) | |
| for _, entry := range entries { | |
| if strings.Contains(entry, "docker/registry/v2/blobs") { | |
| blobEntries = append(blobEntries, entry) | |
| } | |
| } | |
| } | |
| Expect(blobEntries).To(BeEmpty(), | |
| "expected no image blob entries in incremental archive, but found %d:\n%s", | |
| len(blobEntries), strings.Join(blobEntries, "\n")) | |
| } | |
| matches, err := filepath.Glob(filepath.Join(workDir, "mirror_*.tar")) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(matches).NotTo(BeEmpty(), "no tar archive found in %s", workDir) | |
| sort.Strings(matches) | |
| latestTar := matches[len(matches)-1] | |
| blobCount := 0 | |
| entries := listTarEntries(latestTar) | |
| for _, entry := range entries { | |
| if strings.Contains(entry, "docker/registry/v2/blobs/") { | |
| blobCount++ | |
| } | |
| } | |
| return blobCount | |
| } | |
| // expectNoImageBlobsInTar verifies that the tar archive(s) in workDir contain no image layer | |
| // blobs (entries under docker/registry/v2/blobs/). Only working-dir metadata entries should remain. | |
| func expectNoImageBlobsInTar(workDir string) { | |
| matches, err := filepath.Glob(filepath.Join(workDir, "mirror_*.tar")) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(matches).NotTo(BeEmpty(), "no tar archive found in %s", workDir) | |
| sort.Strings(matches) | |
| latestTar := matches[len(matches)-1] | |
| var blobEntries []string | |
| entries := listTarEntries(latestTar) | |
| for _, entry := range entries { | |
| if strings.Contains(entry, "docker/registry/v2/blobs/") { | |
| blobEntries = append(blobEntries, entry) | |
| } | |
| } | |
| Expect(blobEntries).To(BeEmpty(), | |
| "expected no image blob entries in incremental archive, but found %d:\n%s", | |
| len(blobEntries), strings.Join(blobEntries, "\n")) | |
| } |
🤖 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/helpers_test.go` around lines 798 - 833, The blob checks
currently iterate all files matching "mirror_*.tar", which can include the
initial archive; update the helpers (the blob-counting function that returns
blobCount and expectNoImageBlobsInTar) to scope to the incremental archive by
selecting only the incremental tar file instead of all matches—e.g. from matches
choose the single most recently modified tar (use os.Stat on each tarFile and
pick the highest ModTime) or filter by an "incremental" naming convention if
present, then call listTarEntries only on that selected tarFile and perform the
blob checks/assertions on that one file.
| It("should produce a smaller archive on incremental run with --since", func() { | ||
| By("running initial mirrorToDisk") | ||
| result, err := runner.MirrorToDisk(ctx, filepath.Join(iscDir, iscFile), workDir, "--remove-signatures=true") | ||
| expectOcMirrorCommandSuccess(result, err) | ||
|
|
||
| By("verifying initial tar archive was produced") | ||
| initialBlobCount := countTarBlobEntries(workDir) | ||
| GinkgoWriter.Printf("initial archive blob count: %d\n", initialBlobCount) | ||
| Expect(initialBlobCount).To(BeNumerically(">", 0), "initial archive should contain blobs") | ||
|
|
||
| By("running diskToMirror to push initial content to registry") | ||
| result, err = runner.DiskToMirror(ctx, filepath.Join(iscDir, iscFile), workDir, testRegistry.Endpoint(), | ||
| "--remove-signatures=true", "--dest-tls-verify=false") | ||
| expectOcMirrorCommandSuccess(result, err) | ||
|
|
||
| By("verifying images are mirrored in the local registry") | ||
| expectSuccessfulMirrorInRegistry(filepath.Join(iscDir, iscFile), *testRegistry) | ||
|
|
||
| By("running incremental mirrorToDisk with --since set to tomorrow (nothing new expected)") | ||
| sinceDate := time.Now().AddDate(0, 0, 1).Format("2006-01-02") | ||
| GinkgoWriter.Printf("using --since date: %s\n", sinceDate) | ||
| result, err = runner.MirrorToDisk(ctx, filepath.Join(iscDir, iscFile), workDir, | ||
| "--remove-signatures=true", "--since", sinceDate) | ||
| expectOcMirrorCommandSuccess(result, err) | ||
|
|
||
| By("verifying incremental archive has fewer blobs than initial") | ||
| incrementalBlobCount := countTarBlobEntries(workDir) | ||
| GinkgoWriter.Printf("incremental archive blob count: %d (initial was %d)\n", incrementalBlobCount, initialBlobCount) | ||
| Expect(incrementalBlobCount).To(BeNumerically("<", initialBlobCount), | ||
| "incremental archive should have fewer blobs (%d) than initial (%d)", | ||
| incrementalBlobCount, initialBlobCount) | ||
|
|
||
| By("verifying incremental archive contains no image layer blobs") | ||
| expectNoImageBlobsInTar(workDir) | ||
|
|
||
| By("running diskToMirror with incremental archive") | ||
| result, err = runner.DiskToMirror(ctx, filepath.Join(iscDir, iscFile), workDir, testRegistry.Endpoint(), | ||
| "--remove-signatures=true", "--dest-tls-verify=false") | ||
| expectOcMirrorCommandSuccess(result, err) | ||
|
|
||
| By("verifying images are still mirrored in the local registry") | ||
| expectSuccessfulMirrorInRegistry(filepath.Join(iscDir, iscFile), *testRegistry) | ||
| }) |
There was a problem hiding this comment.
Add an explicit Ginkgo timeout for this end-to-end spec.
This test performs several cluster/registry operations; without a spec timeout, failures can stall the suite indefinitely.
Suggested fix
- It("should produce a smaller archive on incremental run with --since", func() {
+ It("should produce a smaller archive on incremental run with --since", func() {
// ...
- })
+ }, SpecTimeout(20*time.Minute))As per coding guidelines, "Ginkgo tests should follow quality requirements: ... (3) appropriate timeouts on cluster interactions, flag indefinite waits or missing timeouts."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| It("should produce a smaller archive on incremental run with --since", func() { | |
| By("running initial mirrorToDisk") | |
| result, err := runner.MirrorToDisk(ctx, filepath.Join(iscDir, iscFile), workDir, "--remove-signatures=true") | |
| expectOcMirrorCommandSuccess(result, err) | |
| By("verifying initial tar archive was produced") | |
| initialBlobCount := countTarBlobEntries(workDir) | |
| GinkgoWriter.Printf("initial archive blob count: %d\n", initialBlobCount) | |
| Expect(initialBlobCount).To(BeNumerically(">", 0), "initial archive should contain blobs") | |
| By("running diskToMirror to push initial content to registry") | |
| result, err = runner.DiskToMirror(ctx, filepath.Join(iscDir, iscFile), workDir, testRegistry.Endpoint(), | |
| "--remove-signatures=true", "--dest-tls-verify=false") | |
| expectOcMirrorCommandSuccess(result, err) | |
| By("verifying images are mirrored in the local registry") | |
| expectSuccessfulMirrorInRegistry(filepath.Join(iscDir, iscFile), *testRegistry) | |
| By("running incremental mirrorToDisk with --since set to tomorrow (nothing new expected)") | |
| sinceDate := time.Now().AddDate(0, 0, 1).Format("2006-01-02") | |
| GinkgoWriter.Printf("using --since date: %s\n", sinceDate) | |
| result, err = runner.MirrorToDisk(ctx, filepath.Join(iscDir, iscFile), workDir, | |
| "--remove-signatures=true", "--since", sinceDate) | |
| expectOcMirrorCommandSuccess(result, err) | |
| By("verifying incremental archive has fewer blobs than initial") | |
| incrementalBlobCount := countTarBlobEntries(workDir) | |
| GinkgoWriter.Printf("incremental archive blob count: %d (initial was %d)\n", incrementalBlobCount, initialBlobCount) | |
| Expect(incrementalBlobCount).To(BeNumerically("<", initialBlobCount), | |
| "incremental archive should have fewer blobs (%d) than initial (%d)", | |
| incrementalBlobCount, initialBlobCount) | |
| By("verifying incremental archive contains no image layer blobs") | |
| expectNoImageBlobsInTar(workDir) | |
| By("running diskToMirror with incremental archive") | |
| result, err = runner.DiskToMirror(ctx, filepath.Join(iscDir, iscFile), workDir, testRegistry.Endpoint(), | |
| "--remove-signatures=true", "--dest-tls-verify=false") | |
| expectOcMirrorCommandSuccess(result, err) | |
| By("verifying images are still mirrored in the local registry") | |
| expectSuccessfulMirrorInRegistry(filepath.Join(iscDir, iscFile), *testRegistry) | |
| }) | |
| It("should produce a smaller archive on incremental run with --since", func() { | |
| By("running initial mirrorToDisk") | |
| result, err := runner.MirrorToDisk(ctx, filepath.Join(iscDir, iscFile), workDir, "--remove-signatures=true") | |
| expectOcMirrorCommandSuccess(result, err) | |
| By("verifying initial tar archive was produced") | |
| initialBlobCount := countTarBlobEntries(workDir) | |
| GinkgoWriter.Printf("initial archive blob count: %d\n", initialBlobCount) | |
| Expect(initialBlobCount).To(BeNumerically(">", 0), "initial archive should contain blobs") | |
| By("running diskToMirror to push initial content to registry") | |
| result, err = runner.DiskToMirror(ctx, filepath.Join(iscDir, iscFile), workDir, testRegistry.Endpoint(), | |
| "--remove-signatures=true", "--dest-tls-verify=false") | |
| expectOcMirrorCommandSuccess(result, err) | |
| By("verifying images are mirrored in the local registry") | |
| expectSuccessfulMirrorInRegistry(filepath.Join(iscDir, iscFile), *testRegistry) | |
| By("running incremental mirrorToDisk with --since set to tomorrow (nothing new expected)") | |
| sinceDate := time.Now().AddDate(0, 0, 1).Format("2006-01-02") | |
| GinkgoWriter.Printf("using --since date: %s\n", sinceDate) | |
| result, err = runner.MirrorToDisk(ctx, filepath.Join(iscDir, iscFile), workDir, | |
| "--remove-signatures=true", "--since", sinceDate) | |
| expectOcMirrorCommandSuccess(result, err) | |
| By("verifying incremental archive has fewer blobs than initial") | |
| incrementalBlobCount := countTarBlobEntries(workDir) | |
| GinkgoWriter.Printf("incremental archive blob count: %d (initial was %d)\n", incrementalBlobCount, initialBlobCount) | |
| Expect(incrementalBlobCount).To(BeNumerically("<", initialBlobCount), | |
| "incremental archive should have fewer blobs (%d) than initial (%d)", | |
| incrementalBlobCount, initialBlobCount) | |
| By("verifying incremental archive contains no image layer blobs") | |
| expectNoImageBlobsInTar(workDir) | |
| By("running diskToMirror with incremental archive") | |
| result, err = runner.DiskToMirror(ctx, filepath.Join(iscDir, iscFile), workDir, testRegistry.Endpoint(), | |
| "--remove-signatures=true", "--dest-tls-verify=false") | |
| expectOcMirrorCommandSuccess(result, err) | |
| By("verifying images are still mirrored in the local registry") | |
| expectSuccessfulMirrorInRegistry(filepath.Join(iscDir, iscFile), *testRegistry) | |
| }, SpecTimeout(20*time.Minute)) |
🤖 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_test.go` around lines 26 - 68, This spec lacks
an explicit Ginkgo timeout and can hang; add a SpecTimeout option to the It
block that wraps the test (the It that starts with "should produce a smaller
archive on incremental run with --since") e.g. append
SpecTimeout(10*time.Minute) (or another appropriate duration) to the It
declaration so the whole spec (including calls to runner.MirrorToDisk,
runner.DiskToMirror, expectOcMirrorCommandSuccess,
expectSuccessfulMirrorInRegistry, etc.) will fail fast on timeout.
|
/retest |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/integration/incremental_test.go (2)
35-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncremental archive scoping bug:
findTarArchives(workDir)returns the initial tar too.The same
workDiris used for bothMirrorToDiskruns. After the second run, the initialmirror_000001.tar(with full blob content) typically still sits next to the new incrementalmirror_000002.tar. Then:
- Line 60–61:
incrementalArchivescontains both archives.- Line 61:
incrementalBlobCountsums blobs from initial + incremental, so it will be>= initialBlobCount, flipping the<assertion at line 64.- Line 69:
expectNoImageBlobsInArchives(incrementalArchives)will fail because the initial archive holds real layer blobs.This is the same root cause raised in the previous review against the helper functions; the refactor moved the responsibility to the caller, but the caller here doesn't filter to the newest archive. Either snapshot the initial archive list and diff (
incrementalArchives = newMatches \ initialArchives), or pick the latest tar by mod time before counting.Suggested fix
By("verifying initial tar archive was produced") initialArchives := findTarArchives(workDir) initialBlobCount := countBlobEntriesInArchives(initialArchives) @@ - By("verifying incremental archive has fewer blobs than initial") - incrementalArchives := findTarArchives(workDir) - incrementalBlobCount := countBlobEntriesInArchives(incrementalArchives) + By("verifying incremental archive has fewer blobs than initial") + allArchives := findTarArchives(workDir) + initialSet := map[string]bool{} + for _, a := range initialArchives { + initialSet[a] = true + } + var incrementalArchives []string + for _, a := range allArchives { + if !initialSet[a] { + incrementalArchives = append(incrementalArchives, a) + } + } + Expect(incrementalArchives).NotTo(BeEmpty(), "no new tar archive produced by incremental run") + incrementalBlobCount := countBlobEntriesInArchives(incrementalArchives)Confirm whether oc-mirror v2 leaves prior
mirror_*.tarfiles in place across successiveMirrorToDiskruns sharing the same working directory:#!/bin/bash # Look for cleanup or overwrite semantics around mirror_*.tar in the oc-mirror v2 code. rg -nP -C3 '\bmirror_(?:\*|%0)|mirror_%0\d+d|os\.Remove.*mirror_' --type=go rg -nP -C3 'mirror_.*\.tar' --type=go -g '!**/integration/**'🤖 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_test.go` around lines 35 - 69, The test uses the same workDir for both MirrorToDisk runs so findTarArchives(workDir) returns both the initial and incremental mirror_*.tar files; update the test around variables initialArchives/incrementalArchives so the second count only inspects the new archive(s) produced by the incremental run (e.g., capture initialArchives, then after the second MirrorToDisk compute incrementalArchives = newMatches minus initialArchives or select the newest tar by modification time) before calling countBlobEntriesInArchives and expectNoImageBlobsInArchives; refer to MirrorToDisk, findTarArchives, initialArchives, incrementalArchives and expectNoImageBlobsInArchives when making the change.
33-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
SpecTimeouton this long-running spec.This It runs two
MirrorToDiskand twoDiskToMirrorinvocations plus registry assertions; without an explicitSpecTimeout, a hung oc-mirror call will stall the suite indefinitely.Suggested fix
- It("should produce a smaller archive on incremental run with --since", func() { + It("should produce a smaller archive on incremental run with --since", func(ctx SpecContext) { By("running initial mirrorToDisk") ... - }) + }, SpecTimeout(20*time.Minute))As per coding guidelines: "Ginkgo tests should follow quality requirements: ... (3) appropriate timeouts on cluster interactions, flag indefinite waits or missing timeouts".
🤖 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_test.go` around lines 33 - 78, The test It("should produce a smaller archive on incremental run with --since") is missing a SpecTimeout and can hang indefinitely; update the It declaration to include a SpecTimeout option (e.g. SpecTimeout(5 * time.Minute)) so the long-running sequence of runner.MirrorToDisk / runner.DiskToMirror calls and registry assertions will fail fast on hangs; keep the same It body and ensure the SpecTimeout value is chosen appropriately for the environment.
🤖 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.
Duplicate comments:
In `@tests/integration/incremental_test.go`:
- Around line 35-69: The test uses the same workDir for both MirrorToDisk runs
so findTarArchives(workDir) returns both the initial and incremental
mirror_*.tar files; update the test around variables
initialArchives/incrementalArchives so the second count only inspects the new
archive(s) produced by the incremental run (e.g., capture initialArchives, then
after the second MirrorToDisk compute incrementalArchives = newMatches minus
initialArchives or select the newest tar by modification time) before calling
countBlobEntriesInArchives and expectNoImageBlobsInArchives; refer to
MirrorToDisk, findTarArchives, initialArchives, incrementalArchives and
expectNoImageBlobsInArchives when making the change.
- Around line 33-78: The test It("should produce a smaller archive on
incremental run with --since") is missing a SpecTimeout and can hang
indefinitely; update the It declaration to include a SpecTimeout option (e.g.
SpecTimeout(5 * time.Minute)) so the long-running sequence of
runner.MirrorToDisk / runner.DiskToMirror calls and registry assertions will
fail fast on hangs; keep the same It body and ensure the SpecTimeout value is
chosen appropriately for the environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e522ea86-201f-48aa-8411-b890c26fde8a
📒 Files selected for processing (3)
tests/integration/helpers_test.gotests/integration/incremental_test.gotests/integration/testdata/imagesetconfigs/incremental/isc-incremental-since.yaml
nidangavali
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@nidangavali: This pull request references CLID-655 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. |
|
/retest |
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks for adding this automated test.
I added few comments, please also have a look on the CodeRabbit suggestions because I think they are valid ones.
Could you please add more details to the PR description following the PULL_REQUEST_TEMPLATE.md we have in our directory .github? It is important for future references.
| incrementalArchives := findTarArchives(workDir) | ||
| incrementalBlobCount := countBlobEntriesInArchives(incrementalArchives) |
There was a problem hiding this comment.
Probably we need to clean the working dir before doing findTarArchives again, otherwise it could lead to wrong results.
The AfterEach runs after each It if I'm not mistaken.
There was a problem hiding this comment.
This file is the same as the one used in the happy path tests, no need to duplicate it.
| func findTarArchives(workDir string) []string { | ||
| matches, err := filepath.Glob(filepath.Join(workDir, "mirror_*.tar")) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(matches).NotTo(BeEmpty(), "no tar archive found in %s", workDir) | ||
| return matches | ||
| } | ||
|
|
||
| // countBlobEntriesInArchives counts the number of blob entries (under docker/registry/v2/blobs/) | ||
| // in the given tar archives. This represents actual image layer data stored in the archive. | ||
| func countBlobEntriesInArchives(tarFiles []string) int { | ||
| blobCount := 0 | ||
| for _, tarFile := range tarFiles { | ||
| entries := listTarEntries(tarFile) | ||
| for _, entry := range entries { | ||
| if strings.Contains(entry, "docker/registry/v2/blobs") { | ||
| blobCount++ | ||
| } | ||
| } | ||
| } | ||
| return blobCount | ||
| } | ||
|
|
||
| // expectNoImageBlobsInArchives verifies that the given tar archives contain no image layer | ||
| // blobs (entries under docker/registry/v2/blobs/). Only repository metadata links and | ||
| // working-dir entries should be present. | ||
| func expectNoImageBlobsInArchives(tarFiles []string) { | ||
| var blobEntries []string | ||
| for _, tarFile := range tarFiles { | ||
| entries := listTarEntries(tarFile) | ||
| for _, entry := range entries { | ||
| if strings.Contains(entry, "docker/registry/v2/blobs") { | ||
| blobEntries = append(blobEntries, entry) | ||
| } | ||
| } | ||
| } | ||
| Expect(blobEntries).To(BeEmpty(), | ||
| "expected no image blob entries in archive, but found %d:\n%s", | ||
| len(blobEntries), strings.Join(blobEntries, "\n")) | ||
| } |
There was a problem hiding this comment.
Since these functions are only used in the incremental mirroring tests, you can put them in the same file (incremental_mirroring_test.go), helpers_test.go is becoming a bit bloated.
|
Also, please add a description to your PRs. It doesn't need to be long, just a couple sentences explaining what you are testing, how, and why. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/incremental_mirroring_test.go (1)
1-1: ⚡ Quick winUpdate comment to match actual filename.
The comment references
incremental_test.gobut the file is namedincremental_mirroring_test.go.📝 Proposed fix
-// incremental_test.go validates the incremental mirroring workflow using the --since flag. +// incremental_mirroring_test.go validates the incremental mirroring workflow using the --since flag.🤖 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 1, Update the file-level comment at the top of incremental_mirroring_test.go so it references the actual filename (incremental_mirroring_test.go) instead of incremental_test.go; locate the header comment string (the first line comment) and change the filename mentioned there to match the file name to keep comments accurate.
🤖 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.
Nitpick comments:
In `@tests/integration/incremental_mirroring_test.go`:
- Line 1: Update the file-level comment at the top of
incremental_mirroring_test.go so it references the actual filename
(incremental_mirroring_test.go) instead of incremental_test.go; locate the
header comment string (the first line comment) and change the filename mentioned
there to match the file name to keep comments accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d1674fea-a555-405f-b5fb-42e7aafeacdd
📒 Files selected for processing (1)
tests/integration/incremental_mirroring_test.go
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks for adding this automated test @nidangavali 🎉
I'm really happy you're contributing to our code base, kudos! Automating our tests is part of sustaining our long term maintainability, so it is a really important task.
From my perspective it will be much more interesting for you generating automated tests than running them manually as we were doing in the past. It will save us a lot of time and you will acquire very important skills, so again thank!
Approved from my side.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adolfo-ab, aguidirh, nidangavali 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
|
/verified by @nidangavali |
openshift-ci-robot
added
the
verified
|
@nidangavali: This PR has been marked as verified by 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. |
|
@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. |
4 participants
Description
Adds a CLI integration test to validate that the --since flag correctly produces incremental archives during mirrorToDisk. The test verifies that when --since is set to a date after the initial mirror, the resulting archive excludes previously cached image layer blobs — containing only repository metadata links.
This ensures the incremental mirroring mechanism works as expected for disconnected environments where operators only need to transfer new content since the last sync.
Github / Jira issue: CLID-655
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Test workflow:
Expected Outcome
Summary by CodeRabbit
--sinceflag. They exercise an initial mirror run and a subsequent incremental run, verifying the incremental archive is smaller and contains no image layer blobs compared to the initial archive.