Migrate lifecycle mapping providers to use PluginVersions directly - #13138
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The migration is mechanically correct — all 8 subclasses updated, deprecated constants removed, PluginVersions.* usage is consistent.
One pre-existing issue is worth fixing in this cleanup PR since it's in the same file being touched.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Remove the deprecated *_PLUGIN_VERSION constants from AbstractLifecycleMappingProvider and update all subclasses to reference PluginVersions.* constants directly, as was done in the 4.0.x backport (apache#13137). The deprecated constants were introduced as a bridge in apache#13080 but the subclasses were never updated to stop using them. This completes the migration.
0fe8f3c to
e602a0e
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after rebase (e602a0e).
The new commits look correct: moving // START SNIPPET: versions / // END SNIPPET: versions markers into PluginVersions.java and updating the .md.vm snippet reference accordingly is the right place — PluginVersions is the canonical source, not AbstractLifecycleMappingProvider.
The outstanding finding from the previous review (trailing comma in MavenPluginLifecycleMappingProvider) is still present and still needs to be fixed.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| "org.apache.maven.plugins:maven-resources-plugin:" + PluginVersions.RESOURCES + ":testResources", | ||
| "test-compile", "org.apache.maven.plugins:maven-compiler-plugin:" + PluginVersions.COMPILER + ":testCompile", | ||
| "test", "org.apache.maven.plugins:maven-surefire-plugin:" + PluginVersions.SUREFIRE + ":test", | ||
| "package", "org.apache.maven.plugins:maven-jar-plugin:" + PluginVersions.JAR + ":jar,", |
There was a problem hiding this comment.
":jar," string is a leftover from when the package phase used to bind two goals (jar + addPluginArtifactMetadata) in a single comma-separated string. addPluginArtifactMetadata was removed but the trailing comma remained. goals.split(",") on "g:a:v:jar," produces ["g:a:v:jar", ""] — the empty string becomes a LifecycleMojo with a blank goal name. It's a silent no-op today but dead weight that could confuse future diagnostics.
| "package", "org.apache.maven.plugins:maven-jar-plugin:" + PluginVersions.JAR + ":jar,", | |
| "package", "org.apache.maven.plugins:maven-jar-plugin:" + PluginVersions.JAR + ":jar", |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after new push (0fe8f3c).
✅ Previous finding addressed: the trailing comma in MavenPluginLifecycleMappingProvider.java (:jar, → :jar) has been fixed in this commit.
The migration is mechanically correct and complete:
- All 8 packaging subclasses migrated from inherited
*_PLUGIN_VERSIONconstants to directPluginVersions.*references - Deprecated constants removed from
AbstractLifecycleMappingProvider // START SNIPPET: versions/// END SNIPPET: versionsmarkers cleaned up- No logic changes, no behavioral impact
Consistent with the 4.0.x backport (#13137). LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
2 participants
Follow-up to #13080.
#13080introducedPluginVersionsand marked the*_PLUGIN_VERSIONconstants inAbstractLifecycleMappingProvideras@Deprecated(since = "4.1.0", forRemoval = true), but the subclasses were never updated — they still reference the deprecated inherited constants.The 4.0.x backport (#13137) did it correctly: it migrated all subclasses to
PluginVersions.*directly and removed the deprecated constants fromAbstractLifecycleMappingProviderentirely.This PR brings master in line with the backport:
*LifecycleMappingProvidersubclasses to referencePluginVersions.*directly*_PLUGIN_VERSIONconstants fromAbstractLifecycleMappingProvider