maven.gitcommitid.skip does not override configuration in POM

[jgerken]

Executing Maven with the property -Dmaven.gitcommitid.skip=true does not skip the plugin execution when the configuration in the POM includes a <skip>false</skip>. Thus, the command line option does not override the setting in the POM.

Usually, all properties set via command line override the settings in the POM (this is even the purpose of the properties), e.g. skipping the tests or skipping linting. In my opinion, the behaviour of the git-commit-plugin should be changed to follow the general Maven approach and let command line property override the setting in the POM.

For now, an easy workaround exists, simply remove <skip>false</skip> from the POM.

[TheSnoozer]

We are using plain maven features and not doing any other magic (here is where the parameter comes from: https://github.com/ktoso/maven-git-commit-id-plugin/blob/master/src/main/java/pl/project13/maven/git/GitCommitIdMojo.java#L218)

I'll investigate but fear that this is a maven bug. What maven version are you using?

[TheSnoozer]

Congratulation you found what seems to be a Maven-Bug!
See Sample-Project:
SampleProject.zip

mvn clean install && mvn clean initialize -PdemoConfigSet -Dmaven.buildHelperMojo.skip=true
doesn't work since it has the configuration-tag set

mvn clean install && mvn clean initialize -PdemoConfigUnSet -Dmaven.buildHelperMojo.skip=true
works as expected since it has the configuration-tag NOT set

Reported here:
https://issues.apache.org/jira/browse/MNG-6278

[TheSnoozer]

Just saying:
The maven-surefire-plugin is using the same mechanism.
Not have tested this but a possible workaround would be using the same mechanism as maven-surefire-plugin.
They use as configuration inside the configuration tag the value <skipTests> and via command line maven.test.skip and if you check closely the source code it actually ends up in two distinct parameters:

• via configuration <skipTests> see https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java#L160
• via command line maven.test.skip see https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java#L178

In the end they check all possible ways to skip the plugin:
https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java#L360

Ugly but would work as a workaround....also from the looks their suggestion of Skipping by default on here seems to run into the same issue.

Edit:
Yes, the maven-surefire-plugin is affected by this as well:
SampleProject_v2.zip

mvn clean package -PdemoConfigUnSet -DskipTests=true
Tests are skipped as expected since configuration is NOT set

mvn clean package -PdemoConfigSet -DskipTests=true
Test will be executed and fail since the Test available has an Assert.fail

Please note that their suggestion Skipping by default on here is not affected since they also define an additional property and provide the property as argument for the parameter inside the configuation of the plugin. Interestingly this works since the property will be overwritten by the command-line and thus I would propose this as an additional workaround.

More recent documentation here under Skipping Tests would be impacted...

[TheSnoozer]

Feedback from the Maven-Issue:
This works as designed.
If you put a explicit value inside the configuration, then it is not possible to overwrite it with the commandline anymore.
If you want to be able to overwrite it and don't like the default value, you should add it as a property in your pom.xml.

<properties>
  <maven.buildHelperMojo.skip>true</maven.buildHelperMojo.skip>
</properties>

I'll deploy the same workaround used inside the maven-surefire-plugin with two distinct parameters. For the time being please use the workaround suggested.

[jgerken]

Thanks for investigating! The design decision by the Maven Team seems odd to me - but it's nothing we can change. It's sad that now every plugin developer has to implement the workaround to bring back intuitive behaviour.

[TheSnoozer]

@jgerken I would fully agree with your comment. As a plugin developer I also could say use whatever has been suggested by maven which would result that a user would need to define a property inside his own project and provide the value of the property to the configuration of the plugin. However I feel this is ugly and not what I want....forcing me as a plugin dev to define two properties inside the plugin is kinda ugly but thats where I will end up....

Regardless thanks for reporting this - i still consider this as a bug that will get fixed properly!
Until then please use the suggested property workaround.

[TheSnoozer]

Alright....I deployed the changes inside the plugin.
it is still insane that Maven forces developer to do such bad things...
Regardless, this will work properly inside the next version 2.2.4.

Until then please use the suggested workaround with defining a property inside your pom:

<properties>
  <maven.gitcommitid.skip>false</maven.gitcommitid.skip>
</properties>
<plugin>
  <groupId>pl.project13.maven</groupId>
  <artifactId>git-commit-id-plugin</artifactId>
  <version>${project.version}</version>
  <configuration>
    <skip>${maven.gitcommitid.skip}</skip>
  </configuration>
</plugin>