Skip to content

Conversation

@StefanHufschmidt
Copy link
Contributor

Added builder functionality for timeouts in JdbcDatabaseContainer to improve usability for e.g. MS SQL Server image which needs much more time to start up.

I have seen the workaround with override from @vpavic at #715 and made it configurable by builder pattern to increase usability.

@kiview or @rnorth may take a look at it?

@kiview
Copy link
Member

kiview commented Jun 13, 2018

Seems like a pretty good idea and it's just a small change.

On the other hand, see this comment by @vpavic:

Just to note, providing custom timeout value(s) isn't a problem by itself, however when you do that in multiple different project it becomes a bit annoying, so my hope was this issue would result in a more sane default in Testcontainers.

So maybe MSSQLServerContainer should also have a higher timeout by default?

/**
* @return startup time to allow, including image pull time, in seconds
*/
protected int getStartupTimeoutSeconds() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OracleContainer is overriding this method, so this PR should also change OracleContainer to use fluent-setter to set it's default value.

@StefanHufschmidt
Copy link
Contributor Author

Initially I wanted to put setting the default timeouts into the configure-method but this would override values which where set by the user.
Now it happens inside the specific constructor so the user can still change it afterwards.
I've added a deprecation annotation to the related getter to make the builder methods more popular and to be able to remove those methods in future, we could use the fields directly then.

@bsideup bsideup added this to the next milestone Jun 13, 2018
@bsideup bsideup merged commit b4920c6 into testcontainers:master Jun 13, 2018
@StefanHufschmidt StefanHufschmidt deleted the jdbcTimeoutBuilder branch June 27, 2018 06:08
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.

3 participants