Skip to content

Don't install a shutdown hook by default #435

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

Closed
wants to merge 1 commit into from

Conversation

lburgazzoli
Copy link
Collaborator

Fixes #434

@lburgazzoli lburgazzoli force-pushed the gh-434 branch 2 times, most recently from 263b204 to d954c07 Compare June 13, 2021 08:25
@lburgazzoli lburgazzoli changed the title Don't install a shutdown hook by default #434 Don't install a shutdown hook by default Jun 13, 2021
@lburgazzoli
Copy link
Collaborator Author

@metacosm the failure here seems not related to he PR.

@metacosm
Copy link
Collaborator

I'd rather keep the current behavior as the default so that close is automatically called when the operator is shut down. However, we could indeed have an installShutdownHook method on Operator that would take a lambda as parameter and wrap it so that close would also be called after whatever user logic needs to happen first.

@lburgazzoli
Copy link
Collaborator Author

I wonder if that's correct, as example in quarkus, the k8s client is closed as part of the lifecycle so I'm not sure if explicitly add something to the shutdown hook, may cause to invoke an operation on an already closed/disposed resource

@metacosm
Copy link
Collaborator

On second thought, it makes sense to move ahead with this change, in particular so that frameworks can decide how to deal with stopping the operator in an idiomatic way. @lburgazzoli can you give me write access to your branch so that I can push updates before merging?

@lburgazzoli
Copy link
Collaborator Author

@metacosm sent an invitation

@metacosm
Copy link
Collaborator

Hmm, seems like I still can't push: ! [remote rejected] gh-434 -> gh-434 (refusing to allow an OAuth App to create or update workflow .github/workflows/master-snapshot-release.ymlwithoutworkflow scope)
I guess I need to do a proper fork as opposed to one from my IDE :)

@lburgazzoli
Copy link
Collaborator Author

lburgazzoli commented Aug 19, 2021

Hmm, seems like I still can't push: ! [remote rejected] gh-434 -> gh-434 (refusing to allow an OAuth App to create or update workflow .github/workflows/master-snapshot-release.ymlwithoutworkflow scope)
I guess I need to do a proper fork as opposed to one from my IDE :)

Since it is quite a small change, feel free to close this one and create a new PR, I really don't mind :)

@metacosm
Copy link
Collaborator

Replaced by #498

@metacosm metacosm closed this Aug 19, 2021
@metacosm metacosm deleted the gh-434 branch August 19, 2021 09: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.

Don't install a shutdown hook by default
2 participants