Drop the scale subresource before building OpenAPI for schema generation#119
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 39 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesScale Subresource Exclusion Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
🧹 Nitpick comments (1)
internal/crd/convert_test.go (1)
115-185: ⚡ Quick winPlease align this test with the table-driven
args/wantpattern used in this repo.Could we convert this single-case test into a table-driven case so follow-up scenarios (e.g., multiple versions, scale absent, malformed subresource blocks) can be added consistently without duplicating setup/assertion code?
As per coding guidelines,
**/*_test.goshould enforce a table-driven test structure with anargs/wantpattern.🤖 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 `@internal/crd/convert_test.go` around lines 115 - 185, Convert the TestToOpenAPIScaleSubresource function to use a table-driven test pattern with args and want fields. Create a struct type to hold test case data including the CRD YAML input (args) and expected validation results like the presence of required schemas and absence of leaked autoscaling schemas (want). Replace the single test body with a loop that iterates over a slice of test cases, executing the same ToOpenAPI call and assertions for each case. This will allow future test scenarios (multiple versions, missing scale subresource, malformed blocks) to be added as additional table entries without duplicating the setup and assertion logic.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@internal/crd/convert_test.go`:
- Around line 115-185: Convert the TestToOpenAPIScaleSubresource function to use
a table-driven test pattern with args and want fields. Create a struct type to
hold test case data including the CRD YAML input (args) and expected validation
results like the presence of required schemas and absence of leaked autoscaling
schemas (want). Replace the single test body with a loop that iterates over a
slice of test cases, executing the same ToOpenAPI call and assertions for each
case. This will allow future test scenarios (multiple versions, missing scale
subresource, malformed blocks) to be added as additional table entries without
duplicating the setup and assertion logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e943d0b-4f47-4b8d-b102-a71323b3f84a
📒 Files selected for processing (2)
internal/crd/convert.gointernal/crd/convert_test.go
Crossplane v2.3 lets XRDs configure a scale subresource. When a resource declares one, generating language schemas for it produces models of the autoscaling/v1 Scale type in place of the resource's own model. For Python the generated module's imports also change shape, so importing the model fails outright with "No module named 'models.ai.apimachinery'". ToOpenAPI builds the intermediate OpenAPI document with BuildOpenAPIV3, which emits the resource's whole REST API surface, not just its structural schema. Declaring a scale subresource adds a /scale endpoint whose request and response body is an autoscaling/v1 Scale. To describe that endpoint the builder adds the Scale, ScaleSpec, and ScaleStatus schemas to the document's components. The meta types it always emits (ObjectMeta and friends) are redirected to a shared module by a later transform, but the autoscaling types are not, so the language generator models them into the resource's own module. The scale subresource is a runtime API behaviour, not part of the resource's structural schema, so it has no bearing on the generated model. This clears it before the conversion, so BuildOpenAPIV3 never emits the /scale endpoint or its Scale schemas and the resource's model generates as it did before. Fixes crossplane#117. Signed-off-by: Nic Cope <nicc@rk0n.org>
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
LGTM. It seems like the scale subresource causes a problem only for Python (I believe due to how we structure the generated Python packages), but dropping scale before generating is reasonable.
I wonder if this would let us also simplify some of the generator code, since I know it has special handling for the v1/autoscaling Scale resource.
2 participants
Fixes #117
When an XRD or CRD declares a
scalesubresource,crossplane xrd generateproduces language schemas for theautoscaling/v1Scaletype in place of the resource's own model. For Python the generated module's imports also change shape, so importing the model fails outright withModuleNotFoundError: No module named 'models.ai.apimachinery'.ToOpenAPIbuilds the intermediate OpenAPI document withBuildOpenAPIV3, which models the resource's whole REST surface, not just its structural schema. Declaring a scale subresource adds a/scaleendpoint whose request and response body is anautoscaling/v1Scale, so the builder addsScale,ScaleSpec, andScaleStatusto the document's components. The meta types it always emits (ObjectMetaand friends) get redirected to a shared module by a later transform, but the autoscaling types don't, so the language generator models them into the resource's own module.The scale subresource is a runtime API behaviour with no bearing on the generated model, so this clears it before the conversion.
BuildOpenAPIV3then never emits the/scaleendpoint or itsScaleschemas, and the resource's model generates as it did before.I have:
nix flake checkto ensure this PR is ready for review.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.