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

Run PreRemove hook before modifying the database #107

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

masnax
Copy link
Contributor

@masnax masnax commented Apr 16, 2024

The PreRemove hook was intended to be used before the node was removed from the database, however it seems it was incorrectly only being run before the node was removed from dqlite.

This poses some issues because an application using microcluster may want to clean up entities associated to records in the database for a particular node, but by removing the node from the database first, ON DELETE CASCADE will clear all of that state.

…orwarding node

The PreRemove hook might need to talk to the cluster so don't
pre-emptively remove trust from any node until after we run the
PreRemove hook

Signed-off-by: Max Asnaashari <[email protected]>
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@masnax
Copy link
Contributor Author

masnax commented Apr 18, 2024

I want to wait to merge this until after we have API updates in place, as this does affect the API.

@tomponline
Copy link
Member

I want to wait to merge this until after we have API updates in place, as this does affect the API.

Which PR is this dependent on?

@markylaing
Copy link
Contributor

I want to wait to merge this until after we have API updates in place, as this does affect the API.

Which PR is this dependent on?

I'm guessing #86, so an API extension can be added for it.

@masnax
Copy link
Contributor Author

masnax commented Apr 24, 2024

Yes that was the case with all of the outstanding PRs since many were modifications to the API.

But since @tomponline merged a bunch of them already I suppose it doesn't matter anymore. We can merge this as is.

@tomponline
Copy link
Member

But since @tomponline merged a bunch of them already I suppose it doesn't matter anymore. We can merge this as is.

Well lets wait a moment - if the previous PRs I merged that were approved also need API extensions then lets also add them too.

@tomponline
Copy link
Member

Its getting very hard to understand which PRs related to each other as so many are backed up.

@masnax masnax merged commit 3659ccf into canonical:main Apr 26, 2024
5 checks passed
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.

4 participants