🌱 Add in-place update hooks to API: Fix review findings#1
Conversation
| @@ -105,7 +105,11 @@ linters: | |||
| ## Excludes for current clusterctl v1alpha3 and Runtime Hooks v1alpha1 apiVersions (can be fixed once we bump their apiVersion). | |||
| # Note: The types in api/runtime/hooks/v1alpha1 are not CRDs, so e.g. SSA markers don't make sense there. | |||
| - path: "cmd/clusterctl/api/v1alpha3|api/runtime/hooks/v1alpha1" | |||
There was a problem hiding this comment.
The main goal is just to get the marshaling right (pointer, omitempty, omitzero, ...) as these are not regular Kubernetes APIs
|
|
||
| // PatchType defines the supported patch types. | ||
| // +enum | ||
| // +kubebuilder:validation:Enum=JSONPatch;JSONMergePatch |
There was a problem hiding this comment.
Just to make the linter (KAL) work better
| "k8s.io/apimachinery/pkg/runtime" | ||
|
|
||
| clusterv1beta1 "sigs.k8s.io/cluster-api/api/core/v1beta1" | ||
| clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" |
There was a problem hiding this comment.
When implementing my prototype I noticed that it would be better if we directly use v1beta2. It's not 100% consistent with our other hooks but v1beta2 is much better from a marshalling perspective and I don't want to run into trouble because of that
(also we can avoid down conversion in our controllers)
There was a problem hiding this comment.
I had a feeling Fabrizio mentioned we should use v1beta1 here
There was a problem hiding this comment.
Yes he did. We just realized later that it's not a good idea (we initially wanted to use v1beta1 to use the same as in the other lifecycle hooks, but it's not worth the downsides)
| // current contains the current state of the Machine and related objects. | ||
| // +required | ||
| Current CanUpdateMachineRequestObjects `json:"current"` | ||
| Current CanUpdateMachineRequestObjects `json:"current,omitempty,omitzero"` |
There was a problem hiding this comment.
API conventions currently recommend to use omitempty + omitzero on all "struct fields" (doesn't matter if optional or required)
| BootstrapConfig *runtime.RawExtension `json:"bootstrapConfig,omitempty"` | ||
| } | ||
|
|
||
| // Patch is a single patch (JSONPatch or JSONMergePatch) which can include multiple operations. |
There was a problem hiding this comment.
Just moved some structs around a bit for consistent top-down ordering
| // machinePatch when applied to the current Machine spec, indicates changes handled in-place. | ||
| // +optional | ||
| MachinePatch *Patch `json:"machinePatch,omitempty"` | ||
| MachinePatch Patch `json:"machinePatch,omitempty,omitzero"` |
There was a problem hiding this comment.
We don't need the pointers anymore, omitzero handles correct marshalling (also matches regular API conventions for CRD API types)
Also reduces potential for nil pointers
There was a problem hiding this comment.
There was a problem hiding this comment.
I think the short answer is, the trade-offs are a bit different for CRDs, not sure how much of the guidance there is for builtin types. I think they are also not yet recommending to use omitzero everywhere, but we are doing it everywhere in core CAPI
We were discussion this with Joel when we refactored everything for v1beta2 and this is inline with what we did there
(just to give some additional context)
| // machine is the full MachineSet object. | ||
| // +required | ||
| MachineSetSpec clusterv1beta1.MachineSetSpec `json:"machineSetSpec"` | ||
| MachineSet clusterv1.MachineSet `json:"machineSet,omitempty,omitzero"` |
There was a problem hiding this comment.
I think we should also use the full objects here (but also here we won't send status)
| MachineSetPatch Patch `json:"machineSetPatch,omitempty,omitzero"` | ||
|
|
||
| // infrastructureMachineTemplateSpecPatch indicates infra template spec changes handled in-place. | ||
| // infrastructureMachineTemplatePatch indicates infra template spec changes handled in-place. |
There was a problem hiding this comment.
Just consistently dropping Spec from the field names here (the patches should only contain spec changes as documented, I just wouldn't surface it in field names)
sbueringer
force-pushed
the
inplacehooks-review
branch
2 times, most recently
from
df75245 to
3338f81
Compare
| @@ -30,10 +30,14 @@ import ( | |||
| // Prior art: https://github.com/kubernetes-sigs/controller-tools/blob/master/pkg/crd/known_types.go. | |||
| func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenAPIDefinition { | |||
| return map[string]common.OpenAPIDefinition{ | |||
There was a problem hiding this comment.
Signed-off-by: Stefan Büringer buringerst@vmware.com
sbueringer
force-pushed
the
inplacehooks-review
branch
from
3338f81 to
10b4a8b
Compare
2 participants
Signed-off-by: Stefan Büringer buringerst@vmware.com
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):xref: kubernetes-sigs#12343