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

Improve fluent builders #657

Closed
wants to merge 1 commit into from

Conversation

arteam
Copy link

@arteam arteam commented Apr 23, 2018

Currently many containers (JDBC based, Nginx, Vault) are not very great as fluent builders. They are all parametrized, that means the user has to instance them with the sqare brackets to denote the type.
That's not intuitive, and the Testcontainers docs don't mention it anywhere. Moreover, the compiler still complains about raw types, because we can't declare a container variable parametrized by itself.

If we don't parametrize the container, than we can't invoke the methods of GenericContainer without losing the call chain. The compiler doesn't know about the concrete type of the builder. For example,
for MySQLContainer this causes a compiler error:

public MySQLContainer mySQLContainer = new MySQLContainer("mysql:5.7")
        .withClasspathResourceMapping("/mysql-initdb", "/docker-entrypoint-initdb.d", BindMode.READ_ONLY)
        .withDatabaseName("test-warehouse");

A solution is not to parametrize concrete containers and let the type propagate to GenericContainer.

@arteam arteam force-pushed the fix-fluent-builders branch 3 times, most recently from 2489760 to 06833ef Compare April 23, 2018 21:58
@bsideup
Copy link
Member

bsideup commented Apr 25, 2018

@arteam could you please check why the tests are failing on CircleCI?

@arteam
Copy link
Author

arteam commented Apr 25, 2018

java.net.UnknownHostException: foo.127.0.0.1.xip.io

and

java.sql.SQLException: Unable to load authentication plugin 'caching_sha2_password'.

Looks like some internal Circle CI failures, I will retry the build.

UPD: Looks like Circle CI is experiencing problems right now, I can't start a build.

@bsideup bsideup added this to the next milestone Apr 25, 2018
@bsideup
Copy link
Member

bsideup commented Apr 29, 2018

@arteam FYI we fixed the issues with JDBC containers (see #671 for the details)

@bsideup bsideup removed this from the next milestone Apr 30, 2018
@asafm
Copy link
Contributor

asafm commented Apr 30, 2018

I personally like removing the generic SELF - makes it much readable IMO - 👍

@bsideup
Copy link
Member

bsideup commented Apr 30, 2018

@asafm me too, just not always possible, especially with GenericContainer :)
FYI check #658, I was experimenting with some new ways of doing fluent-like DSLs without generics

@bsideup bsideup added this to the next milestone Apr 30, 2018
Currently many containers (JDBC based, Nginx, Vault) are not very great
as fluent builders. They are all parametrized, that means the user
has to instance them with the sqare brackets to denote the type.
That's not intuitive, and the Testcontainers docs don't mention it
anywhere. Moreover, the compiler still complains about raw types,
because we can't declare a container variable parametrized by itself.

If we don't parametrize the container, than we can't invoke the methods
of `GenericContainer` without losing the call chain. The compiler
doesn't know about the concrete type of the builder. For example,
for `MySQLContainer` this causes a compiler error:

```
MySQLContainer container = new MySQLContainer("mysql:5.7")
.withClasspathResourceMapping("/mysql-initdb", "/docker-entrypoint-initdb.d", BindMode.READ_ONLY)
.withDatabaseName("test-warehouse");
```

A solution is not to parametrize concrete containers and let the type
propagate to `GenericContainer`.
@arteam
Copy link
Author

arteam commented Apr 30, 2018

@bsideup Thanks, I've rebased my changes.

@bsideup
Copy link
Member

bsideup commented May 14, 2018

Hi @arteam,

I just realized that if we merge this one, it will break the binary compatibility.
And long term goal is to have something like #658.

I feel sorry for closing this PR, but encourage you to join the discussion about the new DSL to help us to shape it 👍

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