use update instead of patch on applying trait #283
Conversation
| patchingClient resource.Applicator | ||
| updatingClient resource.Applicator |
There was a problem hiding this comment.
I think we should answer why we need two client with same type here. Everyone review this code will have the same question. Add the answer to annotation here.
There was a problem hiding this comment.
Yep.
- patchingClient will be used to patching-apply Workload objs while updatingClient will be used to updating-apply Trait objs. As Trait apply should use update instead of patch #272 described, patching-apply cannot remove fields that are present before applying but absent now, that's not expected for Trait. So we use update instead of patch for applying Trait.
- The applying strategy on Workload is TBD, so just keep patchingClient unchanged.
test/e2e-test/trait_update_test.go
Outdated
| @@ -0,0 +1,196 @@ | |||
| package controllers_test | |||
There was a problem hiding this comment.
please try refactor it to be unit-test, thanks! Only necessary cases use e2e test, this could be a unit test like dependency
upgrade crossplane/crossplane-runtime 0.8.0=>0.10.0 add e2e-test Signed-off-by: roy wang <[email protected]>
captainroy-hy
force-pushed
the
fix-trait-apply
branch
from
a0f22da to
f11ab7c
Compare
Signed-off-by: roy wang <[email protected]>
| @@ -95,7 +98,7 @@ func (a *workloads) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, | |||
| continue | |||
| } | |||
| t := trait.Object | |||
| if err := a.client.Apply(ctx, &trait.Object, ao...); err != nil { | |||
| if err := a.updatingClient.Apply(ctx, &trait.Object, ao...); err != nil { | |||
There was a problem hiding this comment.
Why not change Apply() to update? That's more clear. Otherwise it still looks similar to kubectl apply.
There was a problem hiding this comment.
The primary concern is that if use client.Update(), we need more code, e.g., Get/Check-IfNotFound-Create/IfFound-SetResourceVersion-Update, just like what underlying of Apply() has done. So maybe we can just re-use Apply() here.
to fix #272
Solution: Add a new
updatingClientto update trait while retainpatchingClientto apply workload.add e2e-test
Signed-off-by: roy wang [email protected]