testcontainers-java icon indicating copy to clipboard operation
testcontainers-java copied to clipboard

Add InfluxDB v2 support

Open raminqaf opened this issue 4 years ago • 29 comments

This PR fixes #3670

Every item on this list will require judgement by the Testcontainers core maintainers. Exceptions will sometimes be possible; items with should are more likely to be negotiable than those items with must.

Default docker image

  • [x] Should be a Docker Hub official image, or published by a reputable source (ideally the company or organisation that officially supports the technology)
  • [ ] Should have a verifiable open source Dockerfile and a way to view the history of changes
  • [x] MUST show general good practices regarding container image tagging - e.g. we do not use latest tags, and we do not use tags that may be mutated in the future
  • [x] MUST be legal for Testcontainers developers and Testcontainers users to pull and use. Mechanisms exist to allow EULA acceptance to be signalled, but images that can be used without a licence are greatly preferred.

Module dependencies

  • [x] The module should use as few dependencies as possible,
  • [x] Regarding libraries, either:
    • they should be compileOnly if they are likely to already be on the classpath for users' tests (e.g. client libraries or drivers)
    • they can be implementation (and thus transitive dependencies) if they are very unlikely to conflict with users' dependencies.
  • [x] If client libraries are used to test or use the module, these MUST be legal for Testcontainers developers and Testcontainers users to download and use.

API (e.g. MyModuleContainer class)

  • [x] Favour doing the right thing, and least surprising thing, by default
  • [x] Ensure that method and parameter names are easy to understand. Many users will ignore documentation, so IDE-based substitutes (autocompletion and Javadocs) should be intuitive.
  • [x] The module's public API should only handle standard JDK data types and MUST not expose data types that come from compileOnly dependencies. This is to reduce the risk of compatibility problems with future versions of third party libraries.

Documentation

  • [x] Every module MUST have a dedicated documentation page containing:
    • [x] A high level overview
    • [x] A usage example
    • [x] If appropriate, basic API documentation or further usage guidelines
    • [x] Dependency information
    • [x] Acknowledgements, if appropriate
  • [ ] Consider that many users will not read the documentation pages - even if the first person to add it to a project does, people reading/updating the code in the future may not. Try and avoid the need for critical knowledge that is only present in documentation.

raminqaf avatar Jan 07 '21 15:01 raminqaf

Thanks for the contribution @raminqaf

I think one thing we need to be really careful about is the breaking-change nature of this. Is there no way at all to make the InfluxDBContainer be compatible with both v1.x and v2.x of InfluxDB?

We need to avoid a situation where an upgrade of Testcontainers prevents people from being able to run their existing tests that rely on InfluxDB v1.x.

rnorth avatar Jan 10 '21 12:01 rnorth

@rnorth unfortunately not. Another way to add support for InfluxDB 2.x and keep the 1.x supported is to add a new and sperated module like influxDB2.

raminqaf avatar Jan 10 '21 12:01 raminqaf

What if we kept it in the same module, but have a InfluxDBContainerV2 class? We could also rename the current class to InfluxDBContainerV1 and have a @Deprecated class InfluxDBContainer extends InfluxDBContainerV1 to maintain binary compatibility.

rnorth avatar Jan 10 '21 12:01 rnorth

@rnorth Sounds good to me! I will add your suggestions and push them to the PR.

raminqaf avatar Jan 10 '21 12:01 raminqaf

@rnorth any updates on this PR?

raminqaf avatar Mar 11 '21 08:03 raminqaf

I had a quick read and everything is ok at the first glance :eyes:

vcvitaly avatar Mar 11 '21 12:03 vcvitaly

@vektory79 Thanks! Would be could if we could merge this. Since we are using the code in our project. I would like to have it from testcontainers dependency, rather than having it in our project directly :wink: @rnorth @kiview @bsideup

raminqaf avatar Mar 14 '21 17:03 raminqaf

hi, some updates should be done

influxdbV2ClientVersion = '1.14.0'

there is 2.0.0 out.

the example about v2 is a bit missleading i think cause the default for pure v2 s now token based . The user and password is only for the v1=>v2 upgraded instances.

gjemp avatar Mar 23 '21 10:03 gjemp

hi, some updates should be done

influxdbV2ClientVersion = '1.14.0'

there is 2.0.0 out.

the example about v2 is a bit missleading i think cause the default for pure v2 s now token based . The user and password is only for the v1=>v2 upgraded instances.

Thanks for the comment and your feedback on the PR. By the time I created this PR the version was 1.14.0 but I will update it to the newest one. The authentication and setup of the InfluxDB could be done either through username and password or through the token. As you can see on the InfluxDB Docker setup page. And according to the setup flags documentation the username and password are still valid arguments to do the authentication. Besides you can use these credentials to log in, during debug mode, to the dashboard and query the data.

raminqaf avatar Mar 28 '21 15:03 raminqaf

Thanks for the comment and your feedback on the PR. By the time I created this PR the version was 1.14.0 but I will update it to the newest one. The authentication and setup of the InfluxDB could be done either through username and password or through the token. As you can see on the InfluxDB Docker setup page. And according to the setup flags documentation the username and password are still valid arguments to do the authentication. Besides you can use these credentials to log in, during debug mode, to the dashboard and query the data.

Thanks for update. Any idea when we get the review done ? Would really like to have the update as release.

gjemp avatar Mar 29 '21 06:03 gjemp

Thanks for the comment and your feedback on the PR. By the time I created this PR the version was 1.14.0 but I will update it to the newest one. The authentication and setup of the InfluxDB could be done either through username and password or through the token. As you can see on the InfluxDB Docker setup page. And according to the setup flags documentation the username and password are still valid arguments to do the authentication. Besides you can use these credentials to log in, during debug mode, to the dashboard and query the data.

Thanks for update. Any idea when we get the review done ? Would really like to have the update as release.

Unfortunately not. In order to merge the PR, one of the three reviewers (@rnorth @bsideup @bsideup) should approved the PR first.

raminqaf avatar Mar 29 '21 06:03 raminqaf

@rnorth and @Andy2003 thanks for the code review! I updated the classes, the README and added your suggestions to the PR. I have simplified the InfluxDBContainerV2 class. You can find the latest container documentation here.

raminqaf avatar Aug 05 '21 09:08 raminqaf

@rnorth Done! Ready for the merge, hopefully! :smile:

raminqaf avatar Aug 05 '21 11:08 raminqaf

Heads up, running ./gradlew check locally I'm getting this Javadoc error:

/Users/rnorth/github.com/testcontainers/testcontainers-java/modules/influxdb/build/delombok/org/testcontainers/containers/InfluxDBContainerV2.java:60: error: element not closed: a
     * <a href="https://hub.docker.com/_/influxdb">
       ^
1 error
3 warnings

I think this will show up in the CI build any moment now...

rnorth avatar Aug 05 '21 15:08 rnorth

@bsideup and @rnorth piiiiiuuuhhhhh :sweat_smile: all done! Thanks for the great review! :heart:

raminqaf avatar Aug 05 '21 16:08 raminqaf

@rnorth and @bsideup the checks of ./gradlew :influxdb:japicmp are failing... here a screen shot of the report. Any suggestions? I tried to fix this by making the fields public again. Also the InfluxDBContainer creates a InfluxDBContainerV1 object... image

raminqaf avatar Aug 06 '21 14:08 raminqaf

@raminqaf we added japicmp to every module so that we don't need to manually verify the binary incompatibilities.

You will need to bring back the constants (make sure that they are in InfluxDBContainer, not its parent, as the constants are not inheritable in Java) and also you will need to override listed methods and make them return SELF as it was before

bsideup avatar Aug 06 '21 14:08 bsideup

@bsideup I tried to understand your suggestions... Please check if it is fine :smile:

raminqaf avatar Aug 06 '21 15:08 raminqaf

@raminqaf we shouldn't be changing the V1 class, but instead override its methods and return SELF from them (with something like return (SELF) this;)

bsideup avatar Aug 06 '21 15:08 bsideup

@rnorth and @bsideup Is the PR ready for the merge?

raminqaf avatar Aug 13 '21 07:08 raminqaf

@raminqaf thank for your patience on this one! we are still interested on getting this merge. However, a few things have changed since last time and we believe nowadays we can have one class container instead of two. Looks like the the only the difference are in the env variables. We can do now check which version image is used and set those variables according to the version. Check elasticsearch for inspiration. Please, let us know if you are still interested on work on this one and if you prefer to update the PR or I think would be much easier to create a new PR.

eddumelendez avatar Jul 20 '22 17:07 eddumelendez

@eddumelendez Hello! I am still interested in this PR and would love to work on it again! It's been a while since I pushed on this branch! 😅 I need to get into it and will look at the Elasticsearch code for that! Thanks!

raminqaf avatar Jul 21 '22 09:07 raminqaf

@eddumelendez I have added the changes you requested. Unfortunately, I cannot build the project locally on my laptop (Apple m1 Pro). I am getting this error every time I want to build the project:

Build file 'testcontainers/build.gradle' line: 65

Could not compile build file 'testcontainers/build.gradle'.
> startup failed:
  build file 'testcontainers/build.gradle': 65: unable to resolve class org.testcontainers.build.DelombokArgumentProvider 
   @ line 65, column 13.
                 new org.testcontainers.build.DelombokArgumentProvider(srcDirs: project.sourceSets.main.java.srcDirs, outputDir: file("$buildDir/delombok"))

I still like to update the InfluxDB Java client to the newest version for the tests. But first, I need to solve this build problem.

raminqaf avatar Jul 30 '22 09:07 raminqaf

@eddumelendez I added all the reviews you asked for! I hope the PR is ready!

raminqaf avatar Aug 05 '22 12:08 raminqaf

@eddumelendez I have added the changes you requested. Unfortunately, I cannot build the project locally on my laptop (Apple m1 Pro). I am getting this error every time I want to build the project:

Build file 'testcontainers/build.gradle' line: 65

Could not compile build file 'testcontainers/build.gradle'.
> startup failed:
  build file 'testcontainers/build.gradle': 65: unable to resolve class org.testcontainers.build.DelombokArgumentProvider 
   @ line 65, column 13.
                 new org.testcontainers.build.DelombokArgumentProvider(srcDirs: project.sourceSets.main.java.srcDirs, outputDir: file("$buildDir/delombok"))

I still like to update the InfluxDB Java client to the newest version for the tests. But first, I need to solve this build problem.

@eddumelendez Maybe this is interesting (and important) for you: I couldn't solve this problem on my MacBook! I switched to my old Linux laptop to be able to build and test the project!

raminqaf avatar Aug 05 '22 12:08 raminqaf

First of all, thank you for taking care of the comments! intel chip or m1? I currently use m1 and everything is fine. Used to use intel chip months ago and it worked fine too, as far as I remember but would give it a try again and see what's going on. Thanks for let us know

eddumelendez avatar Aug 05 '22 13:08 eddumelendez

First of all, thank you for taking care of the comments! intel chip or m1? I currently use m1 and everything is fine. Used to use intel chip months ago and it worked fine too, as far as I remember but would give it a try again and see what's going on. Thanks for let us know

I have a MacBook with the M1 chip.

raminqaf avatar Aug 06 '22 04:08 raminqaf

@eddumelendez Is it ready? 🚀

raminqaf avatar Aug 16 '22 07:08 raminqaf

@kiview Is this good to merge?

raminqaf avatar Sep 09 '22 12:09 raminqaf

@eddumelendez Thanks for the review! I added all of them!

raminqaf avatar Oct 15 '22 12:10 raminqaf