Add DeepCopy generation to all API types#4527
Conversation
✅ Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
mozesl-nokia
force-pushed
the
deepcopy-all
branch
from
ac3e481 to
b24fa7b
Compare
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
| const maxDepth = 1024 | ||
|
|
||
| func deepCopyInterface(in any, depth uint) any { | ||
| if depth > maxDepth { | ||
| panic(fmt.Sprintf("reached max deepcopy depth of %d", maxDepth)) | ||
| } | ||
|
|
||
| // return all primitive / non-pointer types | ||
| switch t := in.(type) { | ||
| case int, int8, int16, int32, int64, | ||
| uint, uint8, uint16, uint32, uint64, uintptr, | ||
| float32, float64, | ||
| complex64, complex128, | ||
| bool, | ||
| string, | ||
| json.Number: | ||
| return t | ||
| // inspired by k8s.io/apimachinery/pkg/runtime.DeepCopyJSONValue | ||
| case map[string]any: | ||
| clone := make(map[string]interface{}, len(t)) | ||
| for k, v := range t { | ||
| clone[k] = deepCopyInterface(v, depth+1) | ||
| } | ||
| return clone | ||
| case []any: | ||
| clone := make([]interface{}, len(t)) | ||
| for i, v := range t { | ||
| clone[i] = deepCopyInterface(v, depth+1) | ||
| } | ||
| return clone | ||
| default: | ||
| panic(fmt.Sprintf("cannot deepcopy type %T", t)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this is very much overkill, so I'm looking for suggestions on what to do instead.
There was a problem hiding this comment.
The alternative would be not to use go structs coming from kyaml at all, but define our own API structs in the kpt repo. That way we can control our own API schemas.
mozesl-nokia
requested review from
efiacor,
kispaljr and
liamfallon
as code owners
dosubot
Bot
added
size:XL
There was a problem hiding this comment.
Pull request overview
This PR expands DeepCopy support across the kptfile/v1 and fnresult/v1 API types by enabling package-level controller-gen generation, introducing a shared DeepCopy helper for kyaml/fn/framework result types, and updating render status plumbing to store framework.Results directly (removing the bespoke ResultItem mirror types).
Changes:
- Enable package-level
+kubebuilder:object:generate=trueand add/standardizego:generate controller-gen ... objectdirectives for API packages, then check in regeneratedzz_generated.deepcopy.gofiles. - Replace
kptfilev1.PipelineStepResult.Results/ErrorResultsfrom customResultItemslices toframework.Results, and update render executor/test expectations accordingly. - Add
pkg/api/commonDeepCopy utilities (with tests) and manualDeepCopyIntoimplementations for types containingframework.Results.
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/api/kptfile/v1/zz_generated.deepcopy.go | Regenerated DeepCopy methods for all kptfile/v1 API types. |
| pkg/api/kptfile/v1/types.go | Package-level generation annotation; switch PipelineStepResult to framework.Results; remove ResultItem and related mirror types. |
| pkg/api/kptfile/v1/deepcopy.go | Manual DeepCopyInto for PipelineStepResult to correctly deep copy framework.Results. |
| pkg/api/fnresult/v1/zz_generated.deepcopy.go | New generated DeepCopy stubs for fnresult/v1 (works with manual Result.DeepCopyInto). |
| pkg/api/fnresult/v1/types.go | Package-level generation annotation and go:generate for controller-gen. |
| pkg/api/fnresult/v1/deepcopy.go | Manual DeepCopyInto for Result to deep copy framework.Results. |
| pkg/api/common/deepcopy.go | New shared deepcopy helpers for framework.Result(s) and interface field values. |
| pkg/api/common/deepcopy_test.go | Unit tests covering the new common deepcopy helpers. |
| internal/util/render/executor.go | Store framework.Results directly in PipelineStepResult and filter ErrorResults via framework.Error. |
| internal/util/render/executor_test.go | Update assertions for framework.Severity and remove tests for deleted conversion helper. |
| go.sum | Dependency checksum cleanup (removes unused docker/distribution entries). |
Files not reviewed (2)
- pkg/api/fnresult/v1/zz_generated.deepcopy.go: Language not supported
- pkg/api/kptfile/v1/zz_generated.deepcopy.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func DeepCopyInterface(in any) any { | ||
| return deepCopyInterface(in, 0) | ||
| } | ||
|
|
||
| const maxDepth = 1024 | ||
|
|
||
| func deepCopyInterface(in any, depth uint) any { | ||
| if depth > maxDepth { | ||
| panic(fmt.Sprintf("reached max deepcopy depth of %d", maxDepth)) | ||
| } | ||
|
|
||
| // return all primitive / non-pointer types | ||
| switch t := in.(type) { | ||
| case int, int8, int16, int32, int64, | ||
| uint, uint8, uint16, uint32, uint64, uintptr, | ||
| float32, float64, | ||
| complex64, complex128, | ||
| bool, | ||
| string, | ||
| json.Number: | ||
| return t | ||
| // inspired by k8s.io/apimachinery/pkg/runtime.DeepCopyJSONValue | ||
| case map[string]any: | ||
| clone := make(map[string]interface{}, len(t)) | ||
| for k, v := range t { | ||
| clone[k] = deepCopyInterface(v, depth+1) | ||
| } | ||
| return clone | ||
| case []any: | ||
| clone := make([]interface{}, len(t)) | ||
| for i, v := range t { | ||
| clone[i] = deepCopyInterface(v, depth+1) | ||
| } | ||
| return clone | ||
| default: | ||
| panic(fmt.Sprintf("cannot deepcopy type %T", t)) | ||
| } |
There was a problem hiding this comment.
From what I could gather is that these fields should never be unmarshalled as anything other than what DeepCopyJSONValue supports. I did consider (and half-write) a completely generic reflect-based deepcopy, but that seemed even more unnecessary.
Although, the exact type is determined exclusively by what the actual KRM function (or kyaml filter) sets.
If we do want to go the reflect route, there definitely are libraries for it.
| if (*in)[i] != nil { | ||
| in, out := &(*in)[i], &(*out)[i] | ||
| *out = new(framework.Result) | ||
| // (*in).DeepCopyInto(*out) |
There was a problem hiding this comment.
The reason it was left there was to specifically indicate that we are replacing that method.
| step.ExitCode = last.ExitCode | ||
| step.Results = frameworkResultsToItems(last.Results) | ||
| step.Results = last.Results | ||
| for _, ri := range step.Results { |
| Stderr string `yaml:"stderr,omitempty" json:"stderr,omitempty"` | ||
| ExitCode int `yaml:"exitCode" json:"exitCode"` | ||
|
|
||
| Results framework.Results `yaml:"results,omitempty" json:"results,omitempty"` |
There was a problem hiding this comment.
Why do we want to use the kyaml framework Results type here that is missing DeepCopy? Why don't we use our own structure defined in the kpt repo that we control and can generate a proper DeepCopy method for?
There was a problem hiding this comment.
The functions themselves either insert into a *framework.ResourceList (containing *framework.Results) or the equivalent *fnsdk.ResourceList, which used to be a carbon copy, but now has the string version of CurrentValue and ProposedValue (not yet used by catalog functions).
Thus, the functions themselves still need their output converted, which will have to be done on Porch side as well.
We could make it so that the SDK version (an easily serializable one) is used by all functions, and handled accordingly by kpt (no need for conversion then), but that would break compatibility with kyaml filters I assume (if we even care about that).
We have only been generating DeepCopy methods for a few select types, but it would be very useful to do so for all of the API types so it can be easily used by consumers (Porch).
Added the appropriate kubebuilder annotation to be on package level for both the main Kptfile types and fnresult types, the
go generatecomments, then ran the generation.Had to add a manual DeepCopyInto method for fnresultv1.Result, since it has a kyaml framework.Result field, which does not have DeepCopyInto.
Deduplicated
framework.Result(removedResultItem).