ZOOKEEPER-3797 Conflict between fatjar and full-build Maven profiles

[eolivelli]

Enable fatjar module in full-build profile.
This is a fix only for branch-3.6, on master branch maven structure is changing, there is no need for this fix

[eolivelli]

@symat @nkalmar @ctubbsii PTAL
I really want to unblock the 3.6.1 release as soon as possible
thanks in advance

This is the minimal fix I can image in order to make the 3.6.1 release.
We are changing Maven module structure in 3.7.0 so no need to complicate things in 3.6

[tisonkun]

Changes looks good. The question from my side it about version number. Why this branch has a version number 3.6.2-SNAPSHOT and targets for 3.6.1 release? Could you point it out to me the release progress?

[tisonkun]

What the reason of this change? It seems a change we should cheery pick for master.

[eolivelli]

Because fatjar needs it.
master branch is differentf from branch-3.6

[eolivelli]

we have now branch release-3.6.1 for 3.6.1 release.
This is our way.
We cut a release branch when we start the release process.
Then the release manager handles the release branch, but the main branch for 3.6 release version is open for new commits and it has 3.6.2-SNAPSHOT version

[eolivelli]

thank you @tisonkun

[eolivelli]

retest this please

[eolivelli]

it looks like maven build does not start on CI
but the ANT build started but there is not ANT build.xml in branch-3.6

[ctubbsii]

+1 to the changes so far, but you'll need to also include -Pfull-build in the <preparationGoals> and <completionGoals> for the maven-release-plugin configuration if you want to activate all modules for pom version updates during release:prepare. (See my similar changes on #1314 )

Note: if this PR gets applied before #1314, I'll update #1314 to align with whatever was decided in this PR.

[eolivelli]

We are using preparationGoals only in order to fix c client files.

[ctubbsii]

We are using preparationGoals only in order to fix c client files.

I'm not sure I understand this comment. maven-release-plugin updates the POM versions during release:prepare. Will release:prepare update the versions of all modules without specifying -Pfull-build in <preparationGoals>?

[eolivelli]

AFAIK preparationGoals and completion goals are additional goals to execute in different steps of the procedure.

The current procedure works well and modifies the files on the c client that are not handled by Maven.

I think there is no need to change.
I will add a comment on your patch on the master branch

[ctubbsii]

Okay, the part where I was confused is how to activate all the modules during version-bumping when release:prepare runs. Apparently the way to activate this would be mvn -Pfull-build release:prepare, rather than add it to <preparationGoals>.

I've never run a Maven project where all modules weren't activated by default, so I was a little confused on how to deal with that for releases.

You will still need to fix the project parent version in zookeeper-contrib/zookeeper-contrib-fatjar/pom.xml and zookeeper-it/pom.xml so it matches the rest of the project.

[eolivelli]

@ctubbsii I have another problem.
The zookeeper-it module needs fatjar in order to run but fatjar needs zookeeper-it to be built.
This is very annoying, it is a circular dependency
I am going to add skipTests in zookeeper-it in case of "full-build"

[eolivelli]

With this change I am able to run the full build with -Dfull-build, skipping tests of contrib and zookeeper-it

@anmolnar @nkalmar @symat or any other commmiters PTAL

I feel the review of @ctubbsii and @tisonkun are very useful and maybe they are enough to commit this patch

[ctubbsii]

This can be shorter.

Suggested change

	<build>
	<plugins>
	<plugin>
	<artifactId>maven-surefire-plugin</artifactId>
	<configuration>
	<skipTests>true</skipTests>
	</configuration>
	</plugin>
	</plugins>
	</build>
	<properties>
	<skipTests>true</skipTests>
	</properties>

[eolivelli]

done, thank you @ctubbsii

[eolivelli]

Spotbugs failed

[ctubbsii]

Lesson learned from #1314 : you might need to skip checkstyle, too.

Suggested change

	<skipTests>true</skipTests>
	<spotbugs.skip>true</spotbugs.skip>
	<checkstyle.skip>true</checkstyle.skip>
	<skipTests>true</skipTests>
	<spotbugs.skip>true</spotbugs.skip>

[eolivelli]

the precommit job will run someday :-)
there is a huge queue
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/

[eolivelli]

Build completed with success.

[breed]

+1 nice work everyone!

[breed]

per the bylaws, http://zookeeper.apache.org/bylaws.html, it looks like we can't get this in until Monday evening since we have to allow 1 business day for voting :'( this is a pretty simple patch, do we have precedent for pushing release changes faster? @fpj @phunt

[phunt]

I believe the intent is to ensure there is sufficient time for more than just committer/reviewer to take a look before something is committed. This is a bug fix that a number of folks have already commented on, that I don't think would be controversial? I think there has been precedent in the past for such things.

[eolivelli]

@breed I would add that this patch does not touch zk internals, no risks. It is only build boilerplate.

Thank you @breed @phunt

[phunt]

+1, basic testing worked for me (reg/full builds), merged onto branch-3.6 and release-3.6.1 branches.

Thanks all!

[breed]

thank you @phunt !

[breed]

and thank you @eolivelli, @tisonkun, and @ctubbsii for resolving this so quickly!