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

feat: Add final-deletion-state Flag to Set Custom State When DeletionTimestamp is Set #148

Conversation

LeelaChacha
Copy link
Contributor

Description

Changes proposed in this pull request:

  • Added new flag final-deletion-state to Template-Operator Manager - can be used to set a custom state when deletion timestamp is set (default Deleting)
  • Added default state as a patch in kustomization.yaml

Related issue(s)

Closes #147

@LeelaChacha LeelaChacha requested a review from a team as a code owner February 4, 2024 14:55
@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 4, 2024
@LeelaChacha
Copy link
Contributor Author

Note: The logic is implemented in a way that whatever, state is set, the CR will not leave that state once deletion timestamp is set.

The other way to implement would be setting state to custom state when deletion timstamp is set and then letting the normal controller logic set the state as it is done until now. However, if we take that approach, then some states will be flickering like in parent issue. For eg: if --final-deletion-state is set to Error and deletion timestamp is set, the CR will go in state Error but then the controller would handle the error state and find no errors so it set it back to Ready until the next loop sets the CR back to Error and then the cycle repeats. Therefore, it is important that we stay in the state set in the flag. However, this means that once the flag is set and the controller is running, there is no way to change the flag, that means if a CR is set to Warning state after deletion in a test, The finalizer has to be removed manually in the test teardown, otherwise the CR will persist.

@kyma-bot kyma-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 5, 2024
@LeelaChacha LeelaChacha force-pushed the feature/#147-new-flag-for-deletion-state branch from 1b897ff to f923e1d Compare February 5, 2024 10:19
@LeelaChacha
Copy link
Contributor Author

I repurposed the the old SampleCR tests because I would have had to create a whole new suite for test one field as messing with the reconciler in the suite would mean other tests in the suite would become flaky.

Copy link
Contributor

@jeremyharisch jeremyharisch left a comment

Choose a reason for hiding this comment

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

Default should not be set for a FinalDeletionState. If it is not set, then the template-operator should behave normal, without being stuck in some state. Please revisit the implementation and make needed changes to implement it in described way

config/default/kustomization.yaml Show resolved Hide resolved
main.go Show resolved Hide resolved
@LeelaChacha LeelaChacha changed the title feat: Add final-deletion-state Flag to Set Custom Set When DeletionTimestamp is Set feat: Add final-deletion-state Flag to Set Custom State When DeletionTimestamp is Set Feb 5, 2024
@kyma-bot kyma-bot added the lgtm Looks good to me! label Feb 5, 2024
@kyma-bot kyma-bot merged commit 21d8e24 into kyma-project:main Feb 5, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce new flag to set Final-Deleting-State
3 participants