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

Fixed calculation of reconcile event handling for PipePods #1322

Merged
merged 2 commits into from
Feb 3, 2020

Conversation

ANeumann82
Copy link
Member

Signed-off-by: Andreas Neumann aneumann@mesosphere.com

What this PR does / why we need it:

Corrects the calculation if a reconcile should be done for DeleteEvents.

The comment in the code states that it should be skipped for PipePods, but at the moment it triggers reconciles ONLY for PipePods.

Fixes #1321

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
@ANeumann82 ANeumann82 changed the title Fixed calculation of event handling for PipePods Fixed calculation of reconcile event handling for PipePods Jan 31, 2020
Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

nice 👏

as always: would be good to have test added for every regression fixed :) maybe an integration test similar to the case you described in the description?

Added basic unit test for eventFilter delete methods

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
@ANeumann82
Copy link
Member Author

Good Point. I wouldn't like a IT for this as it would probably be flaky if it failed, in my testing today it sometimes worked, sometimes not, probably depending on the order of events that the controller received, and the timing.

I've added a basic unit test though, if we're going to extend the filter we can extend the test as well.

@alenkacz
Copy link
Contributor

@ANeumann82 I think even flaky test is a good indication that something is broken :) but alright...

Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

Nice catch! The original implementation was correct but then I introduced funk library and broke it! 🚢

@ANeumann82 ANeumann82 merged commit 1c0f33c into master Feb 3, 2020
@ANeumann82 ANeumann82 deleted the an/invert-delete-pipepod-event branch February 3, 2020 09:30
ANeumann82 added a commit that referenced this pull request Feb 13, 2020
* Fixed calculation of event handling for PipePods
* Added basic unit test for eventFilter delete methods

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
runyontr pushed a commit that referenced this pull request Mar 11, 2020
* Fixed calculation of event handling for PipePods
* Added basic unit test for eventFilter delete methods

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Thomas Runyon <runyontr@gmail.com>
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.

KUDO ignores reconcile request for Deletion of StatefulSet
3 participants