Skip to content

Allow override pushChanges from terminal #1394

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.

Sign up for GitHub

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

Open
ssheikin wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow override pushChanges from terminal #1394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02e6d10
Allow override pushChanges from terminal
ssheikin Oct 7, 2025
2661d63
Add tests for -DpushChanges
ssheikin Jun 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

@ssheikin ssheikin Oct 7, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

should other properties/methods from ReleaseDescriptorBuilder be overriden here too?

@kwin kwin Nov 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't

maven-release/maven-release-plugin/src/main/java/org/apache/maven/plugins/release/AbstractScmReadWriteReleaseMojo.java

Line 104 in f06aaa2

descriptor.setPushChanges(pushChanges);
enough in the context of the prepare goal?

@ssheikin ssheikin Jun 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

public ReleaseDescriptorBuilder addDevelopmentVersion(String key, String value) {
builder.addDevelopmentVersion(key, value);
return this;
Expand Down
65 changes: 65 additions & 0 deletions ...ease-manager/src/test/java/org/apache/maven/shared/release/DefaultReleaseManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,71 @@ void testPrepareSimulateNoCompletedPhase() throws Exception {
assertFalse(((ReleasePhaseStub) phaseStep3).isExecuted(), "step3 not executed");
}

@Test
void testPrepareOverridesConfiguredPushChangesFromUserProperties() throws Exception {
// pushChanges=false as if configured in the POM <configuration> (where it wins over -DpushChanges)
ReleaseDescriptorBuilder builder = configStore.getReleaseConfiguration();
builder.setCompletedPhase(null);
builder.setPushChanges(false);

// pushChanges=true as if passed on the command line with -DpushChanges
Properties userProperties = new Properties();
userProperties.setProperty("pushChanges", "true");

ReleasePrepareRequest prepareRequest = new ReleasePrepareRequest();
prepareRequest.setReleaseDescriptorBuilder(builder);
prepareRequest.setReleaseEnvironment(new DefaultReleaseEnvironment());
prepareRequest.setResume(false);
prepareRequest.setUserProperties(userProperties);

releaseManagerTest.prepare(prepareRequest);

assertTrue(
((ReleasePhaseStub) phaseStep1).getReleaseDescriptor().isPushChanges(),
"command-line pushChanges=true should override POM-configured pushChanges=false");
}

@Test
void testPrepareUserPropertyCanDisablePushChanges() throws Exception {
ReleaseDescriptorBuilder builder = configStore.getReleaseConfiguration();
builder.setCompletedPhase(null);
builder.setPushChanges(true);

Properties userProperties = new Properties();
userProperties.setProperty("pushChanges", "false");

ReleasePrepareRequest prepareRequest = new ReleasePrepareRequest();
prepareRequest.setReleaseDescriptorBuilder(builder);
prepareRequest.setReleaseEnvironment(new DefaultReleaseEnvironment());
prepareRequest.setResume(false);
prepareRequest.setUserProperties(userProperties);

releaseManagerTest.prepare(prepareRequest);

assertFalse(
((ReleasePhaseStub) phaseStep1).getReleaseDescriptor().isPushChanges(),
"command-line pushChanges=false should override POM-configured pushChanges=true");
}

@Test
void testPrepareKeepsConfiguredPushChangesWhenUserPropertyAbsent() throws Exception {
ReleaseDescriptorBuilder builder = configStore.getReleaseConfiguration();
builder.setCompletedPhase(null);
builder.setPushChanges(false);

ReleasePrepareRequest prepareRequest = new ReleasePrepareRequest();
prepareRequest.setReleaseDescriptorBuilder(builder);
prepareRequest.setReleaseEnvironment(new DefaultReleaseEnvironment());
prepareRequest.setResume(false);
prepareRequest.setUserProperties(new Properties());

releaseManagerTest.prepare(prepareRequest);

assertFalse(
((ReleasePhaseStub) phaseStep1).getReleaseDescriptor().isPushChanges(),
"configured pushChanges should be preserved when no command-line override is given");
}

@Test
void testPrepareSimulateCompletedPhase() throws Exception {
ReleaseDescriptorBuilder builder = configStore.getReleaseConfiguration();
Expand Down
11 changes: 11 additions & 0 deletions ...release-manager/src/test/java/org/apache/maven/shared/release/phase/ReleasePhaseStub.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ public class ReleasePhaseStub implements ReleasePhase, ResourceGenerator {
*/
private boolean cleaned;

/**
* The descriptor the phase was last invoked with.
*/
private ReleaseDescriptor releaseDescriptor;

@Override
public ReleaseResult execute(
ReleaseDescriptor releaseDescriptor,
Expand All @@ -54,6 +59,7 @@ public ReleaseResult execute(
ReleaseResult result = new ReleaseResult();

executed = true;
this.releaseDescriptor = releaseDescriptor;

result.setResultCode(ReleaseResult.SUCCESS);

Expand All @@ -68,6 +74,7 @@ public ReleaseResult simulate(
ReleaseResult result = new ReleaseResult();

simulated = true;
this.releaseDescriptor = releaseDescriptor;

result.setResultCode(ReleaseResult.SUCCESS);

Expand Down Expand Up @@ -96,4 +103,8 @@ public boolean isSimulated() {
public boolean isCleaned() {
return cleaned;
}

public ReleaseDescriptor getReleaseDescriptor() {
return releaseDescriptor;
}
}