Skip to content
This repository was archived by the owner on Sep 14, 2020. It is now read-only.

Avoid purging the annotations that are already absent #362

Merged
merged 2 commits into from
May 11, 2020
Merged

Avoid purging the annotations that are already absent #362

merged 2 commits into from
May 11, 2020

Conversation

nolar
Copy link
Contributor

@nolar nolar commented May 8, 2020 •
edited
Loading

What do these changes do?

Fix a potential bug when unnecessary state purging could prevent the following handling cycles from happening.

Description

This bug was not detected in practice (probably), but it could happen so that purging of a persisted annotation with a handler's state (by patching it with None) could lead to actual absence of changes on the server side if that annotation was already absent, but preventing a dummy-patch to be applied in Kopf because the main patch is not empty.

This, in turn, could lead to no watch-events reported by Kubernetes API (as nothing has changed), and Kopf would not react with the following steps until the next (probably unrelated) change.

Now, with this fix, if the annotation is already absent in the body, it will not be nullified in the patch, letting a dummy-patch to be applied to trigger the next steps.

The only meaningful change is the purge() method. Other methods are switched to the dicts methods for consistency with the status storage (it is easier to manage the annotations that way).

Issues/PRs

Related: detected while working on #360.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactoring (non-breaking change which does not alter the behaviour)

Checklist

  • The code addresses only the mentioned problem, and this problem only
  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

@nolar nolar added the bug Something isn't working label May 8, 2020
@zincr
Copy link

zincr bot commented May 8, 2020 •
edited
Loading

🤖 zincr found 0 problems , 0 warnings

✅ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

@zincr
Copy link

zincr bot commented May 8, 2020

🤖 zincr found 1 problem , 0 warnings

❌ Approvals
✅ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below

Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

@nolar nolar requested review from 3abdelazim and mnarodovitch May 11, 2020 08:47
@nolar nolar added this to the 0.27 milestone May 11, 2020
@nolar nolar merged commit dd0af25 into zalando-incubator:master May 11, 2020
@nolar nolar deleted the 356-no-effect-patches branch May 11, 2020 16:24
@kopf-archiver kopf-archiver bot mentioned this pull request Aug 19, 2020
Closed
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@3abdelazim 3abdelazim Awaiting requested review from 3abdelazim

1 more reviewer

@mnarodovitch mnarodovitch mnarodovitch approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
0.27: daemons, timers, configs, storages
Development

Successfully merging this pull request may close these issues.
2 participants
@nolar @mnarodovitch