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

Added builder for timeouts in JdbcDatabaseContainer (#715) #748

Merged
merged 2 commits into from
Jun 13, 2018

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?

@@ -190,13 +215,13 @@ public void addParameter(String paramName, String value) {
* @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