Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

k8s API sets the default value of claim's spec.compositeDeletePolicy field #4768

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

pedjak
Copy link
Contributor

@pedjak pedjak commented Oct 10, 2023

Description of your changes

  • claim CRD contains now the default value for .spec.compositeDeletePolicy field
    copied from XRD spec.defaultCompositeDeletePolicy. If not specified at claim,
    it will be set by k8s API
  • claim controller does not set the value for spec.compositeDeletePolicy,
    leading to one less claim update, and hence one less no-op reconcile loop
  • added unit tests
  • removed APIDefaultSelector as unneeded

Fixes #4691

I have:

  • Read and followed Crossplane's contribution process.
  • Added or updated unit and E2E tests for my change.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, if necessary.
  • Opened a PR updating the docs, if necessary.

@pedjak pedjak requested review from turkenh and a team as code owners October 10, 2023 14:35
@pedjak pedjak requested a review from ytsarev October 10, 2023 14:35
Copy link
Contributor

@bobh66 bobh66 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so much better than my original implementation - great work!

It LGTM but I'll let someone with more go experience approve it.

internal/xcrd/crd.go Outdated Show resolved Hide resolved
Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I like a mostly red diff, nice work.

Copy link
Contributor

@phisco phisco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small nit, otherwise LGTM! thanks @pedjak 🎉

internal/xcrd/crd_test.go Outdated Show resolved Hide resolved
…` field

* claim CRD contains now the default value for `.spec.compositeDeletePolicy` field
  copied from XRD `spec.defaultCompositeDeletePolicy`. If not specified at claim,
  it will be set by k8s API
* claim controller does not set the value for `spec.compositeDeletePolicy`,
  leading to one less claim update, and hence one less no-op reconcile loop
* added unit and e2e tests
* removed `APIDefaultSelector` as unneeded

Signed-off-by: Predrag Knezevic <predrag.knezevic@upbound.io>
@pedjak pedjak force-pushed the claim-default-deletion-policy branch from ac3f3cc to ca10547 Compare October 16, 2023 09:36
@phisco phisco merged commit e236208 into crossplane:master Oct 16, 2023
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set spec.compositeDeletePolicy default value on claim CRD
4 participants