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

Maven: Resolve artifactId properties before downloading versions #4404

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

sghill
Copy link
Contributor

@sghill sghill commented Aug 10, 2024

What's changed?

Support changing managed maven dependencies with artifactIds that include properties.

What's your motivation?

Jenkins has moved to a model where a bom is now defined like

<artifactId>bom-${jenkins.baseline}.x</artifactId>

This breaks the org.openrewrite.jenkins.ModernizePlugin recipe. Attempting to use an artifactId with a property fails to resolve:

Caused by: org.openrewrite.internal.RecipeRunException: java.lang.IllegalArgumentException: Illegal character in path at index 67: file:///home/sghill/.m2/repository/jakarta/activation/jakarta.acti${partial.arti}-api/maven-metadata.xml

jenkinsci/archetypes#737

Anyone you would like to review specifically?

@timtebeek and I briefly talked about this on OSS Slack.

Have you considered any alternatives or workarounds?

I made an attempt to fix this in the MavenPomDownloader in sghill@941e1ee. It didn't work because the set of resolved poms through this codepath is always empty.

Since this is valid maven, I think it'd be nice if this were universally supported instead of just fixed in this one recipe. I'm not quite sure how to go about making it more universal.

Any additional context

This is more for discussion than an implementation I feel strongly about. My ideal end state is for the ModernizePlugin recipe to compose these building blocks to extract a jenkins.baseline property and use it in multiple places, just like the updated plugin archetype.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@jonesbusy
Copy link
Contributor

In fact such placeholder cause other issues when some recipes

When testing

type: specs.openrewrite.org/v1beta/recipe
name: io.jenkins.tools.pluginmodernizer.UpgradeParentVersion
recipeList:
  - org.openrewrite.maven.UpgradeParentVersion:
      groupId: org.jenkins-ci.plugins
      artifactId: plugin
      newVersion: 4.X

I got some result that OpenRewrite is moving some dependency on dependencyManagement even if they are managed by the pom. Probably because the resolution with placeholder doesn't happen correctly

   <parent>
     <groupId>org.jenkins-ci.plugins</groupId>
     <artifactId>plugin</artifactId>
-    <version>4.86</version>
+    <version>4.87</version>
     <relativePath />
   </parent>
   <groupId>io.jenkins.plugins</groupId>
@@ -39,6 +39,11 @@
   <dependencyManagement>
     <dependencies>
       <dependency>
+        <groupId>io.jenkins.plugins</groupId>
+        <artifactId>asm-api</artifactId>
+        <version>9.7-33.v4d23ef79fcc8</version>
+      </dependency>
+      <dependency>
         <groupId>io.jenkins.tools.bom</groupId>
         <artifactId>bom-${jenkins.baseline}.x</artifactId>
         <version>3271.vf18ea_cb_9edfb_</version>

If I change my pom bom artifact to

<artifactId>bom-2.440.x</artifactId>

Everything is back to normal

@@ -4,7 +4,7 @@ io.jenkins.tools.pluginmodernizer.UpgradeParentVersion
   <parent>
     <groupId>org.jenkins-ci.plugins</groupId>
     <artifactId>plugin</artifactId>
-    <version>4.86</version>
+    <version>4.87</version>
     <relativePath />
   </parent>
   <groupId>io.jenkins.plugins</groupId>

@jonesbusy
Copy link
Contributor

Some other occurence caused by the placeholder

[ERROR] Failed to execute goal org.openrewrite.maven:rewrite-maven-plugin:5.39.1:run (default-cli) on project jobcacher: Execution default-cli of goal org.openrewrite.maven:rewrite-maven-plugin:5.39.1:run failed: Error while visiting pom.xml: java.lang.IllegalStateException: Illegal state while comparing versions : [${jenkins.baseline}.3.0.0.0.0.0.0] and [2.462.1]. Metadata = [2.462.1] 
[ERROR]   org.openrewrite.semver.LatestRelease.compare(LatestRelease.java:162) 
[ERROR]   org.openrewrite.semver.VersionComparator.upgrade(VersionComparator.java:45) 
[ERROR]   org.openrewrite.jenkins.UpgradeVersionProperty$1.visitDocument(UpgradeVersionProperty.java:69) 
[ERROR]   org.openrewrite.jenkins.UpgradeVersionProperty$1.visitDocument(UpgradeVersionProperty.java:62) 
[ERROR]   org.openrewrite.xml.tree.Xml$Document.acceptXml(Xml.java:145) 
[ERROR]   org.openrewrite.xml.tree.Xml.accept(Xml.java:50) 

@jonesbusy
Copy link
Contributor

Is it something we can move on or is prefereble to have a general solution like #4527

I'm asking because we are movin now from a new recommanded version (from 2.440 to 2.452) and we are seeing more and more failing due to the new archetype

<artifactId>bom-${jenkins.baseline}.x</artifactId>

Thanks!

@sambsnyd sambsnyd merged commit 69f6f2e into openrewrite:main Oct 11, 2024
1 of 2 checks passed
@sambsnyd
Copy link
Member

Thanks for this contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants