search

aerospike / aerospike-client-java

Aerospike Java Client Library
Other
236 stars 212 forks source link

Use parent bom for version, dependency and plugin management #112

Closed Aloren closed 6 years ago

Aloren commented 6 years ago

Extracted all common parts from modules pom to parent pom Added test module to the modules section of parent pom

Aloren commented 6 years ago

@BrianNichols I would also recommend using standard maven project layout for the project. What do you think?

Aloren commented 6 years ago

Added pom.xml for newly created module extensions. It contains reactor-client as a submodule now. Could you please explain what was the initial idea behind not including extensions folder as a submodule of aerospike-parent? This is rather uncommon for maven projects to have multiple parent poms in one repo.

BrianNichols commented 6 years ago

Your pull request has been accepted with modifications.

1) Removed extensions from top-level pom.xml. The build fails in reactor-client when java 1.7 is installed. If the minimum java version requirement increases to 1.8, we may consider putting extensions at the top level.

2) Put skipTests config back into reactor-client/pom.xml and added skipTests to test/pom.xml.

Maven made a mistake by combining compilation and testing into the same command (mvn package). Testing deployments can get very sophisticated with different clusters and configurations and this takes time to setup and run. I will usually perform a large number of compiles before I even think of running a formal test. It would have been much easier if there were separate maven commands to do this (mvn compile / mvn test).

Instead, I'm forced to use skipTests which then becomes awkward when I actually want to run a test (mvn test -DskipTests=false). I did a add a "run_tests" script which hides this undesirable syntax.

The standard maven project layout is the result of maven combining tests with the library it's testing. I prefer to keep tests in a separate project.

Aloren commented 6 years ago

Okay, thank you for the explanation. That totally makes sense.