Skip to content
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.

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

ChangePluginGroupIdAndArtifactId does not allow for updating maven plugin versions #4902

Closed
JesseEstum opened this issue Jan 14, 2025 · 3 comments · Fixed by #4905
Closed
Labels
enhancement New feature or request

Comments

@JesseEstum
Copy link
Contributor

What problem are you trying to solve?

ChangePluginGroupIdAndArtifactId does not let you update the plugin's version.

ChangePluginGroupIdAndArtifactId allows one to update a maven plugin to reflect new:

  1. GroupId
  2. ArtifactId

A similar very similar recipe, ChangeDependencyGroupIdAndArtifactId, allows one to update a maven dependency to reflect new:

  1. GroupId
  2. ArtifactId
  3. Version

Describe the solution you'd like

I think ChangePluginGroupIdAndArtifactId should be brought into closer alignment with ChangeDependencyGroupIdAndArtifactId to also support changing a plugin version.

Have you considered any alternatives or workarounds?

I've checked the other recipes in the rewrite-maven module and found no suitable alternative solution.

Additional context

An example of the desired before/after change from this feature might look like this:

BEFORE

<build>
    <plugins>
        <plugin>
            <groupId>org.jvnet.jaxb2.maven2</groupId>
            <artifactId>maven-jaxb2-plugin</artifactId>
            <version>0.14.0</version>
            <executions>
                <execution>
                    <goals>
                        <goal>generate</goal>
                    </goals>
                </execution>
            </executions>
        </plugin>
    </plugins>
</build>

AFTER

<build>
    <plugins>
        <plugin>
            <groupId>org.jvnet.jaxb</groupId>
            <artifactId>jaxb-maven-plugin</artifactId>
            <version>4.0.8</version>
            <executions>
                <execution>
                    <goals>
                        <goal>generate</goal>
                    </goals>
                </execution>
            </executions>
        </plugin>
    </plugins>
</build>

Are you interested in contributing this feature to OpenRewrite?

Yes, I can offer to open a pull request to add the functionality. I would like to know from the group if there is consensus we should have this feature. I may also need design feedback to minimize turn-around-time on the PR.

@JesseEstum JesseEstum added the enhancement New feature or request label Jan 14, 2025
@JesseEstum
Copy link
Contributor Author

JesseEstum commented Jan 14, 2025

Code Change Analysis

Looking at the code changes required, there is a bit of a snag. Specifically, the last constructor arg / parameter to ChangePluginGroupIdAndArtifactId is deprecated. Thus, the go-to solution of "append another parameter called newVersion" will exacerbate the deprecation mess and make it hard to ever remove the deprecated newArtifact parameter in a non-breaking way.

Suggested Implementation

It may be cleaner to:

  1. Copy ChangePluginGroupIdAndArtifactId to a new class ChangePluginGroupIdAndArtifactIdAndVersion.
  2. Remove the deprecated newArtifact from the new class ChangePluginGroupIdAndArtifactIdAndVersion.
  3. Add an optional parameter, newVersion, to the last parameter position of the new class ChangePluginGroupIdAndArtifactIdAndVersion. Implement logic for newVersion, obviously :)
  4. Mark the entire class ChangePluginGroupIdAndArtifactId as deprecated.

Please provide feedback if you'd like me to cut a PR with this design.

@timtebeek
Copy link
Contributor

Thanks for calling out those discrepancies and their impact when making changes! I've done a quick search and updated a downstream project:

I'd think/hope that's the only reference to that field, and using the constructor is quite unexpected. With that I think we're ok to make the changes here in place as opposed to the more cumbersome alternatives. Would welcome such a PR, thanks! 🙏🏻

@JesseEstum
Copy link
Contributor Author

Thanks for the feedback, @timtebeek .
I'll bang out the PR per your recommended approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants