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

Allow topology objects to be deleted if cluster deleted #52

Merged
merged 6 commits into from
Mar 15, 2021
Merged

Conversation

coro
Copy link
Contributor

@coro coro commented Mar 12, 2021

Prior to this PR, if a RabbitmqCluster was deleted prior to a topology object referencing that cluster, any attempts to delete the topology object would fail, due to the finalizer step failing. This was because at the start of every Reconcile, the controller attempts to create a rabbit-hole client to the cluster, which would fail.

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

The function which retrieves connection info about the RabbitmqCluster now returns a NoSuchRabbitmqClusterError, which can be type checked in the Reconcile function, provided the error is wrapped correctly. Any controller which attempts to reconcile a deletion of an object which gets this error can continue with removing the finalizer and ignoring the error creating a rabbit-hole client (the cluster was deleted, so no longer requires the object to be deleted as well).

Additional Context

This problem was really getting in the way of my development workflow - if a system test failed, I would have to manually edit the objects to remove their finalizers rather than just k deleteing them.

This prevents the finalizer step from failing if the cluster is already
down.
@coro coro changed the title WIP: Allow topology objects to be deleted if cluster deleted Allow topology objects to be deleted if cluster deleted Mar 12, 2021
@coro coro marked this pull request as ready for review March 12, 2021 13:26
system_tests/utils.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@coro coro left a comment

Choose a reason for hiding this comment

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

LGTM, @ChunyiLyu

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.

2 participants