Add support for abortOnContainerExit to HTTP wait strategy
#693
Conversation
✅ Deploy Preview for testcontainers-node ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
packages/testcontainers/src/wait-strategies/http-wait-strategy.ts
Outdated
Show resolved
Hide resolved
packages/testcontainers/src/wait-strategies/http-wait-strategy.ts
Outdated
Show resolved
Hide resolved
packages/testcontainers/src/wait-strategies/http-wait-strategy.ts
Outdated
Show resolved
Hide resolved
packages/testcontainers/src/wait-strategies/http-wait-strategy.ts
Outdated
Show resolved
Hide resolved
|
@cristianrgreco Hello! Can you review my PR please? |
|
Hi @NuShoSinkuPomogliTebeTvoiSankcii, thanks for the contribution! I'll review this in the next few days, things are a bit busy around this time of the year 🙂 |
|
@cristianrgreco Hello! Kindly reminder about review :) |
|
Hey @NuShoSinkuPomogliTebeTvoiSankcii, sorry for the delay, I like the idea/suggested implementation. I left one comment above about tests |
packages/testcontainers/src/wait-strategies/http-wait-strategy.test.ts
Outdated
Show resolved
Hide resolved
packages/testcontainers/src/wait-strategies/http-wait-strategy.ts
Outdated
Show resolved
Hide resolved
packages/testcontainers/src/wait-strategies/http-wait-strategy.ts
Outdated
Show resolved
Hide resolved
packages/testcontainers/src/container-runtime/clients/container/docker-container-client.ts
Show resolved
Hide resolved
|
@cristianrgreco thanks for review! Refactored logic and added tests. |
|
@cristianrgreco Hello! Can you please review my changes? |
|
@NuShoSinkuPomogliTebeTvoiSankcii Apologies for the delay, I will get this reviewed by the weekend, thanks for making the changes! |
…rs-node into feature/http-wait-exit-status
packages/testcontainers/src/wait-strategies/http-wait-strategy.ts
Outdated
Show resolved
Hide resolved
packages/testcontainers/src/wait-strategies/http-wait-strategy.ts
Outdated
Show resolved
Hide resolved
packages/testcontainers/src/wait-strategies/http-wait-strategy.test.ts
Outdated
Show resolved
Hide resolved
|
Could you please add this new feature to the docs? https://github.com/testcontainers/testcontainers-node/blob/546d8730fe4fa8a44a35dee6f4c5f99642f3c817/docs/features/wait-strategies.md#http Overall LGTM! just a few minor nitpicks. Thanks again for the contribution. I'm aware this PR has been open a while (my bad) so LMK if you'd like me to make the changes 🙂 |
cristianrgreco
added
enhancement
|
@cristianrgreco Hello! Made refactoring, added docs. Just one question about export from index. |
packages/testcontainers/src/wait-strategies/http-wait-strategy.ts
Outdated
Show resolved
Hide resolved
packages/testcontainers/src/wait-strategies/http-wait-strategy.test.ts
Outdated
Show resolved
Hide resolved
packages/testcontainers/src/wait-strategies/http-wait-strategy.ts
Outdated
Show resolved
Hide resolved
|
It will be nice if you can release it after merge. |
cristianrgreco
changed the title
abortOnContainerExit to HTTP wait strategy
|
@NuShoSinkuPomogliTebeTvoiSankcii I will merge a couple more PRs and then release. Will be a couple hours at the most. Thanks again for your contribution! |
|
Released in 10.7.1 ❤️ |
Hello!
It's common case for me that http strategy will check whole time before startup timeout passed even if container already exited (stopped).
So to stop the healthcheck I added check for container status. If it exited, then I stop strategy check.
Also it convinient for this case to see last nth logs in console. I know that I can restart it with DEBUG mode, but in CI environment it can take many time to restart job with new envs.