OCPBUGS-60903: oc adm release extract: add oc-mirror to the tools list #2106
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@r4f4: This pull request references Jira Issue OCPBUGS-60903, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
|
/cc @joepvd |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: r4f4 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
| To learn more about OpenShift, visit [docs.openshift.com](https://docs.openshift.com) | ||
| and select the version of OpenShift you are using. | ||
| # Installing the tools |
There was a problem hiding this comment.
IMO this should be Installing the Tools
There was a problem hiding this comment.
I copied from the other instances which are all "Installing the tools" but I can change it, no problem.
There was a problem hiding this comment.
Just a nit, anyway. You can just keep it to be consistent.
r4f4
force-pushed
the
ocpbugs-60903-output-ocp-release-oc-mirror
branch
from
a90ea8e to
08da716
Compare
WalkthroughAdded a readme block for the OC Mirror utility and three new extraction target entries (oc-mirror.rhel9, oc-mirror.rhel8, oc-mirror) with binary mappings, archive patterns, and InjectReleaseVersion enabled. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cli/admin/release/extract_tools.go (1)
641-641: Update the error message to include oc-mirror.The error message lists supported commands but does not include the newly added
oc-mirrorcommand. Additionally, there's inconsistent quote usage (backticks foropenshift-baremetal-installvs. single quotes for others).Apply this diff to update the error message:
- return fmt.Errorf("the supported commands are 'oc', 'openshift-install', `openshift-baremetal-install` and 'ccoctl'") + return fmt.Errorf("the supported commands are 'oc', 'openshift-install', 'openshift-baremetal-install', 'ccoctl', and 'oc-mirror'")
🧹 Nitpick comments (1)
pkg/cli/admin/release/extract_tools.go (1)
215-216: Clarify the registry mirroring description.The phrase "from a target registry to another" is awkward. Consider revising to "from one registry to another" or "from a source registry to a target registry" for clarity.
Apply this diff to improve clarity:
- The oc-mirror tool assists with mirroring OpenShift release content from a target - registry to another for disconnected install scenarios. + The oc-mirror tool assists with mirroring OpenShift release content from one + registry to another for disconnected install scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/cli/admin/release/extract_tools.go(2 hunks)
🔇 Additional comments (1)
pkg/cli/admin/release/extract_tools.go (1)
550-579: Good implementation of oc-mirror tool extraction.The implementation correctly adds oc-mirror extraction support with proper version injection (
InjectReleaseVersion: true), which fulfills the PR objective. The structure follows established patterns for similar tools like ccoctl, with appropriate README documentation and RHEL-specific variants.
This will allow oc-mirror to be injected with the OCP release version it's a part of.
r4f4
force-pushed
the
ocpbugs-60903-output-ocp-release-oc-mirror
branch
from
08da716 to
c1f8d9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/cli/admin/release/extract_tools.go (1)
643-643: LGTM! Error message updated correctly.oc-mirror is properly added to the list of supported commands.
Minor pre-existing note: There's inconsistent quote usage in this error message (single quotes vs backticks around
openshift-baremetal-install), but that's not introduced by this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/cli/admin/release/extract_tools.go(3 hunks)
🔇 Additional comments (2)
pkg/cli/admin/release/extract_tools.go (2)
212-231: LGTM! Readme follows the established pattern.The readme content is well-structured and consistent with other tool readmes in the file. The description accurately explains oc-mirror's purpose for disconnected install scenarios.
550-581: LGTM! All three oc-mirror targets are correctly configured.The implementation properly addresses previous review feedback:
NewArch: trueis set on all three targets, ensuring graceful extraction failure on releases prior to 4.10 that don't include oc-mirrorTargetCommandNameis correctly used only for the RHEL-specific variants, following the pattern established by ccoctlInjectReleaseVersion: trueenables version injection as described in the PR objectivesThe structure and patterns match the ccoctl implementation (lines 518-549), maintaining consistency across the codebase.
|
/retest |
|
@r4f4: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
This will allow oc-mirror to be injected with the OCP release version it's a part of.