ssheikin commented on code in PR #1394:
URL: https://github.com/apache/maven-release/pull/1394#discussion_r3420646244
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java:
##########
@@ -118,6 +118,11 @@ private void prepare(ReleasePrepareRequest prepareRequest,
ReleaseResult result)
// Create a config containing values from the session properties (ie
command line properties with cli).
ReleaseUtils.copyPropertiesToReleaseDescriptor(
prepareRequest.getUserProperties(), new
ReleaseDescriptorBuilder() {
+ public ReleaseDescriptorBuilder setPushChanges(boolean
pushChanges) {
+ builder.setPushChanges(pushChanges);
+ return this;
+ }
+
Review Comment:
It's not enough.
PTAL on repro:
https://github.com/ssheikin/maven-release-1394-repro#running-it
Also, let me share AI reasoning about it:
```
Here's the actual Maven mechanism, in three steps.
1. The annotation becomes a default expression in plugin.xml.
@Parameter(defaultValue = "true", property = "pushChanges")
(AbstractScmReadWriteReleaseMojo.java:79) is compiled by maven-plugin-tools
into the plugin descriptor as a default configuration entry, roughly:
<configuration>
<pushChanges default-value="true">${pushChanges}</pushChanges>
</configuration>
So out of the box, the parameter's configured value is the expression
${pushChanges}. property is just "the name of the expression that goes in this
slot."
2. The POM <configuration> is merged on top of that descriptor default (a
DOM merge).
When your POM declares:
<configuration><pushChanges>false</pushChanges></configuration>
Maven merges it over the descriptor config with Xpp3Dom merge rules. Same
element name → the POM's child replaces the descriptor's. The effective config
for the mojo execution is now the literal:
<pushChanges>false</pushChanges> <!-- the ${pushChanges} expression is
gone -->
3. Configuration injection evaluates expressions — but there's no longer
one.
The component configurator walks the merged config and, for each ${...},
calls PluginParameterExpressionEvaluator, which is what resolves ${pushChanges}
against session.getUserProperties() (your -D values).
But after step 2 the value is the plain string "false" — no ${...},
nothing to evaluate, -D is never read. The field is set to false.
So the rule is: -Dxxx only feeds a parameter whose configured value is
still the ${xxx} expression — i.e. one you did not set explicitly in
<configuration>. property supplies a default, it is not an override
channel.
Why it feels surprising: -DskipTests "always works" only because projects
rarely hardcode <skipTests> in surefire's <configuration> — the ${skipTests}
expression stays live. Hardcode
<skipTests>false</skipTests> and -DskipTests=true is ignored the exact
same way.
Why that forces the fix where it is: once the literal wins, the mojo field
is false, so descriptor.setPushChanges(pushChanges) can only ever propagate
false. The raw -DpushChanges survives only as
session.getUserProperties(), which reaches
DefaultReleaseManager.prepare(...) as prepareRequest.getUserProperties() —
hence the override must be applied there, not at the mojo.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
