SDCICD-1840 Add mcsimulation package with CRD installation logic#3254
SDCICD-1840 Add mcsimulation package with CRD installation logic#3254YiqinZhang wants to merge 2 commits into
Conversation
|
There are test jobs defined for this repository which are not configured to run automatically. Comment |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughPromote apiextensions dependency and add mcsimulation package with minimal CRD templates ( ChangesManagement Cluster CRD Installation Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: YiqinZhang 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
YiqinZhang
force-pushed
the
sdcicd-1840-crds
branch
from
cf5543a to
dc598c9
Compare
|
/retest |
![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
🧹 Nitpick comments (1)
pkg/common/mcsimulation/crds_test.go (1)
42-151: ⚡ Quick winConsider adding test for Create success + Establishment timeout scenario.
Current tests cover waitForCRDEstablished timeout in isolation and EnsureCRDsInstalled with various creation outcomes. Missing coverage for the path where Create succeeds but the CRD never reaches Established state, triggering the error at crds.go:151-153. This is a realistic operator failure mode.
🧪 Suggested test case
func TestEnsureCRDsInstalled_EstablishmentTimeout(t *testing.T) { // CRD created but never becomes established crdObj := parseCRDForFake(t) // No conditions set, so it will never be established client := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme()) client.PrependReactor("get", "customresourcedefinitions", func(action k8stesting.Action) (bool, runtime.Object, error) { name := action.(k8stesting.GetAction).GetName() if name == crdObj.GetName() { return true, crdObj, nil } return notFoundReactor(action) }) client.PrependReactor("create", "customresourcedefinitions", func(action k8stesting.Action) (bool, runtime.Object, error) { return true, crdObj, nil }) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() err := EnsureCRDsInstalled(ctx, client, []string{"hcp"}) if err == nil || !strings.Contains(err.Error(), "not Established after install") { t.Fatalf("expected 'not Established' error, got: %v", 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 `@pkg/common/mcsimulation/crds_test.go` around lines 42 - 151, The tests miss the case where EnsureCRDsInstalled successfully creates a CRD but waitForCRDEstablished never observes an Established condition (the code path that returns "not Established after install" in EnsureCRDsInstalled). Add a unit test that uses dynamicfake.NewSimpleDynamicClient with reactors: a "create" reactor that returns the created CRD object (success) and a "get" reactor that returns that CRD object without any status.conditions (so it never becomes Established); call EnsureCRDsInstalled (with a context timeout) for the "hcp" alias and assert the returned error contains the "not Established" message; reference EnsureCRDsInstalled and waitForCRDEstablished to locate the logic to exercise.
🤖 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 `@pkg/common/mcsimulation/crds.go`:
- Around line 7-21: Replace the use of the standard log package and any
fmt.Println calls in pkg/common/mcsimulation/crds.go with structured logr/klog
calls: remove the "log" import, import klog (or controller-runtime logger)
instead, and change each log.Printf(...) occurrence (and any fmt.Println) to
klog.InfoS(...) or klog.ErrorS(...) with key/value pairs for context (e.g.,
"msg" text and keys like "crd", "err", "attempt"), and if the original call
logged an error include it under the "err" key; ensure the call sites that
currently use log.Printf around unstructured objects include the object
name/Kind as structured fields so they’re easy to filter.
---
Nitpick comments:
In `@pkg/common/mcsimulation/crds_test.go`:
- Around line 42-151: The tests miss the case where EnsureCRDsInstalled
successfully creates a CRD but waitForCRDEstablished never observes an
Established condition (the code path that returns "not Established after
install" in EnsureCRDsInstalled). Add a unit test that uses
dynamicfake.NewSimpleDynamicClient with reactors: a "create" reactor that
returns the created CRD object (success) and a "get" reactor that returns that
CRD object without any status.conditions (so it never becomes Established); call
EnsureCRDsInstalled (with a context timeout) for the "hcp" alias and assert the
returned error contains the "not Established" message; reference
EnsureCRDsInstalled and waitForCRDEstablished to locate the logic to exercise.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: eab75654-7ee6-478c-889e-8ab7e1f7cda8
📒 Files selected for processing (2)
pkg/common/mcsimulation/crds.gopkg/common/mcsimulation/crds_test.go
YiqinZhang
force-pushed
the
sdcicd-1840-crds
branch
2 times, most recently
from
e02d096 to
b83a497
Compare
|
/override ci/prow/hypershift-pr-check |
|
@YiqinZhang: Overrode contexts on behalf of YiqinZhang: ci/prow/hypershift-pr-check 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 kubernetes-sigs/prow repository. |
YiqinZhang
force-pushed
the
sdcicd-1840-crds
branch
from
b83a497 to
0b704f2
Compare
|
Oops, something went wrong! Please try again later. 🐰 💔 |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
YiqinZhang
force-pushed
the
sdcicd-1840-crds
branch
from
0b704f2 to
aef47ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/common/mcsimulation/crds.go (1)
136-136: 💤 Low valueConsider extracting CRD establishment timeout as a named constant.
The 30-second timeout is hardcoded. Extract to a package-level constant for clarity and easier tuning.
♻️ Proposed refactor
+const crdEstablishmentTimeout = 30 * time.Second + // EnsureCRDsInstalled resolves a list of CRD aliases and installs any that are // not already present on the cluster. Forbidden errors are collected and returned // together so callers get a single actionable message with all missing CRDs.Then at line 136:
- if err := waitForCRDEstablished(ctx, extClient, name, 30*time.Second); err != nil { + if err := waitForCRDEstablished(ctx, extClient, name, crdEstablishmentTimeout); err != nil {🤖 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 `@pkg/common/mcsimulation/crds.go` at line 136, The hardcoded 30*time.Second passed to waitForCRDEstablished should be extracted into a package-level constant (e.g., crdEstablishTimeout) to improve clarity and tunability; add a top-of-file const like crdEstablishTimeout = 30 * time.Second and replace the literal 30*time.Second in the call to waitForCRDEstablished(ctx, extClient, name, crdEstablishTimeout), updating any related comments if present.
🤖 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 `@pkg/common/mcsimulation/crds.go`:
- Line 111: Replace the hardcoded available-aliases string with a dynamic list
from BuiltinCRDAliases(); specifically, change the fmt.Errorf call that
currently uses "(available: hcp, vpce)" to use strings.Join(BuiltinCRDAliases(),
", ") (e.g. fmt.Errorf("unknown CRD alias %q (available: %s)", alias,
strings.Join(BuiltinCRDAliases(), ", "))). Also add the "strings" import if
missing so the code compiles.
---
Nitpick comments:
In `@pkg/common/mcsimulation/crds.go`:
- Line 136: The hardcoded 30*time.Second passed to waitForCRDEstablished should
be extracted into a package-level constant (e.g., crdEstablishTimeout) to
improve clarity and tunability; add a top-of-file const like crdEstablishTimeout
= 30 * time.Second and replace the literal 30*time.Second in the call to
waitForCRDEstablished(ctx, extClient, name, crdEstablishTimeout), updating any
related comments if present.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 23fddf24-b341-4c9b-b285-3ae9c280d94e
📒 Files selected for processing (3)
go.modpkg/common/mcsimulation/crds.gopkg/common/mcsimulation/crds_test.go
|
/override ci/prow/hypershift-pr-check |
|
@YiqinZhang: Overrode contexts on behalf of YiqinZhang: ci/prow/hypershift-pr-check 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 kubernetes-sigs/prow repository. |
|
@YiqinZhang: all tests passed! 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. |
1 participant
Introduces pkg/common/mcsimulation to support Management Cluster simulation for HCP operator testing. Operators that watch HyperShift resources (e.g. HostedControlPlane) can be tested on ROSA Classic clusters without a real Management Cluster, by installing minimal CRDs that accept arbitrary spec/status fields via
x-kubernetes-preserve-unknown-fields: true.Co-authored-by: Cursor cursor@cursor.com
Summary by CodeRabbit