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

Add container stopping and stopped hooks #1610

Merged
merged 2 commits into from
Jul 21, 2019

Conversation

jalaziz
Copy link
Contributor

@jalaziz jalaziz commented Jul 13, 2019

Add pre-stop and post-stop hooks to the GenericContainer. This provides
symmetry to the equavilent starting/started hooks and allows custom
behvaior to be defined in sub-classes.

Add pre-stop and post-stop hooks to the GenericContainer. This provides
symmetry to the equavilent starting/started hooks and allows custom
behvaior to be defined in sub-classes.
@bsideup
Copy link
Member

bsideup commented Jul 15, 2019

Hi @jalaziz,

Could you please briefly explain why you need them?

@jalaziz
Copy link
Contributor Author

jalaziz commented Jul 15, 2019

@bsideup I was extending one of the built-in Postgres container to provide a couple DB resources. I wanted a simple way to cleanup those resources before the container is terminated.

I was able to accomplish what I needed by overriding stop(). However, it seemed odd that there are containerIsStarted hooks with no equivalent for stopping.

Looking at the other containers, it seems that overriding start and stop is not the preferred method of adding additional post-start/pre-stop behavior. So I wanted to introduce a symmetry to the hooks so that a resource created post-start can be cleaned up before or after the container stops.

@bsideup
Copy link
Member

bsideup commented Jul 15, 2019

@jalaziz the question is more like.... why do you need to cleanup a container that is going to be killed and forcibly removed?

@jalaziz
Copy link
Contributor Author

jalaziz commented Jul 15, 2019

@bsideup Not cleaning up the container, cleaning up resources.

To be more specific, our container sub-class creates a scheduler and provides a Slick database instance based on the container. We want to close those resources when the container stops. We're not trying to actually manipulate the container, we just want to manage resources alongside the lifecycle of the container in the same rule that starts/stops the container.

@kiview
Copy link
Member

kiview commented Jul 16, 2019

So I wanted to introduce a symmetry to the hooks so that a resource created post-start can be cleaned up before or after the container stops.

I think this sounds reasonable.

@bsideup
Copy link
Member

bsideup commented Jul 16, 2019

@jalaziz I see. Ok, let's add it 👍
Just one thing: could you please add a javadoc comment to the hooks that they will not be triggered if the container is being terminated during the shutdown hook or even later by Ryuk?

@bsideup bsideup added this to the next milestone Jul 16, 2019
@jalaziz
Copy link
Contributor Author

jalaziz commented Jul 19, 2019

@jalaziz I see. Ok, let's add it 👍
Just one thing: could you please add a javadoc comment to the hooks that they will not be triggered if the container is being terminated during the shutdown hook or even later by Ryuk?

Will do. Been a bit busy but will get to it as soon as I can.

@bsideup
Copy link
Member

bsideup commented Jul 19, 2019

@jalaziz that would be great, thanks!

@bsideup bsideup merged commit 67501e8 into testcontainers:master Jul 21, 2019
@bsideup
Copy link
Member

bsideup commented Jul 21, 2019

@jalaziz I added the warnings myself, will be released very soon, thanks for your contribution 👍

@jalaziz
Copy link
Contributor Author

jalaziz commented Jul 21, 2019

Thanks @bsideup!

@jalaziz jalaziz deleted the stop-hooks branch July 21, 2019 20:34
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.

None yet

3 participants