generator: make generated Go types implement runtime.Object and provide AddToScheme#162
generator: make generated Go types implement runtime.Object and provide AddToScheme#162erikmiller-gusto wants to merge 1 commit into
Conversation
erikmiller-gusto
requested review from
a team,
jcogilvie and
tampakrap
as code owners
|
Warning Review limit reached
More reviews will be available in 32 minutes and 52 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 To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. 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 (13)
📝 WalkthroughWalkthroughAdds an optional ChangesGo runtime object generation
Sequence Diagram(s)sequenceDiagram
participant main as cmd/crossplane/main.go
participant run as cmd/crossplane/project/run.go
participant iface as internal/schemas/generator/interface.go
participant go as internal/schemas/generator/go.go
participant aug as addRuntimeObjects
main->>run: bind cfg into Kong context
run->>iface: AllLanguages(WithGoRuntimeObjects(cfg.Features.GenerateGoRuntimeObjects))
iface->>go: build goGenerator{runtimeObjects: enabled}
go->>aug: rewrite generated Go source with runtime.Object support
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/crossplane/function/generate_test.go (1)
291-328: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPlease add a flag-enabled regression case here.
Thanks for updating the signature. This still passes only
&config.Config{}, so it won't catchRunforgetting to threadGenerateGoRuntimeObjectsinto schema generation. Could you add aruncase withFeatures.GenerateGoRuntimeObjects: trueand assert the generated Go models include the runtime-object artifacts?🤖 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 `@cmd/crossplane/function/generate_test.go` around lines 291 - 328, Add a new Run regression case in TestRunErrors that exercises generateCmd.Run with config.Config.Features.GenerateGoRuntimeObjects enabled, so the test verifies schema generation still threads that flag through. Update the existing run-path setup in generate_test.go to use a config with Features.GenerateGoRuntimeObjects: true and assert the generated Go model artifacts include the runtime-object outputs, using generateCmd.Run and the related schema generation helpers as the main reference points.cmd/crossplane/function/generate.go (1)
148-161: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
features.generateGoRuntimeObjectsin schema generation.Thanks for threading
cfgintoRun. Did you mean to mirrorcmd/crossplane/project/build.goandcmd/crossplane/project/run.gohere? Line 160 still builds the schema manager withgenerator.AllLanguages()only, socrossplane function generateignores the new config key and keeps generating the default Go models even when users opt in.Suggested fix
schemaMgr := manager.New( c.schemasFS, - generator.Filter(generator.AllLanguages(), c.proj.Spec.Schemas.GetLanguages()), + generator.Filter( + generator.AllLanguages(generator.WithGoRuntimeObjects(cfg.Features.GenerateGoRuntimeObjects)), + c.proj.Spec.Schemas.GetLanguages(), + ), runner.NewRealSchemaRunner(runner.WithImageConfig(c.proj.Spec.ImageConfigs)), )🤖 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 `@cmd/crossplane/function/generate.go` around lines 148 - 161, The schema generation path in generateCmd.Run is still hardcoded to generator.AllLanguages(), so it ignores features.generateGoRuntimeObjects from cfg and keeps producing the default Go models. Update the schema manager setup in Run to mirror the config-aware filtering used in project build/run, using cfg to decide whether Go runtime objects should be included or excluded. Keep the change localized around generateCmd.Run, generator.Filter, and manager.New so the selected schema languages honor the new feature flag.
🧹 Nitpick comments (1)
internal/schemas/generator/runtimeobject_test.go (1)
53-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThanks for adding focused coverage — would you mind table-driving these cases?
They already follow the PascalCase/std-lib parts of the repo test conventions, but this path expects args/want-style tables with named cases. Folding the DeepCopy and root-type scenarios into one table would make the next generator edge case much easier to add without duplicating setup. As per path instructions,
**/*_test.go: "Enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern, use cmp.Diff with cmpopts.EquateErrors() for error testing. Check for proper test case naming and reason fields."🤖 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/schemas/generator/runtimeobject_test.go` around lines 53 - 143, The two separate tests in addRuntimeObjects should be folded into a single table-driven test using named cases, args/want-style inputs, and shared setup so future generator edge cases are easier to add. Update TestAddRuntimeObjectsDeepCopy and TestAddRuntimeObjectsRootType to use a table with clear case names and expected method sets, while keeping the existing assertions around addRuntimeObjects and roMethods; preserve the current coverage for DeepCopy-only vs root-type method generation and make the structure match the *_test.go table-driven convention.Source: Path instructions
🤖 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 `@internal/schemas/generator/runtimeobject_integration_test.go`:
- Around line 85-92: The integration test checks for k8s.io/apimachinery in
generated models/go.mod, but it currently skips when dependency resolution
fails, which can hide broken or drifting generated deps. Update the
runtimeobject integration test(s) to fail the test instead of calling t.Skipf
when go mod download or similar dependency resolution breaks, and use an
explicit opt-out for offline/local runs only if needed. Keep the assertion
around the generated go.mod content in the same test flow so compile-gate
regressions in the runtimeobject generator are caught by CI.
In `@internal/schemas/generator/runtimeobject.go`:
- Around line 149-172: Named map/slice aliases are being treated as scalars, so
DeepCopy emits shallow pointer copies for fields like *Labels and shares backing
storage. Update the type classification path in collectScalarTypes/classifyElem
to resolve local named aliases before falling back to scalar handling, so named
collection types are recognized as composite types and copied deeply. Then
adjust writeFieldCopy to use the deep-copy path for those aliases instead of
generating **out = **in.
---
Outside diff comments:
In `@cmd/crossplane/function/generate_test.go`:
- Around line 291-328: Add a new Run regression case in TestRunErrors that
exercises generateCmd.Run with config.Config.Features.GenerateGoRuntimeObjects
enabled, so the test verifies schema generation still threads that flag through.
Update the existing run-path setup in generate_test.go to use a config with
Features.GenerateGoRuntimeObjects: true and assert the generated Go model
artifacts include the runtime-object outputs, using generateCmd.Run and the
related schema generation helpers as the main reference points.
In `@cmd/crossplane/function/generate.go`:
- Around line 148-161: The schema generation path in generateCmd.Run is still
hardcoded to generator.AllLanguages(), so it ignores
features.generateGoRuntimeObjects from cfg and keeps producing the default Go
models. Update the schema manager setup in Run to mirror the config-aware
filtering used in project build/run, using cfg to decide whether Go runtime
objects should be included or excluded. Keep the change localized around
generateCmd.Run, generator.Filter, and manager.New so the selected schema
languages honor the new feature flag.
---
Nitpick comments:
In `@internal/schemas/generator/runtimeobject_test.go`:
- Around line 53-143: The two separate tests in addRuntimeObjects should be
folded into a single table-driven test using named cases, args/want-style
inputs, and shared setup so future generator edge cases are easier to add.
Update TestAddRuntimeObjectsDeepCopy and TestAddRuntimeObjectsRootType to use a
table with clear case names and expected method sets, while keeping the existing
assertions around addRuntimeObjects and roMethods; preserve the current coverage
for DeepCopy-only vs root-type method generation and make the structure match
the *_test.go table-driven convention.
🪄 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: 799a4495-015a-4313-a14e-5e2a177d5c30
📒 Files selected for processing (13)
cmd/crossplane/config/help/config.mdcmd/crossplane/config/set.gocmd/crossplane/function/generate.gocmd/crossplane/function/generate_test.gocmd/crossplane/main.gocmd/crossplane/project/build.gocmd/crossplane/project/run.gointernal/config/config.gointernal/schemas/generator/go.gointernal/schemas/generator/interface.gointernal/schemas/generator/runtimeobject.gointernal/schemas/generator/runtimeobject_integration_test.gointernal/schemas/generator/runtimeobject_test.go
…ne#143) Add opt-in generation of controller-gen-style DeepCopy methods, runtime.Object and schema.ObjectKind methods on root types, and a per-package groupversion_info.go (GroupVersion/SchemeBuilder/AddToScheme), so generated Go models can be registered in a runtime.Scheme and used with k8s ecosystem libraries. This lets users register types instead of setting apiVersion/kind by hand. Gated behind a new features.generateGoRuntimeObjects config flag (off by default), threaded from config through the project build/run and function generate commands. When enabled, the generated models module additionally depends on k8s.io/apimachinery (v0.33.0, matching the function go template so a generated function that consumes the models still builds). Root types are detected structurally (APIVersion+Kind+Metadata fields). The runtime.Object pass runs after the k8s type-name fixups so it sees final type names, and copies known stdlib-backed alias types (Time, MicroTime, FieldsV1, RawExtension) by value rather than calling DeepCopyInto. Verified by compile gates that build the generated module (CRD and OpenAPI paths) plus a behavioral test asserting deep-copy independence for scalar, slice-of-struct and map fields and AddToScheme GVK round-tripping. Signed-off-by: Erik Miller <erik.miller@gusto.com>
erikmiller-gusto
force-pushed
the
runtime-object-scheme
branch
from
75d816f to
56a98a2
Compare
1 participant
Fixes #143.
What
Generates
runtime.Objectsupport for the Go schema models, behind a new opt-infeature flag. When enabled, generated Go models can be registered in a
runtime.Schemeand used with k8s ecosystem libraries, so users no longer haveto set
apiVersion/kindby hand on composed resources.For every generated struct:
DeepCopyInto/DeepCopy.For root types (structs with
APIVersion+Kind+Metadata, i.e. theresource and its
List):DeepCopyObject() runtime.ObjectGetObjectKind/GroupVersionKind/SetGroupVersionKind(the type implementsschema.ObjectKind, reading/writing the typedAPIVersion/Kindfields)init()registering the type with the packageSchemeBuilder.Per package containing root types, a
groupversion_info.godefiningGroupVersion,SchemeBuilderandAddToScheme(apimachinery-only; nocontroller-runtime dependency).
Feature gate
Off by default; enable with:
The flag is threaded from config through the
project build,project runandfunction generatecommands. When off, generated output (includinggo.mod/go.sum) is byte-for-byte unchanged.Dependencies
When the flag is on, the generated models module additionally requires
k8s.io/apimachinery, pinned to v0.33.0 to match the function Go template —so a generated function that consumes the models via
replacestill resolveseverything from the template's existing
go.sum(verified locally by building afunction against flag-on models using the shipped template
go.mod/go.sum).Testing
detection, scalar/struct/alias field handling).
OpenAPI generation paths) and
go build/go testit — these catchDeepCopyInto codegen bugs that parse cleanly but don't type-check. They skip
gracefully when modules can't be resolved offline.
slice-of-struct and map fields,
AddToSchemeGVK round-tripping, and thatSetGroupVersionKindwrites the typed fields.go.modis unchangedwhen the flag is disabled.
Known limitation
DeepCopyIntodecides struct-vs-scalar for cross-package types via ASTheuristics, with a small denylist of k8s alias types that have no
DeepCopyInto(
Time,MicroTime,FieldsV1,RawExtension). That denylist was validatedagainst the test fixtures (incl. the large
azure_linux_function_appCRD); areal CRD referencing a k8s alias type not covered by the fixtures could emit a
DeepCopyIntocall on a type that lacks it. The compile gate catches this foranything in the fixtures; broadening detection (e.g. deriving the alias set
programmatically) is a possible follow-up.