🐛 inline builders in test for MHC reconcilation #5987
Conversation
k8s-ci-robot
added
cncf-cla: yes
killianmuldoon
changed the title
|
/lgtm |
k8s-ci-robot
added
the
lgtm
killianmuldoon
force-pushed
the
fix/builder-test
branch
from
607603a to
c5d2f5e
Compare
k8s-ci-robot
added
size/M
killianmuldoon
force-pushed
the
fix/builder-test
branch
from
c5d2f5e to
4e30d45
Compare
k8s-ci-robot
added
size/L
internal/test/builder/builders.go
Outdated
| if len(m.conditions) > 0 { | ||
| out.conditions = make([]clusterv1.UnhealthyCondition, len(m.conditions)) | ||
| copy(out.conditions, m.conditions) | ||
| } | ||
| if len(m.ownerRefs) > 0 { | ||
| for _, ref := range m.ownerRefs { | ||
| out.ownerRefs = append(out.ownerRefs, *ref.DeepCopy()) | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: why do we copy those arrays differently?
There was a problem hiding this comment.
My understanding is that because OwnerRefs have pointers in them so they need to be deepcopied to ensure we're getting a copy. UnhealthyConditions only contains values so it should be alright.
There was a problem hiding this comment.
Thx. I'll take a look but that sounds reasonable :)
There was a problem hiding this comment.
The alternative here (and maybe a better option) would be to add deepcopy generation for this file.
There was a problem hiding this comment.
Yup, in the best case it just generates everything for us and if we're fine with calling DeepCopy instead of Clone we don't have to implement anything ourselves/manually.
WDYT, worth a try?
I'm definitely +1 as it's less effort to write and maintain this code and we reduce the probability to forget setting fields or implementing the copy incorrectly to zero.
There was a problem hiding this comment.
It seems the kubebuilder generation doesn't work with map[string]interface{}.
There was a problem hiding this comment.
I disabled generation for those fields - I'm not sure if there's a way to tweak it to make generation of map[string]interface{} work for kubebuilder or if it would be better to stop using that for the spec fields in the builders. For now though those functions aren't needed so I think it's fine to go forward with the PR as it stands.
There was a problem hiding this comment.
Sounds great so far and looks good at a first glance (will take a closer look tomorrow).
I assume "kubebuilder generation" = the deep copy generation of controller-gen? (just to confirm)
There was a problem hiding this comment.
Yeah - I'm doing it here using the kubebuilder tags.
There was a problem hiding this comment.
I think we won't be able to make that work (at least without adjusting controller-gen or changing the data type of the values of the map).
The problem is that controller-gen cannot generate a DeepCopy func for a interface{} (I guess because it can be anything):
https://github.com/kubernetes-sigs/controller-tools/blob/a410b23ff25739563e103e98af887adfd05feb98/pkg/deepcopy/traverse.go#L386-L393
killianmuldoon
force-pushed
the
fix/builder-test
branch
from
4e30d45 to
a5d1b5c
Compare
Signed-off-by: killianmuldoon <[email protected]>
Signed-off-by: killianmuldoon <[email protected]>
killianmuldoon
force-pushed
the
fix/builder-test
branch
from
a5d1b5c to
a55b48b
Compare
| @@ -394,6 +394,7 @@ func (m *MachineDeploymentClassBuilder) Build() *clusterv1.MachineDeploymentClas | |||
| } | |||
|
|
|||
| // InfrastructureMachineTemplateBuilder holds the variables and objects needed to build an InfrastructureMachineTemplate. | |||
| // +kubebuilder:object:generate=false | |||
There was a problem hiding this comment.
Just for my understanding, why are we excluding some builder from generating deep copy?
There was a problem hiding this comment.
The generation didn't work for map[string]interface{} - I'm not sure if there's a way to manage this automatically. This was the simplest solution as we don't strictly need DeepCopy on these types yet, but I'd be happy to fix it. WDYT?
There was a problem hiding this comment.
Thanks for the explanation.
If we will need DeepCopy for those types we can either implement it manually or consider using something like apiextensionsv1.JSON (there are also other options), but I ok in tacking thin in a follow up iteration
There was a problem hiding this comment.
Using something like apiextensionsv1.JSON sounds reasonable to me, too. Let's open an issue?
k8s-ci-robot
added
the
lgtm
|
@sbueringer PTAL |
|
/lgtm I like how we now leverage controller-gen to avoid writing error-prone code manually. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Signed-off-by: killianmuldoon [email protected]
Inline builders for the reconcileMachineDeploymentTests. Previously this test was written so that side effects could occur from one test case to the next.
c/f #5949 (comment)