Copilot commented on code in PR #12351:
URL: https://github.com/apache/maven/pull/12351#discussion_r3470442929
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java:
##########
@@ -728,7 +733,8 @@ private boolean addPluginManagementForEffectivePlugins(
if (upgrade != null) {
// Check if plugin is already managed
if (!isPluginAlreadyManagedInElement(managedPluginsElement,
upgrade)) {
Review Comment:
This change still injects pluginManagement (and possibly build/plugins
overrides) for plugins that appear only via the effective model from a remote
parent. That’s the behavior reported in #11606; adding a comment doesn’t
prevent the spurious injection. To match the PR description/title, the
effective-model plugin list should be filtered to only plugins declared in
local POMs (collected via collectLocallyDeclaredPluginKeys).
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java:
##########
@@ -795,8 +808,12 @@ private boolean addDirectPluginOverrides(UpgradeContext
context, Document pomDoc
PluginUpgrade upgrade = pluginUpgrades.get(pluginKey);
if (upgrade != null) {
if (!isPluginAlreadyManagedInElement(pluginsElement, upgrade))
{
- DomUtils.createPlugin(
+ Element plugin = DomUtils.createPlugin(
pluginsElement, upgrade.groupId(),
upgrade.artifactId(), upgrade.minVersion());
Review Comment:
Direct build/plugins overrides should also be skipped for plugins not
declared in local POMs; otherwise mvnup can still inject overrides for
remote-parent-only plugins and change builds the project doesn’t control.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java:
##########
@@ -132,6 +133,9 @@ public UpgradeResult doApply(UpgradeContext context,
Map<Path, Document> pomMap)
// Phase 2: For each POM, build effective model using the session
and analyze plugins
PluginAnalysisResults analysisResults =
analyzePluginsUsingEffectiveModels(context, pomMap, tempDir);
+ // Collect locally declared plugin keys so we can add comments for
remote-parent overrides
+ Set<String> localPluginKeys =
collectLocallyDeclaredPluginKeys(pomMap);
Review Comment:
The comment says localPluginKeys are collected to add comments for
remote-parent overrides, but the PR description says remote-parent-only plugins
should be skipped to avoid spurious injection. Update this comment to reflect
the intended behavior (filtering), otherwise it’s misleading for maintainers.
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java:
##########
@@ -825,12 +832,12 @@ void shouldDetectInheritedPluginsFromRemoteParent()
throws Exception {
}
@Test
- @DisplayName("should not add direct build/plugins override when plugin
version comes from pluginManagement")
- void shouldNotAddDirectOverrideWhenVersionFromPluginManagement()
throws Exception {
+ @DisplayName("should override remote parent plugin via
pluginManagement with comment, not direct build/plugins")
+ void shouldOverrideRemoteParentPluginViaPluginManagementWithComment()
throws Exception {
Review Comment:
This test currently expects a pluginManagement override + comment for a
plugin coming only from a remote parent. If remote-parent-only plugins are
meant to be skipped, rename the test to reflect that expectation (no injection).
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java:
##########
@@ -868,6 +875,11 @@ void
shouldNotAddDirectOverrideWhenVersionFromPluginManagement() throws Exceptio
.orElse("")));
assertTrue(hasEnforcerInPM, "Should have enforcer in
pluginManagement");
+ String xml = DomUtils.toXml(document);
+ assertTrue(
+ xml.contains("Override version inherited from parent"),
+ "Should add comment explaining the override");
Review Comment:
If remote-parent-only plugins are meant to be skipped (per PR
description/title), this assertion (and earlier assertions in the same test
that require pluginManagement to exist) should be inverted. As written, this
test enforces the spurious injection behavior (#11606) by expecting an injected
override + comment for a plugin not declared in any local POM.
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java:
##########
@@ -806,12 +807,18 @@ void shouldDetectInheritedPluginsFromRemoteParent()
throws Exception {
UpgradeResult result = strategy.doApply(context, pomMap);
assertTrue(result.success(), "Strategy should succeed");
- assertTrue(result.modifiedCount() > 0, "Should have added
plugin management for inherited plugins");
+ assertTrue(result.modifiedCount() > 0, "Should have modified
POM for remote parent plugin upgrade");
String xml = DomUtils.toXml(document);
+ assertTrue(
Review Comment:
With the intended fix (skip effective-model plugins not declared in local
POMs), a project that only inherits a plugin from a remote parent should not be
modified. These assertions currently expect injection and should be inverted.
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java:
##########
@@ -773,11 +773,12 @@ void shouldHaveValidPluginUpgradeDefinitions() throws
Exception {
class InheritedPluginDetectionTests {
@Test
- @DisplayName("should detect inherited plugins from remote parent POM
and add pluginManagement")
- void shouldDetectInheritedPluginsFromRemoteParent() throws Exception {
- // org.apache:apache:23 defines maven-enforcer-plugin:1.4.1 in
pluginManagement.
- // A child POM that inherits from this parent should get
pluginManagement overrides
- // added by mvnup for plugins that need Maven 4 compatibility
upgrades.
+ @DisplayName("should inject pluginManagement with comment for plugins
inherited from remote parent")
+ void shouldInjectPluginManagementWithCommentForRemoteParentPlugins()
throws Exception {
Review Comment:
This test name/DisplayName currently asserts pluginManagement should be
injected for a plugin coming only from a remote parent, which contradicts the
PR summary (and the reported bug). If the intended fix is to skip
remote-parent-only plugins, rename the test accordingly to match the expected
behavior.
--
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]
