fix(examples): flatten SchemaTypeObject blocks in generated example manifests#657
Conversation
…anifests The HCL-to-JSON scraper wraps all Terraform blocks in arrays per HCL spec. For NestingModeSingle (SchemaTypeObject) fields, upjet generates CRD types as embedded objects (pointer-to-struct), but the generated example manifests retained array syntax, causing unknown field errors. ApplyAPIConverters only handled TypeList/TypeSet with MaxItems=1 via SingletonListEmbedder. SchemaTypeObject fields were not covered because they use a different schema type (ValueType 9) and their paths lack [*] wildcards. Add a post-processing step that traverses each resource schema, collects SchemaTypeObject CRD paths, and flattens single-element arrays at those paths. Uses lexical ordering (parents before children) because without wildcards, child paths are not resolvable while the parent is still an array. Fixes crossplane#656 Signed-off-by: Duologic <jeroen@simplistic.be>
Duologic
force-pushed
the
duologic/fix-schema-type-object-example-conversion
branch
from
53da950 to
5433818
Compare
Duologic
requested review from
erhancagirici,
sergenyalcin and
ulucinar
as code owners
📝 WalkthroughWalkthroughThis PR adds SchemaTypeObject flattening to ChangesSchemaTypeObject Flattening Feature
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🧹 Nitpick comments (1)
pkg/examples/conversion/example_conversions_test.go (1)
383-397: 💤 Low valueConsider using stdlib
sort.Stringsfor consistency with Go idioms.The bubble sort implementation works correctly, but Go's
sort.Stringsis more idiomatic and efficient (O(n log n) vs O(n²)). Since the function needs to return a new slice for use incmp.Transformer, you could copy first then sort:func sortStrings(s []string) []string { if s == nil { return nil } result := make([]string, len(s)) copy(result, s) sort.Strings(result) return result }That said, the current implementation is perfectly clear and works fine for test helper purposes—this is purely a style suggestion.
🤖 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/examples/conversion/example_conversions_test.go` around lines 383 - 397, Replace the manual O(n²) bubble-sort inside the helper function sortStrings with the stdlib sort.Strings for idiomatic and faster sorting: retain the nil check and the copy into result (used by cmp.Transformer), then call sort.Strings(result) before returning; ensure you import "sort" in the test file and update any references to sortStrings as needed.
🤖 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/examples/conversion/example_conversions_test.go`:
- Line 335: Update the test case's reason string in example_conversions_test.go:
change the 'reason' field that currently says "Should flatten both parent and
child SchemaTypeObject fields, processing children first." to accurately state
parent-first processing (e.g., "Should flatten both parent and child
SchemaTypeObject fields, processing parents before children.") so the test
description matches the actual behavior verified by the test (the test case with
the 'reason' field referencing parent/child SchemaTypeObject).
---
Nitpick comments:
In `@pkg/examples/conversion/example_conversions_test.go`:
- Around line 383-397: Replace the manual O(n²) bubble-sort inside the helper
function sortStrings with the stdlib sort.Strings for idiomatic and faster
sorting: retain the nil check and the copy into result (used by
cmp.Transformer), then call sort.Strings(result) before returning; ensure you
import "sort" in the test file and update any references to sortStrings as
needed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f82eefa5-5565-4aa6-b70c-6aaf2b9b7588
📒 Files selected for processing (2)
pkg/examples/conversion/example_conversions.gopkg/examples/conversion/example_conversions_test.go
| }, | ||
| }, | ||
| "FlattenNestedParentAndChild": { | ||
| reason: "Should flatten both parent and child SchemaTypeObject fields, processing children first.", |
There was a problem hiding this comment.
Clarify the processing order in test reason.
The test reason states "processing children first" but the implementation processes parents before children (lexical ordering ensures "outer" is processed before "outer.inner", as explained in lines 67-72 of the main file). This test correctly verifies that both parent and child are flattened, but the comment should reflect the actual parent-first processing order to avoid confusing future maintainers.
📝 Suggested correction
- reason: "Should flatten both parent and child SchemaTypeObject fields, processing children first.",
+ reason: "Should flatten both parent and child SchemaTypeObject fields, processing parents first.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| reason: "Should flatten both parent and child SchemaTypeObject fields, processing children first.", | |
| reason: "Should flatten both parent and child SchemaTypeObject fields, processing parents first.", |
🤖 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/examples/conversion/example_conversions_test.go` at line 335, Update the
test case's reason string in example_conversions_test.go: change the 'reason'
field that currently says "Should flatten both parent and child SchemaTypeObject
fields, processing children first." to accurately state parent-first processing
(e.g., "Should flatten both parent and child SchemaTypeObject fields, processing
parents before children.") so the test description matches the actual behavior
verified by the test (the test case with the 'reason' field referencing
parent/child SchemaTypeObject).
1 participant
Summary
ApplyAPIConvertersthat flattens single-element arrays atSchemaTypeObject(NestingModeSingle) field paths in generated example YAML manifestsSchemaTypeObjectfields per resourceBackground
The HCL-to-JSON scraper wraps all Terraform blocks in arrays per HCL spec. For
NestingModeSingleblocks, upjet generates CRD types as embedded objects (pointer-to-struct), not slices. However, the generated example manifests retained the array syntax from the scraper, producing invalid YAML that fails with "unknown field" errors when applied.ApplyAPIConverters(added in #538) only handlesTypeList/TypeSetwithMaxItems=1viaSingletonListEmbedder.SchemaTypeObjectfields were not covered because:SingletonListEmbedder.VisitResourcefilters onType == TypeList || TypeSet, skippingSchemaTypeObject(ValueType 9)[*]wildcards forSchemaTypeObjectpaths, so they can't be passed throughconversion.Convertthe same wayImplementation
A separate lenient flattening function is used instead of reusing
conversion.Convert, because:Converterrors on non-slice values (correct for runtime), but example manifests may already have objects at some pathsSchemaTypeObjectpaths require lexical ordering (parents before children), opposite toConvert's reverse-lexical ordering forToEmbeddedObject, because without[*]wildcards child paths are unresolvable while the parent is still an arrayWho is affected
Any upjet-based provider whose Terraform provider uses the plugin framework
NestingModeSinglepattern. The first provider to hit this isgrafana/crossplane-provider-grafana(grafana/crossplane-provider-grafana#549).Fixes #656