NO-ISSUE: Update oc-mirror locations to CGW following openshift release decoupling - #10601
NO-ISSUE: Update oc-mirror locations to CGW following openshift release decoupling#10601dorzel wants to merge 2 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
size/XS
WalkthroughThe mirror utility now retries transient ChangesMirror reliability and OS image discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MirrorUtility
participant MCO
participant ReleaseMetadata
participant RegistryConfig
MirrorUtility->>MCO: Discover OS image stream sources
MCO-->>MirrorUtility: Return image stream targets
MirrorUtility->>ReleaseMetadata: Fall back to release JSON
ReleaseMetadata-->>MirrorUtility: Return source repositories
MirrorUtility->>RegistryConfig: Configure discovered repositories
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
deploy/operator/mirror_utils.sh (3)
232-233: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve release-metadata query failures.
Without a guaranteed
pipefail, a failedoc adm release infois masked byjqreturning success with no rows. That silently omits required registry mappings. Capture and validate theocJSON before passing it tojq.🤖 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 `@deploy/operator/mirror_utils.sh` around lines 232 - 233, Update the release metadata flow around the oc adm release info invocation to capture its JSON output and validate that the command succeeds before passing the result to jq. Preserve and propagate the oc failure instead of allowing jq to mask it, then retain the existing filtering and unique registry mapping behavior for successful responses.
168-168: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRedact external command output before writing deployment logs.
Both commands can include private registry endpoints and other deployment-specific details.
deploy/operator/mirror_utils.sh#L168-L168: emit a sanitized error summary rather than the completeoc admoutput.deploy/operator/mirror_utils.sh#L214-L215: redact or restrict MCO stderr before forwarding it to logs.As per coding guidelines, flag logging that may expose passwords, tokens, API keys, PII, session IDs, internal hostnames, or customer data.
🤖 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 `@deploy/operator/mirror_utils.sh` at line 168, Sanitize external command output before deployment logging: at deploy/operator/mirror_utils.sh lines 168-168, replace the complete oc adm output emitted by the surrounding function with a sanitized error summary; at lines 214-215, redact or restrict MCO stderr before forwarding it to logs. Preserve the existing error handling while ensuring private registry endpoints and other sensitive deployment details are not logged.Source: Coding guidelines
267-273: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate source-discovery failures.
The exit status of
discover_os_image_stream_sourcesinside process substitution is not the loop’s exit status. A failed discovery therefore produces an apparently successful, emptyregistries.confsection.Proposed fix
function registry_configs_for_os_image_stream_sources() { + local sources release_image="${1}" authfile="${2}" release_mirror_repo="${3}" shift 3 + if ! sources=$(discover_os_image_stream_sources "${release_image}" "${authfile}"); then + return 1 + fi + while IFS= read -r source; do [ -n "${source}" ] || continue for skip_repo in "$@"; do [ "${source}" = "${skip_repo}" ] && continue 2 done registry_config "${source}" "${release_mirror_repo}" - done < <(discover_os_image_stream_sources "${release_image}" "${authfile}") + done <<< "${sources}" }🤖 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 `@deploy/operator/mirror_utils.sh` around lines 267 - 273, Update the loop consuming discover_os_image_stream_sources so its failure status is captured and propagated instead of being hidden by process substitution. Ensure registry_config processing remains unchanged for successful discovery, while a discovery error causes the surrounding operation to return a nonzero status rather than generating an apparently successful empty configuration.
🤖 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 `@deploy/operator/mirror_utils.sh`:
- Line 277: Replace the placeholder default assigned to OC_MIRROR_URL in the
mirror utility configuration with the finalized CGW endpoint required for
disconnected CAPI deployments, ensuring callers can still override it through
the existing environment-variable fallback.
---
Outside diff comments:
In `@deploy/operator/mirror_utils.sh`:
- Around line 232-233: Update the release metadata flow around the oc adm
release info invocation to capture its JSON output and validate that the command
succeeds before passing the result to jq. Preserve and propagate the oc failure
instead of allowing jq to mask it, then retain the existing filtering and unique
registry mapping behavior for successful responses.
- Line 168: Sanitize external command output before deployment logging: at
deploy/operator/mirror_utils.sh lines 168-168, replace the complete oc adm
output emitted by the surrounding function with a sanitized error summary; at
lines 214-215, redact or restrict MCO stderr before forwarding it to logs.
Preserve the existing error handling while ensuring private registry endpoints
and other sensitive deployment details are not logged.
- Around line 267-273: Update the loop consuming
discover_os_image_stream_sources so its failure status is captured and
propagated instead of being hidden by process substitution. Ensure
registry_config processing remains unchanged for successful discovery, while a
discovery error causes the surrounding operation to return a nonzero status
rather than generating an apparently successful empty configuration.
🪄 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: Pro Plus
Run ID: c90a5152-e81e-4b87-844a-6b73b9a07f2e
📒 Files selected for processing (1)
deploy/operator/mirror_utils.sh
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10601 +/- ##
=======================================
Coverage 44.47% 44.48%
=======================================
Files 423 423
Lines 73374 73374
=======================================
+ Hits 32634 32639 +5
+ Misses 37821 37818 -3
+ Partials 2919 2917 -2 🚀 New features to boost your workflow:
|
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielerez, 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
dorzel
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dorzel: This pull request explicitly references no 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: The following test 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. |
4 participants
Changes the oc-mirror download mirror location to the new content gateway (CGW) location following the decoupling of oc-mirror from the openshift release process in version 5.0+.
See:
Summary by CodeRabbit