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

Gradle #574

Merged
merged 35 commits into from
Feb 11, 2018
Merged

Gradle #574

merged 35 commits into from
Feb 11, 2018

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Feb 4, 2018

in #564 we agreed to use Gradle instead of Maven, and also use Bintray to simplify the process of release.

This PR replaces Maven with Gradle and also brings some exciting changes:

  • BOM publishing ( org.testcontainers:testcontainers-bom:${version} ) with every module from this repository
  • Run Delombok before attaching the sources
  • Remote Build Cache to speed up the PRs
  • Revisited shadowing

The result was pushed to Bintray:
Repo: https://dl.bintray.com/testcontainers/releases
BOM: org.testcontainers:testcontainers-bom:1.6.0-gradle

So that you can test it yourself on your projects and verify that it works as previously.

@bsideup bsideup added this to the next milestone Feb 4, 2018
@bsideup bsideup requested review from rnorth and kiview February 4, 2018 14:48
# Conflicts:
#	modules/jdbc/pom.xml
#	pom.xml
Copy link
Contributor

@asafm asafm left a comment

Choose a reason for hiding this comment

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

Quite a sizable PR!
Just throwing in my 2 cent as I’m mostly a user and not a contributor: I think Gradle build files are on the surface area much shorter thus “simpler” but they do much implicit complexity inside as opposed to Verbose Maven which is more explicit.

I read the move Gradle was due to speed of build mainly due to using S3 caching. Wondering how much did it save and wasn’t it achievable in Maven?

@bsideup
Copy link
Member Author

bsideup commented Feb 5, 2018

@asarm

While Gradle's Build Cache indeed saves a lot of time (i.e. if you change the markdown docs, tests will not be executed, or if you change only kafka module, core tests and other modules will not be executed also), it was not the only reason to migrate to Gradle.

With Maven, things like BOM, Delombok or explicit control of shadowed dependencies was hard to do, so we didn't.
Also, if we want to merge all modules into the same repo, we would like to make adding new modules simple. With Maven, you have to list your modules explicitly in your root project's POM. Our Gradle config will auto-detect modules so that we will not get merge conflicts if two PRs with modules were submitted in parallel.

And last but not least, you can & should configure "delegate to Gradle" in your IDE so that you will always get the same compilation result as any other contributor. e.g. technically you don't have to install Lombok plugin to make project compile. It also solves the issues like running the shadowing tests (before, you had to package JAR manually with Maven before you run the JAR tests)

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

The changes seem fine for me, just some questions/remarks and everything works perfectly out of the box. Of course we are still missing documentation updates I assume?

Edit:
It seems we are not 😉

subprojects {
apply plugin: 'nebula.provided-base'
apply plugin: 'java'
apply plugin: 'idea'
Copy link
Member

Choose a reason for hiding this comment

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

This plugin can normally be omitted.

Copy link
Member Author

Choose a reason for hiding this comment

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

No it's not :) We have this line in core/build.gradle:

idea.module.testSourceDirs += sourceSets.jarFileTest.allSource.srcDirs

Copy link
Member

Choose a reason for hiding this comment

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

What do we need this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

can't you just.... errr... read it? :D

Copy link
Member Author

Choose a reason for hiding this comment

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

"add all sources of "jarFileTest" source set to module's test source dirs" :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes I can read it, I was not aware that you have to do this manually, since i.e. gradle-test-sets-plugin was doing this automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I know the plugin, but thought to use as minimum plugins as possible to keep build simple :)

Copy link
Member

Choose a reason for hiding this comment

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

Totally fine this way, I was just explaining my lack of knowledge regarding this configuration 🤤

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, ok, sorry :)

description = "Testcontainers :: Database-Commons"

dependencies {
compile project(':testcontainers')
Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe add this dependency for all subprojects but :testcontainers? I think simple would not need any build.gradle anymore in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of implicit dependencies, to be honest :)
Plus IMO description have to be set by a human.

5 lines file per module shouldn't hurt too much :)

This is introduced to allow the build remote cache on Windows.
docs/index.md Outdated
@@ -47,7 +47,7 @@ This project was initially inspired by a [gist](https://gist.github.com/mosheesh
* discuss with the authors on an issue ticket prior to doing anything big
* follow the style, naming and structure conventions of the rest of the project
* make commits atomic and easy to merge
* verify all tests are passing. Build the project with `mvn clean install -Pproprietary-deps` to do this.
* verify all tests are passing. Build the project with `./gradlew test` to do this.
Copy link
Member

Choose a reason for hiding this comment

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

Please could we mention the build cache here?

People might want to re-run all tests just to be sure, especially, for example, if they're trying to check both Docker Machine and Docker for * compatibility. It would probably be helpful to explain how to disable the cache. (Is ./gradlew test --no-build-cache correct?)

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea, I'll add it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR the recommended test task should be check. check dependsOn test by default. I would document check here, so that future test tasks (e.g. TestNG oder integrative tests), can be introduced without telling people about another test task.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are right. I think we should change the usage in Travis and CircleCI accordingly also.

@rnorth
Copy link
Member

rnorth commented Feb 9, 2018

LGTM 😄
The build time improvements on their own are compelling, but also the general cleanliness and readability make me happy.

rnorth and others added 6 commits February 11, 2018 13:57
* Add ability to specify docker image in KafkaContainer.

* Added 'kafka.container.image' property to TestcontainersConfiguration in order to have an opportunity to replace default image with testcontainers.properties.

* Return version argument to KafkaContainer constructor.
# Conflicts:
#	core/pom.xml
#	modules/database-commons/pom.xml
#	modules/jdbc-test/pom.xml
#	modules/jdbc/pom.xml
#	modules/kafka/pom.xml
#	modules/mysql/pom.xml
#	modules/nginx/pom.xml
#	modules/postgresql/pom.xml
#	modules/selenium/pom.xml
#	modules/virtuoso/pom.xml
#	pom.xml
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.

6 participants