Add releaseImageRepo and releaseComponentRepo to ImageSetConfiguration#1452
Open
eb4x wants to merge 4 commits into
Open
Add releaseImageRepo and releaseComponentRepo to ImageSetConfiguration#1452eb4x wants to merge 4 commits into
eb4x wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eb4x The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe PR adds optional ChangesRelease repository path overrides
Sequence Diagram(s)sequenceDiagram
participant PlatformConfig
participant prepareM2DCopyBatch
participant prepareD2MCopyBatch
participant preparePathComponents
PlatformConfig->>prepareM2DCopyBatch: ReleaseImageRepo / ReleaseComponentRepo
prepareM2DCopyBatch->>preparePathComponents: selected release paths
PlatformConfig->>prepareD2MCopyBatch: ReleaseImageRepo / ReleaseComponentRepo
prepareD2MCopyBatch->>preparePathComponents: selected release paths
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @eb4x. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/features/custom-release-repo-paths.md`:
- Around line 60-63: Add a language tag to the fenced code blocks in the
documentation snippet to satisfy markdownlint MD040. Update the relevant fenced
sections in custom-release-repo-paths.md to use text fences for the examples,
keeping the existing content unchanged and applying the same fix to both
affected blocks.
In `@docs/okd-mirror.md`:
- Around line 3-14: The wording around --ignore-release-signature is too broad
and should match the actual behavior. Update the sentence in the OKD mirror docs
so it explicitly says the flag skips release signature verification only, and
keep the surrounding signature-verification guidance consistent with the runtime
behavior described by the OCP_SIGNATURE_URL and OCP_SIGNATURE_VERIFICATION_PK
instructions.
In `@internal/pkg/api/v2alpha1/type_config.go`:
- Around line 113-117: The Platform.DeepCopy implementation is missing scalar
fields, so copied config objects lose existing values. Update Platform.DeepCopy
to include all scalar members currently defined on Platform, specifically
Release and KubeVirtContainer along with Graph, ReleaseImageRepo, and
ReleaseComponentRepo, so the copied Platform preserves every configured
override.
In `@internal/pkg/release/local_stored_collector_test.go`:
- Around line 847-849: The test setup in local_stored_collector_test.go is
ignoring error returns from image.ParseRef, which can leave releaseImgSpec and
componentImgSpec as zero-value objects and hide parse failures. Update the setup
near the ParseRef calls (including the later one around the same test flow) to
handle errors explicitly, using the existing test function context and the
image.ParseRef results to fail the test immediately if parsing fails instead of
discarding the error.
🪄 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: 2c279d1d-8e05-42b3-b509-80dc0699e5cf
📒 Files selected for processing (5)
docs/features/custom-release-repo-paths.mddocs/okd-mirror.mdinternal/pkg/api/v2alpha1/type_config.gointernal/pkg/release/local_stored_collector.gointernal/pkg/release/local_stored_collector_test.go
Allow overriding the destination path for release images and release component images via the platform section of ImageSetConfiguration, following the targetRepo pattern used by additionalImages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The flag was gated behind an additional check for platform.release being set, which meant it only worked for direct release image references. For channel-based discovery (the normal flow for both OCP and OKD), the flag was silently ignored and signature verification always ran, failing fatally when no signature existed. This made it impossible to mirror OKD releases without setting the OCP_SIGNATURE_URL and OCP_SIGNATURE_VERIFICATION_PK env vars, and broke the flag for older OCP releases that predate cosign signature support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The distribution/distribution library parses all REGISTRY_* env vars as config overrides, colliding with the Podman/containers REGISTRY_AUTH_FILE convention. Unset it before calling configuration.Parse() since the value has already been captured as the --authfile flag default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/pkg/cli/executor.go`:
- Line 714: The REGISTRY_AUTH_FILE cleanup in executor.go is ignoring the error
returned by os.Unsetenv, which violates the no-ignored-errors rule. Update the
code in the executor flow that calls os.Unsetenv("REGISTRY_AUTH_FILE") to check
the returned error and fail fast if it is non-nil, propagating or handling the
failure through the existing error path in executor.go so the cleanup cannot
silently succeed when it actually fails.
🪄 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: ab75728b-9fa3-472d-89d6-298d3a1fa5e6
📒 Files selected for processing (7)
docs/features/custom-release-repo-paths.mddocs/okd-mirror.mdinternal/pkg/api/v2alpha1/type_config.gointernal/pkg/cli/executor.gointernal/pkg/release/local_stored_collector.gointernal/pkg/release/local_stored_collector_test.gointernal/pkg/release/signature.go
✅ Files skipped from review due to trivial changes (1)
- docs/features/custom-release-repo-paths.md
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/pkg/release/local_stored_collector_test.go
- internal/pkg/api/v2alpha1/type_config.go
- internal/pkg/release/local_stored_collector.go
| // distribution/distribution parses all REGISTRY_* env vars as config overrides, | ||
| // colliding with the Podman/containers REGISTRY_AUTH_FILE convention. | ||
| // The value has already been captured as the --authfile default (options.go). | ||
| os.Unsetenv("REGISTRY_AUTH_FILE") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle os.Unsetenv error explicitly.
os.Unsetenv returns an error, but it is currently ignored. Please fail fast if unsetting the variable fails.
As per path instructions, "**/*.go: Go security (prodsec-skills): - Never ignore error returns".
Proposed fix
- os.Unsetenv("REGISTRY_AUTH_FILE")
+ if err := os.Unsetenv("REGISTRY_AUTH_FILE"); err != nil {
+ return fmt.Errorf("unset REGISTRY_AUTH_FILE: %w", err)
+ }📝 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.
Suggested change
| os.Unsetenv("REGISTRY_AUTH_FILE") | |
| if err := os.Unsetenv("REGISTRY_AUTH_FILE"); err != nil { | |
| return fmt.Errorf("unset REGISTRY_AUTH_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/cli/executor.go` at line 714, The REGISTRY_AUTH_FILE cleanup in
executor.go is ignoring the error returned by os.Unsetenv, which violates the
no-ignored-errors rule. Update the code in the executor flow that calls
os.Unsetenv("REGISTRY_AUTH_FILE") to check the returned error and fail fast if
it is non-nil, propagating or handling the failure through the existing error
path in executor.go so the cleanup cannot silently succeed when it actually
fails.
Source: Path instructions
Author
|
The releaseImageRepo, and releaseComponentRepo could technically also go in to each channel object/item, incase you'd like to fetch both okd and ocp from a single imagesetconfig. Probably the most dynamic approach? This PR is just a suggested fix, the important part is we get to tell the exact path to store the images. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allow overriding the destination path for release images and release component images via the platform section of
ImageSetConfiguration, following thetargetRepopattern used byadditionalImages.Description
Override the currently hard-coded
openshift/release-imagesandopenshift/release, If you want to preserveokd/scos-releaseandokd/scos-contentoropenshift-release-dev/ocp-releaseandopenshift-release-dev/ocp-v4.0-art-dev, or just want it somewhere different.Reasons for doing this might include you having a conflicting image-mirror for a larger scope, like all of quay.io
Github issue: #1451, #1453
Type of change
How Has This Been Tested?
Compiled oc-mirror, and tried copying stable-4.22 (4.22.0 - 4.22.0) and type okd, stable-4-scos (4.18.0-okd-scos.8 - 4.18.0-okd-scos.10)
Expected Outcome
Images are copied over to paths specified in yaml.
Summary by CodeRabbit
oc-mirror --v2examples.