NO-ISSUE: test(install): validate DynamicResourceAllocation field exists in CRD #7277
Conversation
Add unit test to verify that the field spec.configuration.scheduler.profileCustomizations.dynamicResourceAllocation exists in the generated HostedCluster CRD manifests. The test validates the field exists in all CRD variants (Default, TechPreviewNoUpgrade, CustomNoUpgrade) by traversing the CRD schema. This ensures that a bump of openshift/api does not accidentally remove this field from HyperShift without proper deprecation. The test uses schema traversal with cycle detection and depth limits to safely navigate the CRD schema structure. Signed-off-by: Cesar Wong <[email protected]>
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughA new test file introduces validation for a nested field in HostedCluster CRD OpenAPI v3 schemas. The test includes a recursive helper function to traverse JSONSchemaProps structures, checking for the presence of the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ 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 Comment |
openshift-ci
bot
added
do-not-merge/needs-area
area/cli
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
csrwng
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@csrwng: This pull request explicitly references no jira issue. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/install/crd_validation_test.go (1)
17-68: Schema traversal helper is sound for the intended useThe recursive traversal with depth limiting and cycle detection over
Properties+AllOf/AnyOf/OneOfis appropriate for guarding against pathological schemas, and it matches how CRDs typically embed nested objects. Given that the target path is a straightpropertieschain, this is sufficient and keeps the helper simple without needing to support the full JSON Schema surface (e.g.,Items,AdditionalProperties,$ref), which would add complexity without clear benefit here.If, in the future, you need to guard additional nested fields that live under arrays or maps, consider extending this helper to also walk
ItemsandAdditionalProperties, but it's not necessary for this PR.
📜 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)
cmd/install/crd_validation_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
cmd/install/crd_validation_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-release-mce-211-on-pull-request
🔇 Additional comments (1)
cmd/install/crd_validation_test.go (1)
70-125: Targeted CRD field-presence test is well-structuredThe test cleanly exercises all relevant HostedCluster CRD variants, decodes only the needed files from the embedded assets, and asserts on the
v1beta1OpenAPI v3 schema in a focused way. The failure messages are descriptive (crdPath+fieldPath), which should make regressions easy to diagnose. Hard-coding the CRD paths and version here is acceptable and desirable given the goal of catching accidental field removal during API bumps.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, csrwng The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
@csrwng Can we shift this problem left somehow? I'm concerned that this is protecting a vendor bump and not a merge at the source (o/api). It would be preferable to catch this and prevent the merge at o/api |
|
sgtm @JoelSpeed. I'll create a test there and close this one. |
|
@csrwng: Closed this PR. 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 kubernetes-sigs/prow repository. |
|
/reopen |
|
@csrwng: Reopened this PR. 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 kubernetes-sigs/prow repository. |
|
/retest-required |
|
/test e2e-aks-4-20 |
|
/verified by unit-test |
openshift-ci-robot
added
the
verified
|
@csrwng: This PR has been marked as verified by 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. |
|
/test verify |
|
/hold Revision 0123e30 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest-required |
|
@csrwng: all tests passed! 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. |
4 participants
What this PR does / why we need it:
Add unit test to verify that the field
spec.configuration.scheduler.profileCustomizations.dynamicResourceAllocation exists in the generated HostedCluster CRD manifests.
The test validates the field exists in all CRD variants (Default, TechPreviewNoUpgrade, CustomNoUpgrade) by traversing the CRD schema. This ensures that a bump of openshift/api does not accidentally remove this field from HyperShift without proper deprecation.
The test uses schema traversal with cycle detection and depth limits to safely navigate the CRD schema structure.