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

Don’t add finalizers to skipped objects #173

Conversation

@dlmiddlecote
Copy link
Contributor

dlmiddlecote commented Aug 7, 2019

Issue : #167

Description

Don't add finalizer to object if there are no handlers for it. Now that it is possible to filter objects out of handler execution, this is pertinent.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)

Tasks

  • Add Tests

Review

  • Tests
  • Documentation
dlmiddlecote added 2 commits Aug 7, 2019
Signed-off-by: Daniel Middlecote <dlmiddlecote@gmail.com>
@dlmiddlecote dlmiddlecote requested review from nolar and samurang87 as code owners Aug 7, 2019
@zincr

This comment has been minimized.

Copy link

zincr bot commented Aug 7, 2019

🤖 zincr found 0 problems , 1 warning

ℹ️ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

@dlmiddlecote

This comment has been minimized.

Copy link
Contributor Author

dlmiddlecote commented Aug 8, 2019

Tests seem to be failing on 1 flakey test.

@psycho-ir

This comment has been minimized.

Copy link
Collaborator

psycho-ir commented Aug 8, 2019

Hi @dlmiddlecote,

Thank you so much for the PR!

@nolar
I tested it locally and it works fine in almost all the cases I had in mind.
The only scenario that it doesn't work correctly is when we annotate a resource and it becomes matched with on of the registered handlers, the finalizer won't be added to the resource.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 8, 2019

Codecov Report

Merging #173 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #173      +/-   ##
=========================================
+ Coverage   81.33%   81.4%   +0.07%     
=========================================
  Files          32      33       +1     
  Lines        1639    1678      +39     
  Branches      237     243       +6     
=========================================
+ Hits         1333    1366      +33     
- Misses        260     263       +3     
- Partials       46      49       +3
Impacted Files Coverage Δ
kopf/reactor/handling.py 85.16% <ø> (-0.69%) ⬇️
kopf/reactor/registries.py 89.67% <100%> (+0.2%) ⬆️
kopf/clients/patching.py 87.09% <0%> (-12.91%) ⬇️
kopf/engines/peering.py 28.34% <0%> (ø) ⬆️
kopf/engines/sleeping.py 100% <0%> (ø)
kopf/reactor/queueing.py 91.3% <0%> (+0.98%) ⬆️
@dlmiddlecote

This comment has been minimized.

Copy link
Contributor Author

dlmiddlecote commented Aug 8, 2019

Hey @psycho-ir

Is this the case?

  • resource with no annotations applied => no finalizer applied
  • resource edited (or kubectl annotate used) to add matching annotations => finalizer should be applied?

If so, I just tried this, and it seems to work.

Let me know if I'm mistaken.

def fn(**_):
pass

assert registry.requires_finalizer(resource=resource, body=OBJECT_BODY) is not optional

This comment has been minimized.

Copy link
@nolar

nolar Aug 8, 2019

Contributor

This construct is a bit counter-intuitive. Maybe, parametrize with 2 values: @pytest.mark.parametrize('optional, expected'..., and then compare with == expected.

Also, it is better to put the actual result of a function call to a variable, and then use it in the assertion. Pytest then renders the failed assertions nice (and not so nice in some case when the function call is used: because double-calling for rendering can have side-effects).

Signed-off-by: Daniel Middlecote <dlmiddlecote@gmail.com>
@psycho-ir

This comment has been minimized.

Copy link
Collaborator

psycho-ir commented Aug 8, 2019

Hey @psycho-ir

Is this the case?

  • resource with no annotations applied => no finalizer applied
  • resource edited (or kubectl annotate used) to add matching annotations => finalizer should be applied?

If so, I just tried this, and it seems to work.

Let me know if I'm mistaken.

Hi @dlmiddlecote,

Sorry you are right this scenario works perfectly fine.
What didn't work as expected for me was the other way around:

  • resource with annotation applied => finalizer applied
  • resource patched (annotation removed) => finalizer is still there
@dlmiddlecote

This comment has been minimized.

Copy link
Contributor Author

dlmiddlecote commented Aug 8, 2019

Hey!

I also can't reproduce.

I have the operator as:

import kopf

@kopf.on.delete('', 'v1', 'serviceaccounts', annotations={'foo': 'bar'})
async def foo(**_):
    pass

Then I apply:

apiVersion: v1
kind: ServiceAccount
metadata:
  annotations:
    foo: bar
  name: test
  namespace: default

and the finalizer is applied.

I then run:
kubectl patch sa test -p '{"metadata": {"annotations": {"foo": "baz"}}}'

and the finalizer is removed.

@psycho-ir

This comment has been minimized.

Copy link
Collaborator

psycho-ir commented Aug 8, 2019

Hey!

I also can't reproduce.

I have the operator as:

import kopf

@kopf.on.delete('', 'v1', 'serviceaccounts', annotations={'foo': 'bar'})
async def foo(**_):
    pass

Then I apply:

apiVersion: v1
kind: ServiceAccount
metadata:
  annotations:
    foo: bar
  name: test
  namespace: default

and the finalizer is applied.

I then run:
kubectl patch sa test -p '{"metadata": {"annotations": {"foo": "baz"}}}'

and the finalizer is removed.

Right 😕,

I probably did a mistake in my tests, all the scenarios are working a charm.
Sorry to bother.

@nolar

This comment has been minimized.

Copy link
Contributor

nolar commented Aug 8, 2019

PS: The solution in general is fine.

I suggest that we merge it now, and release as 0.21rcX (x=3..4 or so), together with lots of other bugfixes/refactorings/improvements, and test them altogether.

@psycho-ir

This comment has been minimized.

Copy link
Collaborator

psycho-ir commented Aug 8, 2019

PS: The solution in general is fine.

I suggest that we merge it now, and release as 0.21rcX (x=3..4 or so), together with lots of other bugfixes/refactorings/improvements, and test them altogether.

Totally agree 👍

@nolar
nolar approved these changes Aug 8, 2019
@nolar nolar merged commit 4f4ddd4 into zalando-incubator:master Aug 8, 2019
3 checks passed
3 checks passed
Zincr-bot Found 0 problems, 1 warnings
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 81.556%
Details
@nolar

This comment has been minimized.

Copy link
Contributor

nolar commented Aug 8, 2019

Pre-released as kopf==0.21rc3 — but beware of other massive changes in rc1+rc2+rc3 combined (see Releases).

@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
4 participants
You can’t perform that action at this time.