Skip to content

Conversation

@Diaphteiros
Copy link
Contributor

@Diaphteiros Diaphteiros commented Oct 23, 2025

What this PR does / why we need it:
With this PR, the cluster controller waits with deleting the cluster until all other finalizers have been removed from the Cluster resource. This is helpful, because other controllers that put finalizers on the Cluster resource likely need access to the cluster to clean up whatever they deployed onto it, which is not possible anymore if the cluster is removed as soon as the Cluster resource gets a deletion timestamp.

Which issue(s) this PR fixes:
Part of openmcp-project/backlog#313
Fixes #81

Special notes for your reviewer:

Release note:

ClusterProvider `kind` now waits with deleting a cluster until all foreign finalizers have been removed from the corresponding `Cluster` resource.

Copy link
Member

@maximiliantech maximiliantech left a comment

Choose a reason for hiding this comment

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

Can you please put the search algorithm in a separate function and add some unit tests for it please?

@Diaphteiros
Copy link
Contributor Author

Diaphteiros commented Oct 29, 2025

Can you please put the search algorithm in a separate function and add some unit tests for it please?

I moved the logic into a small helper function.

Regarding the tests:

The helper method is not exported, which means that unit tests for it would only be possible if the testing package is the same as the code package. I usually use *_test as package for unit tests, in which case testing this function would not be possible. But I feel like this is more of a personal preference than based on any objective arguments.

Instead of testing the helper function, we could also add tests using the controller_test package, which test the whole reconciliation logic instead of the single function. However, since the package does currently not have any tests at all, I would consider this to be out of scope for this PR.

@maximiliantech How should we continue here? Adding a small test for the helper function only or merging this now and create unit tests for the whole reconciliation logic in a separate task?

@maximiliantech
Copy link
Member

Thanks for adding this as a helper function. I would vote for writing a unit test as part of this PR to simply increase the confidence of the implementation. I would say we don't use the package *_test approach but rather keep the actual function private and define the controller_test.go with package controller and write the unit test for it.

The test for the whole reconciliation logic can be done in a separate issue/PR.

Copy link
Member

@maximiliantech maximiliantech left a comment

Choose a reason for hiding this comment

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

Reviewed and approved before ✅ I simply refactored the unit test to use the table driven design + Go's testing package instead of Ginkgo/Gomega.

This PR needs another review from someone else to get merged.

Copy link

@christophrj christophrj left a comment

Choose a reason for hiding this comment

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

👍

@maximiliantech maximiliantech merged commit 88409f8 into main Oct 31, 2025
7 checks passed
@maximiliantech maximiliantech deleted the wait-with-deletion branch October 31, 2025 08:54
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.

cluster controller must wait for foreign finalizers to be removed

4 participants