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 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds an optional ChangesGo runtime object generation
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Commands
participant AllLanguages
participant goGenerator
participant GeneratedModels
CLI->>Commands: provide GenerateGoRuntimeObjects
Commands->>AllLanguages: WithGoRuntimeObjects(enabled)
AllLanguages->>goGenerator: configure runtimeObjects
goGenerator->>GeneratedModels: emit runtime.Object and scheme code
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 |
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>
75d816f to
56a98a2
Compare
Apply accessor generation after Go post-processing at the call sites via a new applyAccessors helper, rather than folding it into generateGo. This mirrors the runtime.Object generation approach in crossplane#162 so both features use the same "post-process the generated code after the fact" pattern, and makes generateGo's signature identical across both, keeping the two changes easy to reconcile. Generating accessors after fixK8sTypeNames/removeSelfImports also means they reference the final type names. Skip unexported struct fields when emitting accessors: generated models don't currently have any, but an accessor for one would be useless to external consumers and could produce oddly-cased method names. Reuse receiverTypeName from accessors.go in the tests instead of redefining an equivalent renderRecv helper. Signed-off-by: Erik Miller <erik.miller@gusto.com>
Honor features.generateGoRuntimeObjects in `crossplane function generate`. Run built the schema manager with a bare generator.AllLanguages(), so the command ignored the feature flag and always emitted the default Go models even when the user opted in. Thread the flag through as project build/run already do. Fold the two addRuntimeObjects tests into a single table-driven test with named cases and reason fields, matching the repo's test conventions. Signed-off-by: Erik Miller <erik.miller@gusto.com>
Extract the shared-K8s-package generation loop body into a generateSharedK8sPackage helper so GenerateFromCRD stays under the gocognit complexity limit, matching the accessors PR. Drop the dead `scalars`/collectScalarTypes plumbing: classifyElem treats every non-struct as scalar and never consulted the set, so it was threaded through the whole deep-copy chain unused (flagged by revive). Also drop the unused fset parameter from writeSliceCopy. No behavior change. Signed-off-by: Erik Miller <erik.miller@gusto.com>
adamwg
left a comment
There was a problem hiding this comment.
This looks good to me overall, and I like that #160 and this PR now work similarly.
One bug I noticed in testing: the core/v1 API group gets the wrong group name in groupversion_info.go:
var GroupVersion = schema.GroupVersion{Group: "core.k8s.io", Version: "v1"}I think this one is a special case, since the Group should actually be empty. Other built-in groups I spot checked (apps/v1, networking.k8s.io/v1, batch/v1) look correct.
| // generateGoRuntimeObjects feature is enabled. It additionally requires | ||
| // k8s.io/apimachinery (used by the generated runtime.Object and AddToScheme | ||
| // code) and its transitive dependencies. | ||
| const goModContentsRuntimeObjects = `module dev.crossplane.io/models |
There was a problem hiding this comment.
I think there would be no harm in using this go.mod and go.sum unconditionally. There will be a couple of superfluous entries when runtime.Object generation is disabled, but go build is fine with that and go mod tidy will clean it up if anyone cares. One thing fewer to maintain and test if we just have one version.
…ation
Write a single go.mod and go.sum for the generated models module rather than
one variant per feature-flag state. The module now always requires
k8s.io/apimachinery: an unused requirement is harmless to `go build`, and one
set of module files is one thing fewer to maintain and test. Generated output
with the feature off is therefore no longer byte-for-byte identical to before -
only the module files differ, and they resolve the same way against the Go
function template's go.sum.
Register built-in Kubernetes types under their real API group. The group labels
the generator uses for built-in packages are synthetic and only drive the
directory layout, so core/v1 and meta/v1 were getting
`GroupVersion{Group: "core.k8s.io"}` and `{Group: "meta.core.k8s.io"}` in their
groupversion_info.go. Those types are all in the core (empty) group, so
AddToScheme registered them under a GVK that disagreed with the GVK the type's
own GroupVersionKind() reports. Real groups from CRDs, and the built-in labels
that are also real groups (autoscaling), pass through unchanged.
Put the compile gates behind a `compilegate` build tag. They shell out to the Go
toolchain to build the generated module, so they need network access and a
writable module cache; the Nix sandbox our unit tests run in has neither, and
the generated module pins the apimachinery version the function template uses
rather than the one this repository depends on, so it can't resolve from
gomod2nix either. Failing on a broken `go mod download` (the previous behaviour)
therefore broke `nix build .#checks.<system>.test`. A build tag is more honest
than a runtime skip, which still reports as a test that ran, and
.golangci.yml now lints the tagged file so it can't rot. Run them with:
go test -tags compilegate ./internal/schemas/generator/...
The gate also covers the flag-off module now, since both states share go.mod.
Mark `runtime.Object`, `runtime.Scheme` and `AddToScheme` as inline code in the
config help text. Vale reads the generated command reference as prose and flags
the dotted identifiers as sentences missing a space, failing validate-docs.
Cover the feature-flag plumbing: AllLanguages threading the option into the Go
generator, and function generate's Run with the flag on.
Signed-off-by: Erik Miller <erik.miller@gusto.com>
The compile gate for the OpenAPI path only built the generated module, so it couldn't catch the class of bug the API group fix is about: several built-in packages now share the core (empty) group, and runtime.Scheme.AddKnownTypes panics if two Go types claim the same GVK. Register every generated built-in package in a single scheme and assert each type is known by the GVK its own apiVersion reports. Reverting the group mapping fails this test. Signed-off-by: Erik Miller <erik.miller@gusto.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/schemas/generator/go.go (1)
636-675: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUser-facing messages in
writeGroupVersionInfo.Thanks for adding the scheme setup path. The wrapper import is already
crossplane-runtime/pkg/errors, but the surrounding messages still describe generator internals; could the messages instead say what package/schema setup failed, such as “failed to write generated scheme registration for package...”?🤖 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/go.go` around lines 636 - 675, Update the error messages in writeGroupVersionInfo to describe the user-facing failure in generated scheme registration, including the relevant package or schema context where available, instead of internal operations like stat, format, create, or write. Preserve error wrapping and the existing behavior for each failure path.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/go.go`:
- Around line 333-336: Update the k8sPkgMetaV1 handling in the package/group
generation flow to reuse getK8sPackageInfo(pkg) instead of assigning
meta.k8s.io, meta, and v1 directly. Preserve the canonical meta.core.k8s.io
mapping and apiGroup synthetic-group behavior used by the OpenAPI path.
---
Nitpick comments:
In `@internal/schemas/generator/go.go`:
- Around line 636-675: Update the error messages in writeGroupVersionInfo to
describe the user-facing failure in generated scheme registration, including the
relevant package or schema context where available, instead of internal
operations like stat, format, create, or write. Preserve error wrapping and the
existing behavior for each failure path.
🪄 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 Plus
Run ID: 507815b2-aef6-47c1-b768-2c7195b9ee23
📒 Files selected for processing (16)
.golangci.ymlcmd/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/interface_test.gointernal/schemas/generator/runtimeobject.gointernal/schemas/generator/runtimeobject_compilegate_test.gointernal/schemas/generator/runtimeobject_integration_test.gointernal/schemas/generator/runtimeobject_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- cmd/crossplane/project/build.go
- cmd/crossplane/config/help/config.md
- cmd/crossplane/project/run.go
- internal/config/config.go
- cmd/crossplane/function/generate_test.go
- cmd/crossplane/config/set.go
- cmd/crossplane/main.go
- internal/schemas/generator/interface.go
- internal/schemas/generator/runtimeobject.go
Replaces the group label -> API group lookup added in the previous commit. The
labels the generator uses for built-in Kubernetes packages only drive the
directory layout, so reverse-mapping them was fragile in both directions:
- It missed the CRD path, which labels the built-in metav1 package
`meta.k8s.io` rather than the OpenAPI path's `meta.core.k8s.io` (the two
paths deliberately emit it to different directories), so that package was
registering its types under `meta.k8s.io/v1`.
- `meta.k8s.io` and `resource.k8s.io` are also real Kubernetes API groups, so
a CRD in either would have had its group rewritten to "".
Thread a goPackage through writeGoCode instead, holding the layout group and the
API group separately. Every producer states which it has: CRDs and OpenAPI
schemas with an x-kubernetes-group-version-kind extension have a real group and
use packageInGroup; the built-in packages set a synthetic layout label and the
real API group (core, i.e. empty) side by side.
Signed-off-by: Erik Miller <erik.miller@gusto.com>
Upstream main dropped the `level: error` input from the Vale step, so reviewdog now runs at `-level=info` and Vale warnings fail validate-docs. "so generated types can be registered in a runtime.Scheme" trips write-good.Passive. Verified with Vale 3.14.2 against the crossplane/docs config at the alert level the action uses: 0 errors, 0 warnings. Signed-off-by: Erik Miller <erik.miller@gusto.com>
adamwg
left a comment
There was a problem hiding this comment.
I gave the latest version of this a test and it is working 👍 I'm able to use the generated schemas with SDK helpers like resource.AsObject and comopsed.From, which lets me eliminate the manual JSON round-trip conversions in my example.
The only thing missing, as in #160, is threading the config through to the other locations where we generate schemas: crossplane dependency ... and the render commands.
Description of your changes
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, noruntime.Object,DeepCopyorgroupversion_info.gocode is emitted.Dependencies
The generated models module requires
k8s.io/apimachinery, pinned to v0.33.0to match the function Go template — so a generated function that consumes the
models via
replacestill resolves everything from the template's existinggo.sum(verified locally by building a function against the models using theshipped template
go.mod/go.sum).There is one
go.mod/go.sumregardless of the flag. With the flag off nothingimports apimachinery, but the requirement stays: an unused requirement is
harmless to
go build,go mod tidyremoves it for anyone who cares, and oneset of module files is one thing fewer to maintain and test.
Testing
Unit tests for the DeepCopy / runtime.Object generation (root vs non-root
detection, scalar/struct/alias field handling), for the API group written into
groupversion_info.gofor built-in Kubernetes packages, and for the featureflag plumbing.
A default-off test confirming no
runtime.Objectartifacts are emitted.Compile gates that materialize the generated module (the CRD path, the
OpenAPI path, and the flag-off path) and
go build/go testit — thesecatch
DeepCopyIntocodegen bugs that parse cleanly but don't type-check. Oneof them is a behavioral test asserting deep-copy independence for scalar,
slice-of-struct and map fields,
AddToSchemeGVK round-tripping, and thatSetGroupVersionKindwrites the typed fields.The compile gates shell out to the Go toolchain to resolve the generated
module's dependencies, so they need network access and a writable module
cache. The Nix sandbox our unit tests run in has neither, and the generated
module pins the apimachinery version the function template uses rather than
the one this repo depends on, so it can't resolve from gomod2nix either. They
are therefore behind a
compilegatebuild tag rather than a runtime skip,which would report as a test that ran:
go test -tags compilegate ./internal/schemas/generator/....golangci.ymllints the tagged file so it can't rot. Happy to add anetwork-enabled CI job that runs them as a follow-up if you'd like the gate
enforced on every PR — didn't want to add a non-hermetic job unilaterally.
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.
I have:
./nix.sh 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.On the two struck items: the
crossplane confighelp text this PR extends isrendered into the generated command reference, so
crossplane/docspicks the newkey up without a separate change. And this is a new opt-in feature rather than a
fix, so it isn't a backport candidate.
Need help with this checklist? See the cheat sheet.