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.

Sign up for GitHub

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

Jump to bottom

Unable to provide timeout for waiting #247

Closed
ctron opened this issue Jan 26, 2021 · 6 comments · Fixed by #643
Closed

Unable to provide timeout for waiting #247

ctron opened this issue Jan 26, 2021 · 6 comments · Fixed by #643
Assignees
@DDtKey
Labels
enhancement New feature or request
Milestone
0.17.0

Comments

@ctron
Copy link
Contributor

ctron commented Jan 26, 2021

When using "wait for" there seems to be no timeout.

While you can add a timeout to tests in general, it would make more sense for me to put a much shorter timeout for the startup of a container.
@thomaseizinger
Copy link
Collaborator

When using "wait for" there seems to be no timeout.

Correct.

Generally, we try to ship testcontainers in a way that works out-of-the-box, i.e. with pinned versions of containers and statically-encoded parameters and env variables. Green tests on our side should therefore ensure that this particular image will always work for the end user.

What is the usecase in which you would have needed a timeout?

While it would certainly be possible to pass or configure one, I'd be more interested in making it harder / impossible to use a docker image through testcontainers in a way that it wouldn't successfully start up.

@ctron
Copy link
Contributor Author

ctron commented Jan 27, 2021

What is the usecase in which you would have needed a timeout?

The issue I ran into is that it looks like Postgres containers fail to start up properly on GitHub actions. So the expected line doesn't show up in the logs, and the whole job is stuck.

While it would certainly be possible to pass or configure one, I'd be more interested in making it harder / impossible to use a docker image through testcontainers in a way that it wouldn't successfully start up.

I think there will always be a case you didn't think about. Allowing to put in a timeout make total sense to me.

@thomaseizinger
Copy link
Collaborator

That makes sense, thanks for sharing!

@thomaseizinger
Copy link
Collaborator

Do you think we should have a fixed timeout or something that is configurable per container?

I am leaning towards a global one that can maybe be overridden through an env variable to keep the images themselves simple.

@ctron
Copy link
Contributor Author

ctron commented Feb 2, 2021

I am leaning towards a global one that can maybe be overridden through an env variable to keep the images themselves simple.

Sounds reasonable to me. If additional requirements come up, it should be easy to extend. But it should also cover most cases already.

@thomaseizinger
Copy link
Collaborator

I propose a default timeout of 1 minute for now.

If we want to make this configurable, #259 should be a good foundation for that. I am thinking of something like:

TESTCONTAINERS=ready_timeout=2m,keep

@thomaseizinger thomaseizinger added the help wanted Extra attention is needed label Feb 3, 2021
@DDtKey DDtKey self-assigned this May 20, 2024
@DDtKey DDtKey added enhancement New feature or request and removed help wanted Extra attention is needed labels May 20, 2024
@DDtKey DDtKey added this to the 0.17.0 milestone May 20, 2024
DDtKey added a commit that referenced this issue May 26, 2024
@DDtKey
feat!: add container startup timeout with default of 1 minute
2a736e1 
Closes #247

- Sets default startup timeout to 1 minute
- Allows to override with `RunnableImage::with_startup_timeout`

Note: breaking change because of default timeout

For now, ENV variable to configure global timeout isn't introduced. First of all, that's quote similar to other languages, (e.g [Go](https://golang.testcontainers.org/features/wait/introduction/#startup-timeout-and-poll-interval), [Java](https://java.testcontainers.org/features/startup_and_waits/))

Another question is which one should take precedence: ENV over `with_startup_timeout` or vice versa?

We can extend it later in compatible way, so not that important at the moment.
@DDtKey DDtKey mentioned this issue May 26, 2024
Merged
DDtKey added a commit that referenced this issue May 26, 2024
@DDtKey
feat!: add container startup timeout with default of 1 minute (#643)
54e80e4 
Closes #247

- Sets default startup timeout to 1 minute
- Allows to override with `RunnableImage::with_startup_timeout`

Note: breaking change because of default timeout

For now, ENV variable to configure global timeout isn't introduced.
First of all, that's quote similar to other languages, (e.g
[Go](https://golang.testcontainers.org/features/wait/introduction/#startup-timeout-and-poll-interval),
[Java](https://java.testcontainers.org/features/startup_and_waits/))

Another question is which one should take precedence: ENV over
`with_startup_timeout` or vice versa?

We can extend it later in compatible way, so not that important at the
moment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DDtKey DDtKey

Labels
enhancement New feature or request
Projects
None yet
Milestone
0.17.0
Development

Successfully merging a pull request may close this issue.

feat!: add container startup timeout with default of 1 minute testcontainers/testcontainers-rs
3 participants
@ctron @thomaseizinger @DDtKey