MCO-2146: do not use OSImageStream in Hypershift#5750
MCO-2146: do not use OSImageStream in Hypershift#5750cheesesashimi wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@cheesesashimi: This pull request references MCO-2146 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi 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
WalkthroughThe changes add OSImageStream factory dependency injection to the Bootstrap controller for improved testability, introduce a control plane topology condition check (non-external required) for OSImageStream enablement, and expand test coverage with factory-based testing patterns and a Hypershift-specific test scenario. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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 Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
@cheesesashimi: This pull request references MCO-2146 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
/test unit verify |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/bootstrap/bootstrap_test.go (1)
198-198: Prefer a Go-side copy helper overcphere.Shelling out to
cpmakes these tests depend on an external binary and an extra process. Go's standard library (os.ReadFile,os.WriteFile,filepath.Walk,filepath.WalkDir) handles both recursive directory copies (line 198) and single file copies (line 228) without external dependencies, keeping tests self-contained and more portable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/bootstrap/bootstrap_test.go` at line 198, The test currently shells out to "cp -r" (require.NoError(t, exec.Command("cp", "-r", "testdata/bootstrap/.", srcDir).Run())) which introduces an external dependency; replace this with a Go-side recursive copy helper used in the test (e.g., copyDir or copyFile helpers) that walks the source with filepath.WalkDir, creates destination directories with os.MkdirAll, reads files with os.ReadFile and writes with os.WriteFile (preserving file mode where appropriate via os.FileMode), and use that helper in place of the exec.Command call as well as for the single-file copy call around line 228 so the tests no longer rely on external binaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controller/bootstrap/bootstrap_test.go`:
- Line 198: The test currently shells out to "cp -r" (require.NoError(t,
exec.Command("cp", "-r", "testdata/bootstrap/.", srcDir).Run())) which
introduces an external dependency; replace this with a Go-side recursive copy
helper used in the test (e.g., copyDir or copyFile helpers) that walks the
source with filepath.WalkDir, creates destination directories with os.MkdirAll,
reads files with os.ReadFile and writes with os.WriteFile (preserving file mode
where appropriate via os.FileMode), and use that helper in place of the
exec.Command call as well as for the single-file copy call around line 228 so
the tests no longer rely on external binaries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0efb797c-63e1-4bd6-adb8-027b3c21de94
📒 Files selected for processing (4)
pkg/controller/bootstrap/bootstrap.gopkg/controller/bootstrap/bootstrap_test.gopkg/controller/bootstrap/testdata/bootstrap-hypershift/machineconfigcontroller-controllerconfig.yamlpkg/controller/bootstrap/testdata/bootstrap/featuregate.yaml
| // Enable OSImageStreams if the FeatureGate is active and the deployment is not OKD | ||
| if osimagestream.IsFeatureEnabled(fgHandler) { | ||
| // Enable OSImageStreams if the FeatureGate is active and the control plane topology is not external (e.g., Hypershift) | ||
| if osimagestream.IsFeatureEnabled(fgHandler) && cconfig.Spec.Infra.Status.ControlPlaneTopology != apicfgv1.ExternalTopologyMode { |
There was a problem hiding this comment.
I'm a bit surprissed about not needing the same check in other places, specially here https://github.com/openshift/machine-config-operator/blob/main/pkg/operator/osimagestream_ocp.go#L170, but not limited to that place only.
There was a problem hiding this comment.
I don't think it would be needed in other places. The reason is because in HyperShift, the tenant clusters don't have a running MCO on them. Additionally, the MCD is only ran on-demand by the HyperShift in response to a config change.
|
/test e2e-hypershift-techpreview |
pablintino
left a comment
pablintino
left a comment
There was a problem hiding this comment.
The change looks good to me.
Given that the osimagestream package is going to change a bit and https://github.com/openshift/machine-config-operator/pull/5750/changes#diff-78aad83f4838506d3b9ce960f12bb3c670b649ce5cd01603ed1fd2d00ba48d48R176 may require some small touches, what do you prefer to do, wait for the defaultStream PR to land and rebase this one or land this one now and adjust this in that PR? I don't mind rebasing my change on top of this content if we merge this PR soon.
|
I think we should merge this one sooner rather than later. I'm fairly confident that this change addresses the root issue. The e2e-hypershift failure does not appear related to this change. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@cheesesashimi: 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. |
3 participants
- What I did
During Hypershift bootstrapping, the OSImageStream value for a given release should not (yet) be consumed.
- How to verify it
- Description for the changelog
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests