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

GH-8558 Make Ryuk shutdown hook configurable. #8732

Closed
wants to merge 2 commits into from

Conversation

mmaeller
Copy link

Description

#7717 added the Ryuk shutdown hook. Unfortunately, this leads to some issues e.g. with spring-boot like #8558.
With this PR I want to make the initial feature configurable.

@mmaeller mmaeller requested a review from a team as a code owner May 28, 2024 13:54
@mmaeller mmaeller changed the title Gh 8558 GH-8558 Make Ryuk shutdown hook configurable. May 28, 2024
@s-jepsen
Copy link

Any progress on this issue?

@ahmed-marzook
Copy link

Any update?

@s-jepsen
Copy link

s-jepsen commented Aug 9, 2024

Any progress on this issue?

@alex-arana
Copy link

alex-arana commented Aug 13, 2024

Why is this PR still in the freezer after nearly 3 months?

Has any feedback been provided as to why the proposed change has not yet been approved? Are there any concerns on the part of the reviewers that we could work through? The way I read it, this change is purely about adding a configurable option to testcontainers configuration such that we can bypass an early termination of the containers and Spring Application Context beans that use those services can coalesce gracefully. Why is this so controversial?

@jpalaz
Copy link

jpalaz commented Aug 13, 2024

@eddumelendez Could you please take a look please?

@mariuszptasinskii
Copy link

Any updates? Is there a way to move forwards with this change?

@kiview
Copy link
Member

kiview commented Oct 14, 2024

Please see my considerations in this comment in the original issue, which explains why we don't just merge this change, although it would allow for a workaround:
#8558 (comment)

@eddumelendez
Copy link
Member

We have decided to revert the change and it will be part of the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants