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

Optional finalizers #118

Merged
merged 12 commits into from Jul 3, 2019
Merged

Conversation

@dlmiddlecote
Copy link
Contributor

dlmiddlecote commented Jun 16, 2019

Issue : #24

Description

Adding finalizers to resources means that resources cannot be deleted if the operator is not running. This PR makes sure that finalizers are added only when strictly necessary, i.e. when there are mandatory deletion handlers. If there are no deletion handlers registered, then no finalizers will be added, same if a deletion handler is marked as optional.

Types of Changes

What types of changes does your code introduce? Keep the ones that apply:

  • New feature (non-breaking change which adds functionality)
  • Documentation / non-code

Tasks

List of tasks you will do to complete the PR

  • Add documentation

Review

List of tasks the reviewer must do to review the PR

  • Tests
  • Documentation
@dlmiddlecote dlmiddlecote requested review from nolar and samurang87 as code owners Jun 16, 2019
@zincr

This comment has been minimized.

Copy link

zincr bot commented Jun 16, 2019

🤖 zincr found 0 problems , 0 warnings

✅ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing
Copy link
Contributor

nolar left a comment

Thanks for your PR! Here are some notes on the code itself.

The solution should work in general. Except maybe for the case with initial & CREATE detection — I have some doubts there for some cases (see comments).

I will test the functionality manually maybe in the next 1-2 days — to get the feel of how it behaves in all typical & edge cases, and if there are any "surprises" from the user point of view (though I expect none).

kopf/on.py Outdated Show resolved Hide resolved
kopf/reactor/causation.py Outdated Show resolved Hide resolved
kopf/reactor/handling.py Outdated Show resolved Hide resolved
kopf/reactor/registries.py Outdated Show resolved Hide resolved
@dlmiddlecote

This comment has been minimized.

Copy link
Contributor Author

dlmiddlecote commented Jun 17, 2019

Thank you for your review! I’ll get onto making the changes ASAP.

docs/handlers.rst Outdated Show resolved Hide resolved
@nolar
nolar approved these changes Jun 19, 2019
@nolar

This comment has been minimized.

Copy link
Contributor

nolar commented Jun 26, 2019

@dlmiddlecote Just for information regarding delay: All is okay with this PR. I will merge it a bit later. There is a little issue in the master branch (merged, but not released yet), which I would like to fix and release as a separate version without any intermediate merges; and also to keep the releases' deltas small.

@nolar nolar merged commit a09a748 into zalando-incubator:master Jul 3, 2019
2 checks passed
2 checks passed
Zincr-bot Found 0 problems, 0 warnings
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nolar nolar added the enhancement label Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.