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

MockServerContainer: remove dependency to java api #833

Merged
merged 2 commits into from
Sep 14, 2018

Conversation

lanwen
Copy link
Contributor

@lanwen lanwen commented Aug 14, 2018

originally that was not good idea to depend on something which
can brake binary compatibility :)

this change will allow to start any mockserver container version,
doesn't matter which version of the client in the classpath

originally that was not good idea to depend on something which
can brake binary compatibility :)

this change will allow to start any mockserver container version,
doesn't matter which version of the client in the classpath
@bsideup bsideup added this to the next milestone Aug 14, 2018
@lanwen lanwen changed the title remove dependency to java api remove dependency to java api in mockserver Aug 15, 2018
Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

Actually... I'm not sure that we should make this change. Here's my thinking:

  1. There is a benefit in having a binary dependency of a fixed version, because the docker image is (by default) on that same version. This makes it easier to be confident about compatibility. If a different version is needed, you can specify this in the constructor and (mostly) just as easily override the dependency version.

  2. The class adds value because it provides easy access to a wired-up MockServerClient instance. Without that getter, its reason for existence is less (you could just provide a snippet showing how to use GenericContainer to do the same thing). This module 'earns its place' by doing (just) a bit more than such a code snippet.

If we wanted to not have a binary dependency, but still benefit from (1), then it would be awesome to do automatic JAR/docker-image dependency matching like we do for Selenium. However, I assume there isn't as much need for strict compatibility, so this is probably only a nice-to-have.

I won't close right away, but I think that's the way I'm leaning. Does my logic make sense to you?

@rnorth
Copy link
Member

rnorth commented Aug 26, 2018

However, I assume there isn't as much need for strict compatibility

... and now I find out from @bsideup that there is a breaking change in the mockserver API which messes this up 😆

Would it be practical to detect the JAR version (e.g. via MANIFEST.MF) and automatically select the right docker image version?

@lanwen
Copy link
Contributor Author

lanwen commented Aug 26, 2018

Actually the REST api is pretty stable, what breaks is calling java api when initializing client. If you pick other than fixed version with broken binary compatibility, you just receive NoClassDefFound on container start.

Why its useful still to have this snippet without client - is that client initialization takes 3 lines in your code, when whole class is 10 times bigger.

What we can try is checking that known java version is in classpath and create a client, if no - just be noop and return null for client.

@@ -20,7 +21,7 @@

@Test
public void testBasicScenario() throws Exception {
mockServer.getClient()
new MockServerClient(mockServer.getContainerIpAddress(), mockServer.getMappedPort(MockServerContainer.PORT))
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to add mockServer.getServerPort() to hide the constant

@bsideup bsideup changed the title remove dependency to java api in mockserver MockServerContainer: remove dependency to java api Sep 1, 2018
@bsideup
Copy link
Member

bsideup commented Sep 3, 2018

@rnorth so, are you OK with this change?

@bsideup bsideup modified the milestones: 1.9.0-rc1, next Sep 12, 2018
@kiview
Copy link
Member

kiview commented Sep 13, 2018

Removing the client looks good to me, isn't it the same we are doing in case of Selenium?

@bsideup
Copy link
Member

bsideup commented Sep 13, 2018

@kiview Selenium has a stable public API, this is why we never had such issue with it, but MockServer project is not strict enough about the public API and broke it in one of their latest releases

@rnorth
Copy link
Member

rnorth commented Sep 13, 2018

Yep go for it.

Some day maybe I’d like to extract the Selenium module’s automatic JAR-image pairing. But that can wait.

@bsideup bsideup merged commit 9ed75d7 into testcontainers:master Sep 14, 2018
@lanwen lanwen deleted the rm_mockserver_client branch September 14, 2018 08:40
@rnorth
Copy link
Member

rnorth commented Sep 21, 2018

Released for preview in 1.9.0-rc2, to be published on Bintray.

Thanks @lanwen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/breaking-api-change Public APIs are being changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants