gnodet-bot commented on code in PR #13251:
URL: https://github.com/apache/maven/pull/13251#discussion_r4083790707
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/Check.java:
##########
@@ -42,6 +42,7 @@ protected boolean shouldSaveModifications() {
@Override
public int execute(UpgradeContext context) throws Exception {
+ context.setDryRun(true);
Review Comment:
⚠️ **Design concern: two parallel mechanisms for the same concept.**
`shouldSaveModifications()` already encodes whether this is a dry-run — it
returns `false` for `Check` and `true` for `Apply`. The PR adds a second,
independent boolean (`isDryRun`) that must always agree with it. There is no
enforcement: a future goal could override `shouldSaveModifications()` without
calling `setDryRun()`, and the log messages will lie again silently.
A simpler and safer approach: derive dry-run state from the existing
mechanism rather than maintaining a parallel flag. For instance,
`AbstractUpgradeGoal` could call
`context.setDryRun(!shouldSaveModifications())` in `execute()` before
delegating, so goals don't have to remember to set it themselves. Or replace
`shouldSaveModifications()` with `isDryRun()` entirely.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java:
##########
@@ -277,7 +277,11 @@ public UpgradeResult doApply(UpgradeContext context,
Map<Path, Document> pomMap)
if (hasUpgrades) {
modifiedPoms.add(pomPath);
- context.success("Plugin upgrades applied");
+ if (context.isDryRun()) {
+ context.action("Plugin upgrades would be applied");
+ } else {
+ context.success("Plugin upgrades applied");
Review Comment:
⚠️ **Coverage gap: `InferenceStrategy` still says "applied" during `mvnup
check`.**
`InferenceStrategy.doApply()` (not in this PR) has:
```java
context.success("Full inference optimizations applied");
context.success("Limited inference optimizations applied (parent-related
only)");
```
These fire during `mvnup check` (since `doApply` is called by both `Check`
and `Apply`), but they were not updated with the `isDryRun()` guard. The PR
fixes `PluginUpgradeStrategy` and `StrategyOrchestrator` but misses
`InferenceStrategy`. A user running `mvnup check --infer` still sees misleading
"applied" output.
`InferenceStrategy` needs the same treatment:
```java
if (hasInferences) {
modifiedPoms.add(pomPath);
if (MODEL_VERSION_4_1_0.equals(currentVersion) ||
ModelVersionUtils.isNewerThan410(currentVersion)) {
if (context.isDryRun()) {
context.action("Full inference optimizations would be applied");
} else {
context.success("Full inference optimizations applied");
}
} else {
if (context.isDryRun()) {
context.action("Limited inference optimizations would be applied
(parent-related only)");
} else {
context.success("Limited inference optimizations applied
(parent-related only)");
}
}
}
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/Apply.java:
##########
@@ -42,6 +42,7 @@ protected boolean shouldSaveModifications() {
@Override
public int execute(UpgradeContext context) throws Exception {
+ context.setDryRun(false);
Review Comment:
🔹 **Nit: `setDryRun(false)` is a no-op.**
`boolean dryRun` defaults to `false` in Java. This call is defensive but
signals the design is fragile (you felt the need to be explicit about the
default). It's harmless, but it underscores the concern above: if you need to
set both `shouldSaveModifications` and `isDryRun`, something is off.
--
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]
