CLID-197: Add TargetRepo, TargetTag to additionalImages#1327
CLID-197: Add TargetRepo, TargetTag to additionalImages#1327dorzel wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
dorzel
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dorzel: This pull request references CLID-197 which is a valid jira issue. 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. |
dorzel
force-pushed
the
CLID-198
branch
3 times, most recently
from
2d0ff3d to
6553e05
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
| TargetRepo string `json:"targetRepo,omitempty"` | ||
| // TargetTag is the tag the image will be mirrored with. If unset, | ||
| // the image will be mirrored with the provided tag in the Name | ||
| // field or a tag calculated from the partial digest. | ||
| TargetTag string `json:"targetTag,omitempty"` |
There was a problem hiding this comment.
I'm wondering if now that Image struct is becoming more specific for additionalImages if we should have a dedicated struct for BlockedImages. Sharing the same struct betweem them could bring problems in future.
dorzel
force-pushed
the
CLID-198
branch
2 times, most recently
from
b12fb75 to
016fd64
Compare
r4f4
left a comment
r4f4
left a comment
There was a problem hiding this comment.
Can you please add a reference to https://issues.redhat.com/browse/CLID-198 in the commit that implements it? That way we have a link git log -> Jira to know the background of the change.
r4f4
left a comment
r4f4
left a comment
There was a problem hiding this comment.
- Tested with m2m of a release image + delete.
- Tested m2m with following ISC:
kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v2alpha1
mirror:
additionalImages:
- name: registry.redhat.io/ubi9/ubi:latest
targetRepo: redhat-ubi
targetTag: ubi9❯ curl -X GET localhost:5000/v2/redhat-ubi/tags/list
{"name":"redhat-ubi","tags":["sha256-a3fb564e8be461d5bf8344996eb3ef6eb24a4b8c9333053fe4f3e782657591d3.sig","sha256-c8df11b65b18fcf384d5b7fa53e2a92f381a2d9ad78b9de41dc6f2aa161a0e04.sig","sha256-d263526ff62573b43c6c46c5500fe42f91f88b2ce64c455365852a4d6afba942.sig","sha256-d8660bb38670b46155c2384b4a496547e835bed10c5da4636f313181726c97b4.sig","sha256-fdd18b4d04f5882098bebb3ff057b5b6e239721b113b1e22746cd378ca77e8bf.sig","ubi9"]}/lgtm
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks @dorzel, the PR is working as expected, approving it 🎉
Test Steps:
With the following ImageSetConfiguration:
kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v2alpha1
mirror:
additionalImages:
- name: registry.redhat.io/ubi9/ubi:latest
targetRepo: redhat-ubi
targetTag: ubi999
- name: registry.redhat.io/ubi8/ubi:latest@sha256:44d75007b39e0e1bbf1bcfd0721245add54c54c3f83903f8926fb4bef6827aa2
- name: quay.io/prometheus/prometheus:latest
- name: registry.redhat.io/ubi8/ubi:latest
- name: quay.io/rh_ee_aguidi/multi-platform-container:latest
- name: quay.io/rh_ee_aguidi/empty-image:latest
I did m2d/d2m and m2m and got the following results:
Fetching tags for repository: prometheus/prometheus
{
"name": "prometheus/prometheus",
"tags": [
"latest"
]
}
Fetching tags for repository: redhat-ubi
{
"name": "redhat-ubi",
"tags": [
"sha256-a3fb564e8be461d5bf8344996eb3ef6eb24a4b8c9333053fe4f3e782657591d3.sig",
"sha256-c8df11b65b18fcf384d5b7fa53e2a92f381a2d9ad78b9de41dc6f2aa161a0e04.sig",
"sha256-d263526ff62573b43c6c46c5500fe42f91f88b2ce64c455365852a4d6afba942.sig",
"sha256-d8660bb38670b46155c2384b4a496547e835bed10c5da4636f313181726c97b4.sig",
"sha256-fdd18b4d04f5882098bebb3ff057b5b6e239721b113b1e22746cd378ca77e8bf.sig",
"ubi999"
]
}
Fetching tags for repository: rh_ee_aguidi/empty-image
{
"name": "rh_ee_aguidi/empty-image",
"tags": [
"latest",
"sha256-7d27ba1da0f64410102468d138f9ec8f61f2cc23bb58ff4cd63243a4434e3d99.sig"
]
}
Fetching tags for repository: rh_ee_aguidi/multi-platform-container
{
"name": "rh_ee_aguidi/multi-platform-container",
"tags": [
"latest",
"sha256-02f29c270f30416a266571383098d7b98a49488723087fd917128045bcd1ca75.sig",
"sha256-0de0e983a4980f32b1aad2d7f7f387cea2d4e9517b47f336cef27f63735911fa.sig",
"sha256-832f20ad3d7e687c581b0a7d483174901d8bf22bb96c981b3f9da452817a754e.sig",
"sha256-b15a2f174d803fd5fd7db0b3969c75cee0fe9131e0d8478f8c70ac01a4534869.sig",
"sha256-e033aa62f84267cf44de611acac2e76bfa4d2f0b6b2b61f1c4fecbefefde7159.sig"
]
}
Fetching tags for repository: ubi8/ubi
{
"name": "ubi8/ubi",
"tags": [
"latest",
"sha256-0a95276776dc776fc42ebaa0ce79aa0b3ac72ab762df1d262beef7b092c2b23e.sig",
"sha256-0f4503f709fd8f7a2d85049dfd2068f1bf68cd0aae0504bcab677387e455794b.sig",
"sha256-44d75007b39e0e1bbf1bcfd0721245add54c54c3f83903f8926fb4bef6827aa2.sig",
"sha256-9bad9c04ec49b48a6ca01b55e7dd6a451010b8b438906af78cb1a1ccc3cb72a3.sig",
"sha256-f2ae71354f94af134caa8fc6ff9b01bbc974b13c1c783c17927211750bf2c04b.sig"
]
}
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adolfo-ab, aguidirh, dorzel 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
|
New changes are detected. LGTM label has been removed. |
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
@dorzel: This pull request references CLID-197 which is a valid jira issue. 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. |
![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)
internal/pkg/api/v2alpha1/type_config_test.go (1)
85-97: Add a digest+targetTagregression case.
TestGetUniqueNameWithTargetcurrently covers digest + targetPath but not digest + targetTag. Adding that case would lock in expected behavior and catch the current rewrite bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/api/v2alpha1/type_config_test.go` around lines 85 - 97, Add a new test case to TestGetUniqueNameWithTarget that covers a source image with a digest and a non-empty targetTag (e.g., name "Valid/WithDigestAndTargetTag", sourceName set to the sha256 digest form, targetTag set to "new-tag", targetPath left empty) and assert the expected string keeps the original digest unchanged (i.e., targetTag should not replace or remove the digest); update the test table entries for sourceName, targetTag, targetPath, and expected to lock in this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/pkg/api/v2alpha1/type_config.go`:
- Around line 206-210: The code replaces imgSpec.Tag in imgSpec.Reference and
imgSpec.ReferenceWithTransport, which breaks for digest-only images where
imgSpec.Tag == ""; instead, when targetTag != "" reconstruct the references from
imgSpec.Name (and the transport prefix if present) using ":" + targetTag (e.g.,
imgSpec.Reference = imgSpec.Name + ":" + targetTag and similarly build
imgSpec.ReferenceWithTransport), update imgSpec.Tag = targetTag, and clear any
digest-related fields (e.g., imgSpec.Digest and imgSpec.DigestReference) so the
spec no longer carries the old digest; also add a unit test covering the
digest-only + targetTag case to prevent regression.
---
Nitpick comments:
In `@internal/pkg/api/v2alpha1/type_config_test.go`:
- Around line 85-97: Add a new test case to TestGetUniqueNameWithTarget that
covers a source image with a digest and a non-empty targetTag (e.g., name
"Valid/WithDigestAndTargetTag", sourceName set to the sha256 digest form,
targetTag set to "new-tag", targetPath left empty) and assert the expected
string keeps the original digest unchanged (i.e., targetTag should not replace
or remove the digest); update the test table entries for sourceName, targetTag,
targetPath, and expected to lock in this behavior.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
internal/pkg/additional/const.gointernal/pkg/additional/local_stored_collector.gointernal/pkg/additional/local_stored_collector_test.gointernal/pkg/api/v2alpha1/type_config.gointernal/pkg/api/v2alpha1/type_config_test.gointernal/pkg/config/validate_test.gointernal/pkg/delete/const.gointernal/pkg/delete/delete_images.gointernal/pkg/delete/delete_images_test.go
|
@dorzel: This pull request references CLID-197 which is a valid jira issue. 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. |
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)
internal/pkg/delete/delete_images.go (1)
159-162:⚠️ Potential issue | 🟡 MinorTypo in error message.
"parse imag eref" should be "parse image ref".
✏️ Suggested fix
imgSpecRef, err := image.ParseRef(img.ImageReference) if err != nil { - allErrs = append(allErrs, fmt.Errorf("parse imag eref %q: %w", img.ImageReference, err)) + allErrs = append(allErrs, fmt.Errorf("parse image ref %q: %w", img.ImageReference, err)) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/delete/delete_images.go` around lines 159 - 162, Fix the typo in the error message produced when parsing image references in delete_images.go: update the fmt.Errorf call inside the block that handles the error from image.ParseRef(img.ImageReference) (the append to allErrs) to use the correct text "parse image ref" instead of "parse imag eref" so the error reads fmt.Errorf("parse image ref %q: %w", img.ImageReference, err).
🧹 Nitpick comments (3)
internal/pkg/image/image_test.go (1)
320-334: Error messages reference wrong function name.The error messages in lines 324 and 328 mention
ParseRef()but this test is exercisingWithMaxNestedPaths. This could cause confusion during test debugging.💡 Suggested fix
for _, aTestCase := range testCases { t.Run(aTestCase.caseName, func(t *testing.T) { outRef, err := WithMaxNestedPaths(aTestCase.imgRef, aTestCase.maxNestedPaths) if aTestCase.expectedError != "" && err == nil { - t.Errorf("ParseRef() expected to fail for %q: got %v, want %v", aTestCase.imgRef, err, aTestCase.expectedError) + t.Errorf("WithMaxNestedPaths() expected to fail for %q: got %v, want %v", aTestCase.imgRef, err, aTestCase.expectedError) } if err != nil { if aTestCase.expectedError != err.Error() { - t.Errorf("ParseRef() returned unexpected error for %q: got %v, want %v", aTestCase.imgRef, err, aTestCase.expectedError) + t.Errorf("WithMaxNestedPaths() returned unexpected error for %q: got %v, want %v", aTestCase.imgRef, err, aTestCase.expectedError) } } else { require.Equal(t, aTestCase.expectedOutRef, outRef) } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/image/image_test.go` around lines 320 - 334, The test error messages incorrectly reference ParseRef(); update the assertions in the table-driven test loop so they mention WithMaxNestedPaths instead — change the message strings used in t.Errorf calls (the two occurrences that currently say "ParseRef()") to "WithMaxNestedPaths()" so failures correctly identify the function under test (look for the t.Run loop that calls WithMaxNestedPaths with aTestCase.imgRef and aTestCase.maxNestedPaths).internal/pkg/delete/delete_images_test.go (1)
332-333: Redundant cleanup -t.TempDir()auto-cleans.
t.TempDir()automatically removes the directory and its contents when the test finishes. The explicitdefer os.RemoveAll(tempDir)is unnecessary.🧹 Suggested fix
tempDir := t.TempDir() - defer os.RemoveAll(tempDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/delete/delete_images_test.go` around lines 332 - 333, The test creates tempDir with t.TempDir() and then redundantly calls defer os.RemoveAll(tempDir); remove the explicit cleanup by deleting the defer os.RemoveAll(tempDir) statement so the test relies on t.TempDir()'s automatic cleanup (identify the tempDir variable and the defer os.RemoveAll(tempDir) call in the test).internal/pkg/additional/local_stored_collector.go (1)
122-127: Inconsistent condition between helper methods.In
buildMirrorToDiskPaths(line 123), the condition checksimg.TargetTag != "", while the analogous code inbuildDiskToMirrorPaths(line 155) checkstargetTag != "".For OCI images,
targetTagis derived fromimgSpec.Tag(typically empty for OCI) orimg.TargetTag. The current logic inbuildMirrorToDiskPathsignoresimgSpec.Tagfor OCI and only uses the explicit override, which is the intended behavior per the comment. Consider aligningbuildDiskToMirrorPathsfor consistency.♻️ Suggested alignment for `buildDiskToMirrorPaths`
if imgSpec.Transport != consts.DockerProtocol { // oci image tag := latestTag - if targetTag != "" { + if img.TargetTag != "" { tag = targetTag }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/additional/local_stored_collector.go` around lines 122 - 127, In buildMirrorToDiskPaths the tag override checks img.TargetTag != "" but buildDiskToMirrorPaths uses targetTag != "", causing inconsistent behavior for OCI images; update buildDiskToMirrorPaths to use img.TargetTag (or the same explicit-override check used in buildMirrorToDiskPaths) when deciding between latestTag and targetTag so both functions derive tmpDest the same way (reference buildMirrorToDiskPaths, buildDiskToMirrorPaths, img.TargetTag, targetTag, imgSpec.Tag, latestTag, tmpDest).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/pkg/delete/delete_images.go`:
- Around line 159-162: Fix the typo in the error message produced when parsing
image references in delete_images.go: update the fmt.Errorf call inside the
block that handles the error from image.ParseRef(img.ImageReference) (the append
to allErrs) to use the correct text "parse image ref" instead of "parse imag
eref" so the error reads fmt.Errorf("parse image ref %q: %w",
img.ImageReference, err).
---
Nitpick comments:
In `@internal/pkg/additional/local_stored_collector.go`:
- Around line 122-127: In buildMirrorToDiskPaths the tag override checks
img.TargetTag != "" but buildDiskToMirrorPaths uses targetTag != "", causing
inconsistent behavior for OCI images; update buildDiskToMirrorPaths to use
img.TargetTag (or the same explicit-override check used in
buildMirrorToDiskPaths) when deciding between latestTag and targetTag so both
functions derive tmpDest the same way (reference buildMirrorToDiskPaths,
buildDiskToMirrorPaths, img.TargetTag, targetTag, imgSpec.Tag, latestTag,
tmpDest).
In `@internal/pkg/delete/delete_images_test.go`:
- Around line 332-333: The test creates tempDir with t.TempDir() and then
redundantly calls defer os.RemoveAll(tempDir); remove the explicit cleanup by
deleting the defer os.RemoveAll(tempDir) statement so the test relies on
t.TempDir()'s automatic cleanup (identify the tempDir variable and the defer
os.RemoveAll(tempDir) call in the test).
In `@internal/pkg/image/image_test.go`:
- Around line 320-334: The test error messages incorrectly reference ParseRef();
update the assertions in the table-driven test loop so they mention
WithMaxNestedPaths instead — change the message strings used in t.Errorf calls
(the two occurrences that currently say "ParseRef()") to "WithMaxNestedPaths()"
so failures correctly identify the function under test (look for the t.Run loop
that calls WithMaxNestedPaths with aTestCase.imgRef and
aTestCase.maxNestedPaths).
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (10)
internal/pkg/additional/const.gointernal/pkg/additional/local_stored_collector.gointernal/pkg/additional/local_stored_collector_test.gointernal/pkg/api/v2alpha1/type_config.gointernal/pkg/api/v2alpha1/type_config_test.gointernal/pkg/config/validate_test.gointernal/pkg/delete/const.gointernal/pkg/delete/delete_images.gointernal/pkg/delete/delete_images_test.gointernal/pkg/image/image_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/pkg/additional/local_stored_collector_test.go
- internal/pkg/additional/const.go
- internal/pkg/config/validate_test.go
- internal/pkg/delete/const.go
- internal/pkg/api/v2alpha1/type_config.go
|
/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. |
openshift-ci-robot
removed
the
verified
|
@dorzel: This pull request references CLID-197 which is a valid jira issue. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/pkg/delete/delete_images.go (1)
190-192: Improve default-path mismatch diagnostics.The default-case error omits the offending parsed name, which makes triage harder compared to the release-specific branches.
♻️ Suggested improvement
- allErrs = append(allErrs, fmt.Errorf("delete destination %s does not match values found in the delete-images yaml file (please verify full name)", o.Opts.Global.DeleteDestination)) + allErrs = append(allErrs, fmt.Errorf( + "delete destination %s does not match values found in the delete-images yaml file (expected prefix %s/, got %s)", + o.Opts.Global.DeleteDestination, deleteDestBase, imgSpecRef.Name, + ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/delete/delete_images.go` around lines 190 - 192, The error appended to allErrs when the parsed image name doesn't start with deleteDestBase should include the actual offending parsed name to aid diagnostics; update the fmt.Errorf call in the branch that checks if !strings.HasPrefix(imgSpecRef.Name, deleteDestBase+"/") so it references imgSpecRef.Name (and keep context by retaining o.Opts.Global.DeleteDestination in the message) so logs show both the expected delete destination and the offending parsed name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/pkg/additional/local_stored_collector.go`:
- Around line 57-58: The current use of the raw source name (`origin :=
img.Name` in internal/pkg/additional/local_stored_collector.go) causes different
target mappings to share the same Origin and be deduplicated later in
processDeleteItems; update the Origin to be destination-aware by incorporating
the target mapping (e.g., append or format in the targetRepo/targetTag or a
mapping ID) when constructing origin for each img so each mirrored output yields
a unique img.Origin; ensure the change is applied wherever `origin := img.Name`
is set (including the second occurrence noted) and remains compatible with the
delete pipeline which reads img.Origin in processDeleteItems.
---
Nitpick comments:
In `@internal/pkg/delete/delete_images.go`:
- Around line 190-192: The error appended to allErrs when the parsed image name
doesn't start with deleteDestBase should include the actual offending parsed
name to aid diagnostics; update the fmt.Errorf call in the branch that checks if
!strings.HasPrefix(imgSpecRef.Name, deleteDestBase+"/") so it references
imgSpecRef.Name (and keep context by retaining o.Opts.Global.DeleteDestination
in the message) so logs show both the expected delete destination and the
offending parsed name.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (10)
internal/pkg/additional/const.gointernal/pkg/additional/local_stored_collector.gointernal/pkg/additional/local_stored_collector_test.gointernal/pkg/api/v2alpha1/type_config.gointernal/pkg/api/v2alpha1/type_config_test.gointernal/pkg/config/validate_test.gointernal/pkg/delete/const.gointernal/pkg/delete/delete_images.gointernal/pkg/delete/delete_images_test.gointernal/pkg/image/image_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/pkg/delete/const.go
- internal/pkg/image/image_test.go
|
@dorzel: This pull request references CLID-197 which is a valid jira issue. 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/pkg/delete/delete_images_test.go (2)
445-458: Isolate metadata state per subtest to prevent order-coupled behavior.At Line [447]-Line [452], all write/read cases share the same
di+ working directory. Spinning up a freshDeleteImagesper subtest makes this future-proof and deterministic.Suggested refactor
for _, tt := range writeMetadataTests { + tt := tt t.Run("WriteDeleteMetaData "+tt.name, func(t *testing.T) { - err := di.WriteDeleteMetaData(context.Background(), tt.cpImages) + caseDir := t.TempDir() + localGlobal := *global + localGlobal.WorkingDir = caseDir + localOpts := opts + localOpts.Global = &localGlobal + localDI := New(log, localOpts, &mockBatch{}, &mockBlobs{}, cfg, &mockManifest{}, "/tmp", &mockSignatureHandler{}) + + err := localDI.WriteDeleteMetaData(context.Background(), tt.cpImages) assert.NoError(t, err) - data, err := di.ReadDeleteMetaData() + data, err := localDI.ReadDeleteMetaData() assert.NoError(t, err) assert.Equal(t, tt.expectedItemCount, len(data.Items)) if tt.expectedItemCount == 1 { assert.Equal(t, tt.expectedImageRef, data.Items[0].ImageReference) assert.Equal(t, tt.expectedOrigin, data.Items[0].ImageName) } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/delete/delete_images_test.go` around lines 445 - 458, The tests share a single DeleteImages instance (di) and working directory across subtests causing order-dependent failures; for each subtest in the loop that calls WriteDeleteMetaData and ReadDeleteMetaData, instantiate a fresh DeleteImages (e.g., create a new di via the constructor used elsewhere) and give it an isolated working directory (use t.TempDir() or otherwise unique path) before calling di.WriteDeleteMetaData/di.ReadDeleteMetaData so each subtest has independent state and deterministic results.
425-442: Strengthen the multi-target assertion beyond count-only checks.For the
"same origin with different targets should not be deduped"case, asserting onlylen(data.Items) == 2can miss wrong-entry regressions. Consider asserting both expected destination refs are present.Also applies to: 453-456
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/delete/delete_images_test.go` around lines 425 - 442, The test case "same origin with different targets should not be deduped" currently only asserts expectedItemCount (len(data.Items) == 2); update the assertion to verify the actual destination refs are present as well: iterate data.Items (or extract their Destination/Ref field) and assert that both "docker://localhost:5000/myregistry/repo-a/ubi:latest" and "docker://localhost:5000/myregistry/repo-b/ubi:latest" appear, similarly strengthen the other case at the second occurrence (around the block referencing cpImages, CopyImageSchema and expectedItemCount) to avoid count-only false positives. Ensure you reference data.Items and the Destination (or Ref) property when checking presence.internal/pkg/additional/local_stored_collector.go (1)
81-90: Prefer explicit unsupported-mode handling in the switch.Right now unknown mode falls through to a generic src/dst error. A
defaultbranch with mode context gives clearer diagnostics and keeps intent explicit.Suggested change
switch { case o.Opts.IsMirrorToDisk(), o.Opts.IsMirrorToMirror(): tmpSrc, tmpDest = o.buildMirrorToDiskPaths(img, imgSpec, targetRepo, targetTag) case o.Opts.IsDiskToMirror(), o.Opts.IsDelete(): tmpSrc, tmpDest = o.buildDiskToMirrorPaths(img, imgSpec, targetRepo, targetTag) + default: + return allImages, fmt.Errorf("unsupported copy mode %q for additional image %s", o.Opts.Mode, img.Name) } if tmpSrc == "" || tmpDest == "" { o.Log.Error(collectorPrefix+"unable to determine src %s or dst %s for %s", tmpSrc, tmpDest, img.Name) return allImages, fmt.Errorf("unable to determine src %s or dst %s for %s", tmpSrc, tmpDest, img.Name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/additional/local_stored_collector.go` around lines 81 - 90, The switch that chooses tmpSrc/tmpDest in the method should include an explicit default branch to handle unsupported/unknown modes instead of falling through to the generic src/dst check; add a default case in the switch (around the block that calls o.buildMirrorToDiskPaths and o.buildDiskToMirrorPaths) that logs via o.Log.Error (include collectorPrefix and the current mode/state from o.Opts) and returns a descriptive error (use fmt.Errorf) identifying the unsupported mode and the image (img.Name) so callers get immediate, contextual diagnostics rather than the later empty-path error.internal/pkg/api/v2alpha1/type_config_test.go (1)
155-174: Reduce brittleness in invalid-path error assertions.At Line [159], Line [166], and Line [173], the expected error substring is very long and tightly coupled to full wording/docs URL. Consider asserting only stable invariants (e.g.,
invalid target path componentplus the offending value) to avoid noisy test breakage from message text churn.Also applies to: 187-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/api/v2alpha1/type_config_test.go` around lines 155 - 174, The test assertions are brittle because expError contains the full error text and docs URL; update the cases for "Invalid/TargetPathWithTag", "Invalid/TargetPathWithDigest", "Invalid/TargetPathWithInvalidCharacters" (and the similar cases around lines 187-190) to assert only stable invariants: that the produced error contains the substring "invalid target path component" and the offending targetPath value (use the expError field or change the assertion to strings.Contains(err.Error(), targetPath) && strings.Contains(err.Error(), "invalid target path component")). Locate the table entries with the expError field and adjust the test assertion logic to check these minimal substrings instead of the entire message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/pkg/additional/local_stored_collector.go`:
- Around line 81-90: The switch that chooses tmpSrc/tmpDest in the method should
include an explicit default branch to handle unsupported/unknown modes instead
of falling through to the generic src/dst check; add a default case in the
switch (around the block that calls o.buildMirrorToDiskPaths and
o.buildDiskToMirrorPaths) that logs via o.Log.Error (include collectorPrefix and
the current mode/state from o.Opts) and returns a descriptive error (use
fmt.Errorf) identifying the unsupported mode and the image (img.Name) so callers
get immediate, contextual diagnostics rather than the later empty-path error.
In `@internal/pkg/api/v2alpha1/type_config_test.go`:
- Around line 155-174: The test assertions are brittle because expError contains
the full error text and docs URL; update the cases for
"Invalid/TargetPathWithTag", "Invalid/TargetPathWithDigest",
"Invalid/TargetPathWithInvalidCharacters" (and the similar cases around lines
187-190) to assert only stable invariants: that the produced error contains the
substring "invalid target path component" and the offending targetPath value
(use the expError field or change the assertion to strings.Contains(err.Error(),
targetPath) && strings.Contains(err.Error(), "invalid target path component")).
Locate the table entries with the expError field and adjust the test assertion
logic to check these minimal substrings instead of the entire message.
In `@internal/pkg/delete/delete_images_test.go`:
- Around line 445-458: The tests share a single DeleteImages instance (di) and
working directory across subtests causing order-dependent failures; for each
subtest in the loop that calls WriteDeleteMetaData and ReadDeleteMetaData,
instantiate a fresh DeleteImages (e.g., create a new di via the constructor used
elsewhere) and give it an isolated working directory (use t.TempDir() or
otherwise unique path) before calling
di.WriteDeleteMetaData/di.ReadDeleteMetaData so each subtest has independent
state and deterministic results.
- Around line 425-442: The test case "same origin with different targets should
not be deduped" currently only asserts expectedItemCount (len(data.Items) == 2);
update the assertion to verify the actual destination refs are present as well:
iterate data.Items (or extract their Destination/Ref field) and assert that both
"docker://localhost:5000/myregistry/repo-a/ubi:latest" and
"docker://localhost:5000/myregistry/repo-b/ubi:latest" appear, similarly
strengthen the other case at the second occurrence (around the block referencing
cpImages, CopyImageSchema and expectedItemCount) to avoid count-only false
positives. Ensure you reference data.Items and the Destination (or Ref) property
when checking presence.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (10)
internal/pkg/additional/const.gointernal/pkg/additional/local_stored_collector.gointernal/pkg/additional/local_stored_collector_test.gointernal/pkg/api/v2alpha1/type_config.gointernal/pkg/api/v2alpha1/type_config_test.gointernal/pkg/config/validate_test.gointernal/pkg/delete/const.gointernal/pkg/delete/delete_images.gointernal/pkg/delete/delete_images_test.gointernal/pkg/image/image_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/pkg/image/image_test.go
- internal/pkg/delete/delete_images.go
- internal/pkg/api/v2alpha1/type_config.go
- internal/pkg/config/validate_test.go
- internal/pkg/additional/const.go
|
@dorzel: 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. |
6 participants
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
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
New Features
Improvements
Bug Fixes
Tests