feat: Add delete override implementation#977
Conversation
ryanzhang-oss
changed the title
ryanzhang-oss
force-pushed
the
add-delete-override
branch
from
21f6556 to
bf2edde
Compare
ryanzhang-oss
force-pushed
the
add-delete-override
branch
from
a2dded2 to
4e0e73a
Compare
| } | ||
| } | ||
| } | ||
| switch rule.OverrideType { |
There was a problem hiding this comment.
can be replaced by using cel
for example,
// +kubebuilder:validation:XValidation:rule="self.overrideType == 'JSONPatch' || self.jsonPatchOverrides.size() == 0 ",message="jsonPatchOverrides must be empty when type is not JSONPatch"
There was a problem hiding this comment.
good to know but unless we remove this validator, we need to fix it even with the CEL, right?
There was a problem hiding this comment.
yeah, but the fix is much simpler, just need to add the type check.
There was a problem hiding this comment.
I think there is some advantage to keep all the validation logics (and test) together instead of splitting them into two places.
|
|
||
| // If the resource is selected by both ClusterResourceOverride and ResourceOverride, ResourceOverride will win when | ||
| // resolving conflicts. | ||
| // If the resource is selected by both ClusterResourceOverride and ResourceOverride, ResourceOverride will win when resolving conflicts. |
There was a problem hiding this comment.
what happens for a namespace scope resource which is selected by both cro and ro?
for example, cro deletes the resource, and ro overrides the resource.
There was a problem hiding this comment.
we actually have the same problem now when a CRO deletes a field and a ro wants to replace it. This can happen even between any CRO or RO.
| }: true, | ||
| }, | ||
| IsClusterScopedResource: false, | ||
| IsClusterScopedResource: true, |
There was a problem hiding this comment.
should be "false"? deployment is not a cluster scoped resource.
There was a problem hiding this comment.
the fakeInformer logic is weird that this field is not what it's name is about. I think some of the tests were passing simply because this flag were set wrong (so the CROs are skipped).
There was a problem hiding this comment.
Finally figured out where's the bug. The bug is in line 1064. The deployment group should be "app". If you correct there, this value should be set as false.
| }, | ||
| wantErr: controller.ErrUserError, | ||
| }, | ||
| { |
There was a problem hiding this comment.
add more tests about delete type:
- cro delete, ro patch
- cro patch, ro delete
- cro delete, ro delete
ryanzhang-oss
force-pushed
the
add-delete-override
branch
from
3ba0594 to
3a41176
Compare
ryanzhang-oss
force-pushed
the
add-delete-override
branch
from
3473060 to
998d32c
Compare
| }: true, | ||
| }, | ||
| IsClusterScopedResource: false, | ||
| IsClusterScopedResource: true, |
There was a problem hiding this comment.
Finally figured out where's the bug. The bug is in line 1064. The deployment group should be "app". If you correct there, this value should be set as false.
| wantErr: controller.ErrUserError, | ||
| }, | ||
| { | ||
| name: "delete during the clusterResourceOverride", |
There was a problem hiding this comment.
add one more test, delete after patch?
There was a problem hiding this comment.
isn't test 1719 the delete after patch. There is another delete after patch case for a field earlier.
2 participants
Description of your changes
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer