Define behavior of retention policy in multi-namespace plans.#45
Define behavior of retention policy in multi-namespace plans.#45awels wants to merge 2 commits intokubevirt:mainfrom
Conversation
Before it was undefined what would happen if the user provided both the retention policy on the multi-namespace plan and on the namespace plan. Now the namespace plan inherits from the multi-namespace plan if not set. CEL validation is still used to validate the retention policy. In particular it should prevent the user from removing the retention policy once it is set. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Alexander Wels <awels@redhat.com>
kubevirt-bot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| ) | ||
|
|
||
| // VirtualMachineStorageMigrationPlanSpec defines the desired state of VirtualMachineStorageMigrationPlan | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.retentionPolicy) || has(self.retentionPolicy)", message="retentionPolicy cannot be removed once set" |
There was a problem hiding this comment.
Are you sure self == oldSelf doesn't cover this case? logically I would assume it does
There was a problem hiding this comment.
There is some weirdness that happens when going from nil to something or vice versa. Especially with a default defined. I believe this is to ensure that if we have nil and select the default, it actually 'works'.
| Expect(err).To(HaveOccurred()) | ||
| // Verify the specific CEL error message from your marker | ||
| Expect(err.Error()).To(ContainSubstring("retentionPolicy is immutable")) | ||
| Expect(err.Error()).To(ContainSubstring("retentionPolicy")) |
There was a problem hiding this comment.
this assertion is a little weird, i think you know the exact error message string you're going to get?
There was a problem hiding this comment.
this depends on the outcome of https://github.com/kubevirt/kubevirt-migration-controller/pull/45/changes#r3037032397
There was a problem hiding this comment.
Yeah let me double check and make a more specific error message.
| if plan.Spec.RetentionPolicy != nil && *plan.Spec.RetentionPolicy != migrations.RetentionPolicyKeepSource { | ||
| // Parent has non-default value | ||
| if spec.RetentionPolicy == nil || *spec.RetentionPolicy == migrations.RetentionPolicyKeepSource { | ||
| // Namespace has default or nil - inherit from parent | ||
| spec.RetentionPolicy = plan.Spec.RetentionPolicy | ||
| } | ||
| // else: namespace has explicit non-default value, use it (priority 1) |
There was a problem hiding this comment.
do we want to unit test the priority? doesn't have to involve etcd, could extract to small util and test that
There was a problem hiding this comment.
Sure give me a bit to update this.
Signed-off-by: Alexander Wels <awels@redhat.com>
|
@awels: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@awels FYI linter issue is real |
| if client == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
yes it can, if you manually run a particular test from the IDE, it forgot to set the client.
3 participants
What this PR does / why we need it:
Before it was undefined what would happen if the user provided both the retention policy on the
multi-namespace plan and on the namespace plan. Now the namespace plan inherits from the multi-namespace plan if not set.
CEL validation is still used to validate the retention policy. In particular it should prevent
the user from removing the retention policy once it is set.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes https://redhat.atlassian.net/browse/CNV-82386
Special notes for your reviewer:
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: