[release-4.22] OCPBUGS-98716: Include index image sub-digests in dry run mapping.txt#1474
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@dorzel: This pull request references Jira Issue OCPBUGS-66263, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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
Bot
added
the
needs-rebase
WalkthroughThe PR adds manifest-list sub-digest support to dry-run mappings, moves operator catalog digest pinning earlier in M2D/M2M workflows, migrates blob and history tracking to typed sets, and includes tag-and-digest images in digest-only mirror generation. Changesoc-mirror functional changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Executor
participant DryRun
participant Manifest
participant MappingFile
Executor->>DryRun: enable manifest-list dry run
DryRun->>Manifest: retrieve manifest-list digests
Manifest-->>DryRun: return instance digests
DryRun->>MappingFile: write base and digest-pinned mappings
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 11❌ Failed checks (1 warning, 10 inconclusive)
✅ Passed checks (4 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 |
|
@dorzel: This pull request references Jira Issue OCPBUGS-66263, which is invalid:
Comment 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: 7
🧹 Nitpick comments (2)
internal/pkg/history/history.go (1)
154-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnsure deterministic history output.
Iterating over a set (
mapunder the hood) produces an unpredictable order on every run. Consider sorting the blobs to make the written history file deterministic, which improves reproducibility and simplifies diffing if these files are archived.♻️ Proposed fix
- for blob := range historyBlobs { + for _, blob := range sets.List(historyBlobs) { _, err := writer.WriteString(blob + "\n") if err != nil { return historyBlobs, fmt.Errorf("unable to write to history file: %w", err) } }🤖 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/history/history.go` around lines 154 - 159, Sort the history blobs before the write loop so output from the history-writing flow is deterministic. Update the iteration around historyBlobs to use the sorted order while preserving the existing writer.WriteString error handling and return behavior.internal/pkg/archive/archive.go (1)
160-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid O(N²) allocations by inserting elements directly instead of using
Unioninside loops.Using
sets.Unioninside a loop allocates a new set and copies all elements on every iteration, leading to quadratic time complexity and unnecessary memory churn. Iterate over the new elements and insert them into the accumulator set directly.
internal/pkg/archive/archive.go#L160-L160: Replace with a loopfor blob := range addedBlobs { allAddedBlobs.Insert(blob) }.internal/pkg/archive/image-blob-gatherer.go#L115-L115: Replace with a loopfor blob := range singleArchBlobs { blobs.Insert(blob) }.🤖 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.go` at line 160, The archive blob accumulation in internal/pkg/archive/archive.go lines 160-160 should iterate over addedBlobs and insert each blob into allAddedBlobs directly instead of calling Union. Apply the same change in internal/pkg/archive/image-blob-gatherer.go lines 115-115 by iterating over singleArchBlobs and inserting each blob into blobs; preserve the existing accumulator behavior without allocating a new set per iteration.
🤖 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 `@go.mod`:
- Line 10: Revert the manual version change for the indirect
github.com/go-jose/go-jose/v4 dependency in go.mod. Identify and upgrade the
direct dependency that introduces it, then run go mod tidy to resolve the
indirect version automatically.
In `@internal/pkg/cli/dryrun.go`:
- Around line 141-199: Bound both new registry-inspection paths with a finite
timeout, using the existing checkRegistryAccess timeout pattern. In
internal/pkg/cli/dryrun.go lines 141-199, update inspectManifestLists and its
GetManifestListDigests call after newSystemContextForSource to use a
timeout-derived context and cancel it; in internal/pkg/cli/executor.go lines
581-588, replace context.Background() before config.PinCatalogDigests with the
command context when available and derive a bounded timeout context.
- Around line 59-67: Update the availability log condition in the dry-run flow
to emit the “all images available” message only when no images are missing,
while preserving the existing message when all required images are cached. Use
the surrounding nbMissingImgs and nbAvailableImgs checks in the method as the
change point.
- Around line 107-139: Update writeSubDigestEntries to derive sourceBase by
parsing img.Source with the same image.ParseRef approach used by
subDigestDestination, ensuring both tags and existing digests are removed before
appending `@digest`. Preserve the current fallback behavior for parse failures and
keep subDigestDestination unchanged.
In `@internal/pkg/cli/executor.go`:
- Line 299: Update the help text for the IsDryRunManifestLists flag registration
to state that this option enables dry-run behavior automatically and must not be
used together with an explicit --dry-run flag; remove the self-contradictory
wording while preserving the existing flag semantics.
- Around line 1383-1384: Update the comment above
ExecutorSchema.createConfigsWithPinnedCatalogs to reference the actual
config.PinCatalogDigests call used at workflow start, replacing the nonexistent
pinOperatorCatalogs name while preserving the M2D/M2M context.
In `@internal/pkg/config/pin_catalogs.go`:
- Around line 135-156: Move the TargetTag preservation block in the catalog
pinning flow after catalogDigest is confirmed non-empty and the empty-digest
early return in the digest resolution logic. Keep the existing condition and log
message, ensuring op.TargetTag is not mutated when resolution fails or pinning
is skipped.
---
Nitpick comments:
In `@internal/pkg/archive/archive.go`:
- Line 160: The archive blob accumulation in internal/pkg/archive/archive.go
lines 160-160 should iterate over addedBlobs and insert each blob into
allAddedBlobs directly instead of calling Union. Apply the same change in
internal/pkg/archive/image-blob-gatherer.go lines 115-115 by iterating over
singleArchBlobs and inserting each blob into blobs; preserve the existing
accumulator behavior without allocating a new set per iteration.
In `@internal/pkg/history/history.go`:
- Around line 154-159: Sort the history blobs before the write loop so output
from the history-writing flow is deterministic. Update the iteration around
historyBlobs to use the sorted order while preserving the existing
writer.WriteString error handling and return 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/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e93bc181-f07c-4efc-8ebf-76cb0137667b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
go.modinternal/pkg/additional/local_stored_collector_test.gointernal/pkg/archive/archive.gointernal/pkg/archive/archive_test.gointernal/pkg/archive/image-blob-gatherer.gointernal/pkg/archive/image-blob-gatherer_test.gointernal/pkg/archive/interface.gointernal/pkg/cli/dryrun.gointernal/pkg/cli/dryrun_test.gointernal/pkg/cli/executor.gointernal/pkg/clusterresources/clusterresources.gointernal/pkg/clusterresources/clusterresources_test.gointernal/pkg/config/pin_catalogs.gointernal/pkg/config/pin_catalogs_test.gointernal/pkg/delete/delete_images_test.gointernal/pkg/history/history.gointernal/pkg/history/history_test.gointernal/pkg/history/interface.gointernal/pkg/manifest/interface.gointernal/pkg/manifest/manifest.gointernal/pkg/mirror/options.gointernal/pkg/operator/filtered_collector.gointernal/pkg/operator/filtered_collector_test.gointernal/pkg/release/cincinnati_test.gointernal/pkg/release/local_stored_collector_test.gointernal/pkg/signature/cosign_tag_based_signatures_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🧹 Nitpick comments (2)
internal/pkg/history/history.go (1)
154-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnsure deterministic history output.
Iterating over a set (
mapunder the hood) produces an unpredictable order on every run. Consider sorting the blobs to make the written history file deterministic, which improves reproducibility and simplifies diffing if these files are archived.♻️ Proposed fix
- for blob := range historyBlobs { + for _, blob := range sets.List(historyBlobs) { _, err := writer.WriteString(blob + "\n") if err != nil { return historyBlobs, fmt.Errorf("unable to write to history file: %w", err) } }🤖 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/history/history.go` around lines 154 - 159, Sort the history blobs before the write loop so output from the history-writing flow is deterministic. Update the iteration around historyBlobs to use the sorted order while preserving the existing writer.WriteString error handling and return behavior.internal/pkg/archive/archive.go (1)
160-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid O(N²) allocations by inserting elements directly instead of using
Unioninside loops.Using
sets.Unioninside a loop allocates a new set and copies all elements on every iteration, leading to quadratic time complexity and unnecessary memory churn. Iterate over the new elements and insert them into the accumulator set directly.
internal/pkg/archive/archive.go#L160-L160: Replace with a loopfor blob := range addedBlobs { allAddedBlobs.Insert(blob) }.internal/pkg/archive/image-blob-gatherer.go#L115-L115: Replace with a loopfor blob := range singleArchBlobs { blobs.Insert(blob) }.🤖 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.go` at line 160, The archive blob accumulation in internal/pkg/archive/archive.go lines 160-160 should iterate over addedBlobs and insert each blob into allAddedBlobs directly instead of calling Union. Apply the same change in internal/pkg/archive/image-blob-gatherer.go lines 115-115 by iterating over singleArchBlobs and inserting each blob into blobs; preserve the existing accumulator behavior without allocating a new set per iteration.
🤖 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 `@go.mod`:
- Line 10: Revert the manual version change for the indirect
github.com/go-jose/go-jose/v4 dependency in go.mod. Identify and upgrade the
direct dependency that introduces it, then run go mod tidy to resolve the
indirect version automatically.
In `@internal/pkg/cli/dryrun.go`:
- Around line 141-199: Bound both new registry-inspection paths with a finite
timeout, using the existing checkRegistryAccess timeout pattern. In
internal/pkg/cli/dryrun.go lines 141-199, update inspectManifestLists and its
GetManifestListDigests call after newSystemContextForSource to use a
timeout-derived context and cancel it; in internal/pkg/cli/executor.go lines
581-588, replace context.Background() before config.PinCatalogDigests with the
command context when available and derive a bounded timeout context.
- Around line 59-67: Update the availability log condition in the dry-run flow
to emit the “all images available” message only when no images are missing,
while preserving the existing message when all required images are cached. Use
the surrounding nbMissingImgs and nbAvailableImgs checks in the method as the
change point.
- Around line 107-139: Update writeSubDigestEntries to derive sourceBase by
parsing img.Source with the same image.ParseRef approach used by
subDigestDestination, ensuring both tags and existing digests are removed before
appending `@digest`. Preserve the current fallback behavior for parse failures and
keep subDigestDestination unchanged.
In `@internal/pkg/cli/executor.go`:
- Line 299: Update the help text for the IsDryRunManifestLists flag registration
to state that this option enables dry-run behavior automatically and must not be
used together with an explicit --dry-run flag; remove the self-contradictory
wording while preserving the existing flag semantics.
- Around line 1383-1384: Update the comment above
ExecutorSchema.createConfigsWithPinnedCatalogs to reference the actual
config.PinCatalogDigests call used at workflow start, replacing the nonexistent
pinOperatorCatalogs name while preserving the M2D/M2M context.
In `@internal/pkg/config/pin_catalogs.go`:
- Around line 135-156: Move the TargetTag preservation block in the catalog
pinning flow after catalogDigest is confirmed non-empty and the empty-digest
early return in the digest resolution logic. Keep the existing condition and log
message, ensuring op.TargetTag is not mutated when resolution fails or pinning
is skipped.
---
Nitpick comments:
In `@internal/pkg/archive/archive.go`:
- Line 160: The archive blob accumulation in internal/pkg/archive/archive.go
lines 160-160 should iterate over addedBlobs and insert each blob into
allAddedBlobs directly instead of calling Union. Apply the same change in
internal/pkg/archive/image-blob-gatherer.go lines 115-115 by iterating over
singleArchBlobs and inserting each blob into blobs; preserve the existing
accumulator behavior without allocating a new set per iteration.
In `@internal/pkg/history/history.go`:
- Around line 154-159: Sort the history blobs before the write loop so output
from the history-writing flow is deterministic. Update the iteration around
historyBlobs to use the sorted order while preserving the existing
writer.WriteString error handling and return 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/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e93bc181-f07c-4efc-8ebf-76cb0137667b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
go.modinternal/pkg/additional/local_stored_collector_test.gointernal/pkg/archive/archive.gointernal/pkg/archive/archive_test.gointernal/pkg/archive/image-blob-gatherer.gointernal/pkg/archive/image-blob-gatherer_test.gointernal/pkg/archive/interface.gointernal/pkg/cli/dryrun.gointernal/pkg/cli/dryrun_test.gointernal/pkg/cli/executor.gointernal/pkg/clusterresources/clusterresources.gointernal/pkg/clusterresources/clusterresources_test.gointernal/pkg/config/pin_catalogs.gointernal/pkg/config/pin_catalogs_test.gointernal/pkg/delete/delete_images_test.gointernal/pkg/history/history.gointernal/pkg/history/history_test.gointernal/pkg/history/interface.gointernal/pkg/manifest/interface.gointernal/pkg/manifest/manifest.gointernal/pkg/mirror/options.gointernal/pkg/operator/filtered_collector.gointernal/pkg/operator/filtered_collector_test.gointernal/pkg/release/cincinnati_test.gointernal/pkg/release/local_stored_collector_test.gointernal/pkg/signature/cosign_tag_based_signatures_test.go
🛑 Comments failed to post (7)
go.mod (1)
10-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid manually updating indirect dependencies.
As per learnings, in
go.modfiles, do not manually upgrade entries marked as indirect dependencies. Instead, upgrade the direct dependencies that introduce those indirect requirements, then rungo mod tidyso the indirect versions are updated automatically.🤖 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 `@go.mod` at line 10, Revert the manual version change for the indirect github.com/go-jose/go-jose/v4 dependency in go.mod. Identify and upgrade the direct dependency that introduces it, then run go mod tidy to resolve the indirect version automatically.Source: Learnings
internal/pkg/cli/dryrun.go (3)
59-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
"All images available" message can be misleading in mixed missing/available runs.
This log fires whenever
nbAvailableImgs > 0, even whennbMissingImgs > 0in the same run, printing a contradictory "all %d images required for mirroring are available in local cache" right alongside the missing-images warning.💬 Suggested fix
- if nbAvailableImgs > 0 { + if nbAvailableImgs > 0 && nbMissingImgs == 0 { o.Log.Info("all %d images required for mirroring are available in local cache. You may proceed with mirroring from disk to disconnected registry", nbAvailableImgs) }📝 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.if nbMissingImgs > 0 { if err := o.writeMissingImagesFile(outDir, missingImgsBuff.Bytes(), nbMissingImgs, len(allImages)); err != nil { return err } } if nbAvailableImgs > 0 && nbMissingImgs == 0 { o.Log.Info("all %d images required for mirroring are available in local cache. You may proceed with mirroring from disk to disconnected registry", nbAvailableImgs) }🤖 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/dryrun.go` around lines 59 - 67, Update the availability log condition in the dry-run flow to emit the “all images available” message only when no images are missing, while preserving the existing message when all required images are cached. Use the surrounding nbMissingImgs and nbAvailableImgs checks in the method as the change point.
107-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'Files of interest:\n' git ls-files 'internal/pkg/cli/dryrun.go' 'go.mod' | cat printf '\nSearch for relevant symbols/usages:\n' rg -n --no-heading 'writeSubDigestEntries|subDigestDestination|manifestListDigests|mapping\.txt|subDigestEntry|CopyImageSchema' internal/pkg -S printf '\nOutline dryrun.go:\n' ast-grep outline internal/pkg/cli/dryrun.go --view expanded || trueRepository: openshift/oc-mirror
Length of output: 1924
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'Read relevant sections of internal/pkg/cli/dryrun.go:\n' nl -ba internal/pkg/cli/dryrun.go | sed -n '1,220p' printf '\nInspect go.mod for containers/image version:\n' rg -n --no-heading 'containers/image|github.com/containers/image' go.mod -S || trueRepository: openshift/oc-mirror
Length of output: 1924
🌐 Web query:
containers/image ParseRef transport syntax tag and digest docker transport oci transport mapping.txt💡 Result:
The containers/image library uses a unified syntax for identifying container images, generally defined as transport:details [1][2]. The transport prefix determines how the remaining details are parsed by the corresponding transport driver [3]. Docker Transport (docker://) The docker transport follows the format: docker://docker-reference, where docker-reference is defined as name[:tag |
@digest] [2][4]. - Tag and Digest: The docker transport does not support specifying both a tag and a digest simultaneously [2][5]. - Unknown Digest: To push an image without a tag or digest, the reference can be suffixed with @@unknown-digest@@ (e.g., docker://my-image@@unknown-digest@@) [4][6]. OCI Transport (oci:path[:reference]) The OCI transport refers to an image in a directory structure compliant with the Open Container Image Layout Specification [7]. - Syntax: The syntax is oci:path[:{reference|@source-index}] [7]. - Path Handling: The path value terminates at the first colon (:) character; any subsequent colons are treated as part of the reference string [7]. - Reference: The reference part is used to match or set the org.opencontainers.image.ref.name annotation in the OCI index [7]. - Digest Support: Unlike the docker transport, the OCI transport does not natively support referencing an image by manifest digest using the@digestsyntax [8]. - Indexing: For reading images,@source-index(a zero-based index) can be used to access untagged images [7]. If neither a reference nor a@source-indexis specified, the path must contain exactly one image [7]. Summary of Transport Mappings - docker://: Maps to name[:tag |@digest] [2]. - oci:path: Maps to a directory at path, optionally with a tag reference or a numeric@source-index[7]. - General rule: The library functions such as ParseImageName split the input string at the first colon to determine the transport, then pass the remainder to the transport's specific ParseReference method [3][9].Citations:
- 1: https://github.com/containers/image/blob/main/docs/containers-transports.5.md
- 2: https://manpages.debian.org/bookworm/golang-github-containers-image/containers-transports.5.en.html
- 3: https://github.com/containers/image/blob/master/transports/alltransports/alltransports.go
- 4: https://github.com/containers/image/blob/master/docs/containers-transports.5.md
- 5: https://github.com/containers/image/blob/main/docker/docker_transport.go
- 6: https://man.archlinux.org/man/containers-transports.5.en
- 7: https://man.archlinux.org/man/extra/containers-common/containers-transports.5.en
- 8: https://github.com/containers/image/issues/1828
- 9: https://github.com/containers/image/blob/main/oci/layout/oci_transport.go
internal/pkg/cli/dryrun.go:107-139 — Strip any tag from
img.Sourcebefore appending@digest.strings.Cut(img.Source, "@")only removes an existing digest, so tag-based sources becomename:tag@digest, which containers/image rejects for docker references and doesn’t document for OCI references. Reuse the same ref parsing used insubDigestDestinationto derivesourceBase.🤖 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/dryrun.go` around lines 107 - 139, Update writeSubDigestEntries to derive sourceBase by parsing img.Source with the same image.ParseRef approach used by subDigestDestination, ensuring both tags and existing digests are removed before appending `@digest`. Preserve the current fallback behavior for parse failures and keep subDigestDestination unchanged.
141-199: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
New network calls in both files lack a bounded timeout. This PR introduces two new registry-inspection network paths — manifest-list digest inspection and early catalog digest pinning — neither of which applies a timeout, unlike the existing
checkRegistryAccesspattern (context.WithTimeout(ctx, 2*time.Minute)) already used elsewhere inexecutor.go.
internal/pkg/cli/dryrun.go#L141-L199: wrap the per-imageGetManifestListDigestscall (vianewSystemContextForSource) in a boundedcontext.WithTimeoutso one slow/unresponsive registry can't hold a semaphore slot indefinitely.internal/pkg/cli/executor.go#L581-L588: replacecontext.Background()with a context derived with a timeout (and, ideally, threaded from the command's context) before callingconfig.PinCatalogDigests.📍 Affects 2 files
internal/pkg/cli/dryrun.go#L141-L199(this comment)internal/pkg/cli/executor.go#L581-L588🤖 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/dryrun.go` around lines 141 - 199, Bound both new registry-inspection paths with a finite timeout, using the existing checkRegistryAccess timeout pattern. In internal/pkg/cli/dryrun.go lines 141-199, update inspectManifestLists and its GetManifestListDigests call after newSystemContextForSource to use a timeout-derived context and cancel it; in internal/pkg/cli/executor.go lines 581-588, replace context.Background() before config.PinCatalogDigests with the command context when available and derive a bounded timeout context.internal/pkg/cli/executor.go (2)
299-299: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Confusing flag help text.
"implies --dry-run, cannot be combined with --dry-run" reads as self-contradictory. Consider clarifying that it already enables dry-run behavior and must not be passed alongside an explicit
--dry-run.📝 Suggested fix
- cmd.Flags().BoolVar(&opts.IsDryRunManifestLists, "dry-run-manifest-lists", false, "Like --dry-run, but also includes manifest list sub-digests in mapping.txt (implies --dry-run, cannot be combined with --dry-run)") + cmd.Flags().BoolVar(&opts.IsDryRunManifestLists, "dry-run-manifest-lists", false, "Like --dry-run, but also includes manifest list sub-digests in mapping.txt. This flag already implies --dry-run; do not pass --dry-run explicitly alongside it")📝 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.cmd.Flags().BoolVar(&opts.IsDryRunManifestLists, "dry-run-manifest-lists", false, "Like --dry-run, but also includes manifest list sub-digests in mapping.txt. This flag already implies --dry-run; do not pass --dry-run explicitly alongside it")🤖 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` at line 299, Update the help text for the IsDryRunManifestLists flag registration to state that this option enables dry-run behavior automatically and must not be used together with an explicit --dry-run flag; remove the self-contradictory wording while preserving the existing flag semantics.
1383-1384: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment references a function name that doesn't exist in this file.
The comment says catalogs are "already pinned by
pinOperatorCatalogs()", but the function actually invoked (line 586) isconfig.PinCatalogDigests. This mismatch can mislead future readers trying to trace the pinning logic.📝 Suggested fix
-// Catalogs are already pinned by pinOperatorCatalogs() at workflow start for M2D/M2M modes. +// Catalogs are already pinned by config.PinCatalogDigests() at workflow start for M2D/M2M modes.📝 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.// Catalogs are already pinned by config.PinCatalogDigests() at workflow start for M2D/M2M modes. func (o *ExecutorSchema) createConfigsWithPinnedCatalogs() {🤖 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 1383 - 1384, Update the comment above ExecutorSchema.createConfigsWithPinnedCatalogs to reference the actual config.PinCatalogDigests call used at workflow start, replacing the nonexistent pinOperatorCatalogs name while preserving the M2D/M2M context.internal/pkg/config/pin_catalogs.go (1)
135-156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
TargetTag is mutated before digest resolution succeeds, contradicting the "skip = no action" contract.
op.TargetTagis preserved at Lines 136-139 unconditionally, before the network resolution at Line 147 and the empty-digest skip at Lines 152-156. If resolution fails or returns an empty digest,op.Catalogstays unpinned butop.TargetTaghas already been mutated as a side effect — inconsistent with the doc comment's "skipped (catalog won't be pinned)" / "no action needed" framing. Move the TargetTag preservation after the digest is confirmed non-empty.🐛 Proposed fix to defer TargetTag mutation until pinning succeeds
- // Preserve the original tag for the destination if user didn't provide targetTag - if op.TargetTag == "" && imgSpec.Tag != "" { - op.TargetTag = imgSpec.Tag - log.Debug("Preserving original tag %s as TargetTag for catalog %s", imgSpec.Tag, op.Catalog) - } - // Resolve tag to digest via network srcCtx, err := opts.SrcImage.NewSystemContext() if err != nil { return fmt.Errorf("failed to create system context: %w", err) } catalogDigest, err := manifestAPI.ImageDigest(ctx, srcCtx, imgSpec.ReferenceWithTransport) if err != nil { return fmt.Errorf("failed to resolve digest for catalog %s: %w", op.Catalog, err) } // Skip pinning if digest is empty if catalogDigest == "" { log.Debug("Skipping catalog %s (empty digest)", op.Catalog) return nil } + + // Preserve the original tag for the destination if user didn't provide targetTag + if op.TargetTag == "" && imgSpec.Tag != "" { + op.TargetTag = imgSpec.Tag + log.Debug("Preserving original tag %s as TargetTag for catalog %s", imgSpec.Tag, op.Catalog) + }📝 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.// Resolve tag to digest via network srcCtx, err := opts.SrcImage.NewSystemContext() if err != nil { return fmt.Errorf("failed to create system context: %w", err) } catalogDigest, err := manifestAPI.ImageDigest(ctx, srcCtx, imgSpec.ReferenceWithTransport) if err != nil { return fmt.Errorf("failed to resolve digest for catalog %s: %w", op.Catalog, err) } // Skip pinning if digest is empty if catalogDigest == "" { log.Debug("Skipping catalog %s (empty digest)", op.Catalog) return nil } // Preserve the original tag for the destination if user didn't provide targetTag if op.TargetTag == "" && imgSpec.Tag != "" { op.TargetTag = imgSpec.Tag log.Debug("Preserving original tag %s as TargetTag for catalog %s", imgSpec.Tag, op.Catalog) }🤖 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/config/pin_catalogs.go` around lines 135 - 156, Move the TargetTag preservation block in the catalog pinning flow after catalogDigest is confirmed non-empty and the empty-digest early return in the digest resolution logic. Keep the existing condition and log message, ensuring op.TargetTag is not mutated when resolution fails or pinning is skipped.
|
@dorzel: The following tests 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. |
openshift-ci
Bot
removed
the
needs-rebase
|
/retest |
|
/jira cherrypick OCPBUGS-66263 |
|
@aguidirh: Jira Issue OCPBUGS-66263 has been cloned as Jira Issue OCPBUGS-98716. Will retitle bug to link to clone. 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
Bot
changed the title
|
@dorzel: This pull request references Jira Issue OCPBUGS-98716, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
aguidirh
changed the title
|
/jira refresh |
|
@aguidirh: This pull request references Jira Issue OCPBUGS-98716, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 the command above is needed when we do manual backport, it will clone the bug in jira and generate a new one for the manual backport PR. For this PR I already did it, only sharing information here. |
|
/label backport-risk-assessed |
openshift-ci
Bot
added
the
backport-risk-assessed
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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
|
/test okd-scos-images |
|
/retest |
1 similar comment
|
/retest |
4 participants
Description
Manual backport of #1355
Github / Jira issue: https://redhat.atlassian.net/browse/OCPBUGS-66263
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
mapping.txt.