Merge pull request #514 from jonesbusy/bugfix/ensure-second-execution…

…-work Several improvements and fixes on recipes
jenkins-infra · Dec 27, 2024 · 7330237 · 7330237
2 parents 3fda79b + 4dfeb3f
commit 7330237
Show file tree

Hide file tree

Showing 22 changed files with 978 additions and 50 deletions.
diff --git a/plugin-modernizer-cli/pom.xml b/plugin-modernizer-cli/pom.xml
@@ -65,6 +65,12 @@
       <groupId>org.slf4j</groupId>
       <artifactId>slf4j-api</artifactId>
     </dependency>
+    <dependency>
+      <groupId>commons-io</groupId>
+      <artifactId>commons-io</artifactId>
+      <version>2.18.0</version>
+      <scope>test</scope>
+    </dependency>
     <!-- Test dependencies -->
     <dependency>
       <groupId>io.github.sparsick.testcontainers.gitserver</groupId>

diff --git a/...modernizer-cli/src/test/java/io/jenkins/tools/pluginmodernizer/cli/CommandLineITCase.java b/...modernizer-cli/src/test/java/io/jenkins/tools/pluginmodernizer/cli/CommandLineITCase.java
@@ -25,6 +25,7 @@
 import java.security.Security;
 import java.util.Properties;
 import java.util.stream.Stream;
+import org.apache.commons.io.FileUtils;
 import org.apache.maven.shared.invoker.DefaultInvocationRequest;
 import org.apache.maven.shared.invoker.DefaultInvoker;
 import org.apache.maven.shared.invoker.InvocationRequest;
@@ -119,7 +120,6 @@ public void testVersion() throws Exception {
                 () -> assertEquals(0, result2.getExitCode()),
                 () -> assertTrue(Files.readAllLines(logFile).stream()
                         .anyMatch(line -> line.matches("plugin modernizer ([a-zA-Z0-9.\\-_]+) (.*)"))));
-
         InvocationResult result3 = invoker.execute(buildRequest("version --short", logFile));
         assertAll(
                 () -> assertEquals(0, result3.getExitCode()),
@@ -286,27 +286,56 @@ public void testBuildMetadata(PluginMetadata expectedMetadata, WireMockRuntimeIn
     @Test
     public void testDryRunReplaceLibrariesWithApiPlugin(WireMockRuntimeInfo wmRuntimeInfo) throws Exception {
 
-        Path logFile = setupLogs("testDryRunReplaceLibrariesWithApiPlugin");
+        Path logFile1 = setupLogs("testDryRunReplaceLibrariesWithApiPlugin1");
+        Path logFile2 = setupLogs("testDryRunReplaceLibrariesWithApiPlugin2");
 
         final String plugin = "replace-by-api-plugins";
         final String recipe = "ReplaceLibrariesWithApiPlugin";
 
         // Junit attachment with logs file for the plugin build
         System.out.printf(
                 "[[ATTACHMENT|%s]]%n", Plugin.build(plugin).getLogFile().toAbsolutePath());
-        System.out.printf("[[ATTACHMENT|%s]]%n", logFile.toAbsolutePath());
+        System.out.printf("[[ATTACHMENT|%s]]%n", logFile1.toAbsolutePath());
+        System.out.printf("[[ATTACHMENT|%s]]%n", logFile2.toAbsolutePath());
 
         try (GitHubServerContainer gitRemote = new GitHubServerContainer(wmRuntimeInfo, keysPath, plugin, "main")) {
 
             gitRemote.start();
 
             Invoker invoker = buildInvoker();
             InvocationRequest request = buildRequest(
-                    "dry-run --recipe %s %s".formatted(recipe, getRunArgs(wmRuntimeInfo, plugin)), logFile);
+                    "dry-run --recipe %s %s".formatted(recipe, getRunArgs(wmRuntimeInfo, plugin)), logFile1);
             InvocationResult result = invoker.execute(request);
 
             // Assert output
-            assertAll(() -> assertEquals(0, result.getExitCode()));
+            assertAll(
+                    () -> assertEquals(0, result.getExitCode()),
+                    () -> assertTrue(Files.readAllLines(logFile1).stream()
+                            .anyMatch(line -> line.matches("(.*)Dry run mode. Changes were commited on (.*)"))));
+
+            // Delete target folder to use data from cache
+            File targetDirectory = cachePath
+                    .resolve("jenkins-plugin-modernizer-cli")
+                    .resolve(plugin)
+                    .resolve("sources")
+                    .resolve("target")
+                    .toFile();
+            FileUtils.deleteDirectory(targetDirectory);
+
+            // Ensure metadata is still present on cache
+            assertTrue(Files.exists(cachePath
+                    .resolve("jenkins-plugin-modernizer-cli")
+                    .resolve(plugin)
+                    .resolve(CacheManager.PLUGIN_METADATA_CACHE_KEY)));
+
+            // Ensure it still works after running again (with caching)
+            InvocationRequest request2 = buildRequest(
+                    "dry-run --recipe %s %s".formatted(recipe, getRunArgs(wmRuntimeInfo, plugin)), logFile2);
+            InvocationResult result2 = invoker.execute(request2);
+            assertAll(
+                    () -> assertEquals(0, result2.getExitCode()),
+                    () -> assertTrue(Files.readAllLines(logFile2).stream()
+                            .anyMatch(line -> line.matches("(.*)Dry run mode. Changes were commited on (.*)"))));
         }
     }
 

diff --git a/plugin-modernizer-cli/src/test/resources/replace-by-api-plugins/Jenkinsfile b/plugin-modernizer-cli/src/test/resources/replace-by-api-plugins/Jenkinsfile
@@ -0,0 +1,11 @@
+/*
+ See the documentation for more options:
+ https://github.com/jenkins-infra/pipeline-library/
+*/
+buildPlugin(
+  forkCount: '1C', // run this number of tests in parallel for faster feedback.  If the number terminates with a 'C', the value will be multiplied by the number of available CPU cores
+  useContainerAgent: true, // Set to `false` if you need to use Docker for containerized tests
+  configurations: [
+    [platform: 'linux', jdk: 21],
+    [platform: 'windows', jdk: 17],
+])
diff --git a/plugin-modernizer-core/pom.xml b/plugin-modernizer-core/pom.xml
@@ -165,6 +165,27 @@
       <artifactId>logback-classic</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>jakarta.servlet</groupId>
+      <artifactId>jakarta.servlet-api</artifactId>
+      <version>6.1.0</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>javax.servlet</groupId>
+      <artifactId>javax.servlet-api</artifactId>
+      <version>4.0.1</version>
+      <scope>test</scope>
+    </dependency>
+
+    <!-- Recipes test dependencies -->
+    <dependency>
+      <groupId>org.kohsuke.stapler</groupId>
+      <artifactId>stapler</artifactId>
+      <version>1928.v9115fe47607f</version>
+      <scope>test</scope>
+    </dependency>
+
     <!-- Test dependencies -->
     <dependency>
       <groupId>org.openrewrite</groupId>
@@ -177,6 +198,7 @@
       <version>2.1.7</version>
       <scope>test</scope>
     </dependency>
+
   </dependencies>
 
   <build>
@@ -206,6 +228,27 @@
       </resource>
     </resources>
     <plugins>
+
+      <!-- Used to copy openrewrite test dependencies to a separate directory for running java recipes -->
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-dependency-plugin</artifactId>
+        <version>3.8.1</version>
+        <executions>
+          <execution>
+            <id>copy-dependencies</id>
+            <goals>
+              <goal>copy-dependencies</goal>
+            </goals>
+            <phase>validate</phase>
+            <configuration>
+              <outputDirectory>${project.build.directory}/openrewrite-classpath</outputDirectory>
+              <!-- Adapt when testing java transformation -->
+              <includeGroupIds>javax.servlet,jakarta.servlet,org.kohsuke.stapler</includeGroupIds>
+            </configuration>
+          </execution>
+        </executions>
+      </plugin>
       <plugin>
         <groupId>gg.jte</groupId>
         <artifactId>jte-maven-plugin</artifactId>

diff --git a/...modernizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/config/Settings.java b/...modernizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/config/Settings.java
@@ -192,6 +192,10 @@ private static Path getDefaultMavenLocalRepo() {
         return readProperty("jenkins.parent.version", "versions.properties");
     }
 
+    public static @Nullable String getBomVersion() {
+        return readProperty("bom.version", "versions.properties");
+    }
+
     private static @Nullable URL getUpdateCenterUrl() throws MalformedURLException {
         String url = System.getenv("JENKINS_UC");
         if (url != null) {

diff --git a/...-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/extractor/MetadataVisitor.java b/...-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/extractor/MetadataVisitor.java
@@ -66,7 +66,7 @@ public Tree visit(Tree tree, ExecutionContext executionContext) {
         // Extract metadata from POM
         else if (PathUtils.matchesGlob(sourceFile.getSourcePath(), "**/pom.xml")) {
             LOG.debug("Visiting POM {}", sourceFile.getSourcePath());
-            PluginMetadata pomMetadata = new PomVisitor().reduce(tree, pluginMetadata);
+            PluginMetadata pomMetadata = new PomResolutionVisitor().reduce(tree, pluginMetadata);
             LOG.debug("POM metadata: {}", JsonUtils.toJson(pomMetadata));
             executionContext.putMessage("pomMetadata", pomMetadata); // Is there better than context messaging ?
             return tree;

diff --git a/...modernizer/core/extractor/PomVisitor.java → .../core/extractor/PomResolutionVisitor.java b/...modernizer/core/extractor/PomVisitor.java → .../core/extractor/PomResolutionVisitor.java
@@ -12,13 +12,24 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-public class PomVisitor extends MavenIsoVisitor<PluginMetadata> {
+/**
+ * A pom visitor that accumulate PluginMetadata using maven resolution result.
+ * Maven resolution might not get updated if the tree is modified by other visitor
+ * So it's best used in preconditons recipes to avoid side effect
+ * An other implementation of this visitor could be extraction from the tree instead of maven resolution result
+ */
+public class PomResolutionVisitor extends MavenIsoVisitor<PluginMetadata> {
 
-    private static final Logger LOG = LoggerFactory.getLogger(PomVisitor.class);
+    /**
+     * LOGGER.
+     */
+    private static final Logger LOG = LoggerFactory.getLogger(PomResolutionVisitor.class);
 
     @Override
     public Xml.Document visitDocument(Xml.Document document, PluginMetadata pluginMetadata) {
 
+        document = super.visitDocument(document, pluginMetadata);
+
         Markers markers = getCursor().firstEnclosingOrThrow(Xml.Document.class).getMarkers();
 
         // Ensure maven resolution result is present

diff --git a/...nizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/recipes/IsMissingBom.java b/...nizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/recipes/IsMissingBom.java
@@ -1,7 +1,7 @@
 package io.jenkins.tools.pluginmodernizer.core.recipes;
 
 import io.jenkins.tools.pluginmodernizer.core.extractor.PluginMetadata;
-import io.jenkins.tools.pluginmodernizer.core.extractor.PomVisitor;
+import io.jenkins.tools.pluginmodernizer.core.extractor.PomResolutionVisitor;
 import org.openrewrite.ExecutionContext;
 import org.openrewrite.Recipe;
 import org.openrewrite.TreeVisitor;
@@ -37,7 +37,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
             @Override
             public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
 
-                PluginMetadata pluginMetadata = new PomVisitor().reduce(document, new PluginMetadata());
+                PluginMetadata pluginMetadata = new PomResolutionVisitor().reduce(document, new PluginMetadata());
 
                 if (pluginMetadata.getBomVersion() == null) {
                     LOG.info("Project is missing Jenkins BOM");

diff --git a/...java/io/jenkins/tools/pluginmodernizer/core/recipes/IsMissingJenkinsBaselineProperty.java b/...java/io/jenkins/tools/pluginmodernizer/core/recipes/IsMissingJenkinsBaselineProperty.java
@@ -1,7 +1,7 @@
 package io.jenkins.tools.pluginmodernizer.core.recipes;
 
 import io.jenkins.tools.pluginmodernizer.core.extractor.PluginMetadata;
-import io.jenkins.tools.pluginmodernizer.core.extractor.PomVisitor;
+import io.jenkins.tools.pluginmodernizer.core.extractor.PomResolutionVisitor;
 import org.openrewrite.ExecutionContext;
 import org.openrewrite.Recipe;
 import org.openrewrite.TreeVisitor;
@@ -37,7 +37,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
             @Override
             public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
 
-                PluginMetadata pluginMetadata = new PomVisitor().reduce(document, new PluginMetadata());
+                PluginMetadata pluginMetadata = new PomResolutionVisitor().reduce(document, new PluginMetadata());
 
                 if (!pluginMetadata.getProperties().containsKey("jenkins.baseline")) {
                     LOG.info("Project is missing jenkins.baseline property");

diff --git a/...ernizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/recipes/IsUsingBom.java b/...ernizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/recipes/IsUsingBom.java
@@ -1,7 +1,7 @@
 package io.jenkins.tools.pluginmodernizer.core.recipes;
 
 import io.jenkins.tools.pluginmodernizer.core.extractor.PluginMetadata;
-import io.jenkins.tools.pluginmodernizer.core.extractor.PomVisitor;
+import io.jenkins.tools.pluginmodernizer.core.extractor.PomResolutionVisitor;
 import org.openrewrite.ExecutionContext;
 import org.openrewrite.Recipe;
 import org.openrewrite.TreeVisitor;
@@ -37,7 +37,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
             @Override
             public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
 
-                PluginMetadata pluginMetadata = new PomVisitor().reduce(document, new PluginMetadata());
+                PluginMetadata pluginMetadata = new PomResolutionVisitor().reduce(document, new PluginMetadata());
 
                 if (pluginMetadata.getBomVersion() != null) {
                     LOG.info("Project is using Jenkins bom at version {}", pluginMetadata.getBomVersion());

diff --git a/...java/io/jenkins/tools/pluginmodernizer/core/recipes/MigrateToJenkinsBaseLineProperty.java b/...java/io/jenkins/tools/pluginmodernizer/core/recipes/MigrateToJenkinsBaseLineProperty.java
@@ -1,31 +1,40 @@
 package io.jenkins.tools.pluginmodernizer.core.recipes;
 
 import io.jenkins.tools.pluginmodernizer.core.extractor.PluginMetadata;
-import io.jenkins.tools.pluginmodernizer.core.extractor.PomVisitor;
+import io.jenkins.tools.pluginmodernizer.core.extractor.PomResolutionVisitor;
 import io.jenkins.tools.pluginmodernizer.core.visitors.AddBeforePropertyVisitor;
 import io.jenkins.tools.pluginmodernizer.core.visitors.AddPropertyCommentVisitor;
 import org.jspecify.annotations.Nullable;
 import org.openrewrite.*;
 import org.openrewrite.Cursor;
 import org.openrewrite.maven.AddPropertyVisitor;
+import org.openrewrite.maven.ChangeManagedDependencyGroupIdAndArtifactId;
 import org.openrewrite.maven.MavenIsoVisitor;
-import org.openrewrite.xml.ChangeTagValueVisitor;
 import org.openrewrite.xml.tree.Xml;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+/**
+ * A recipe to migrate the pom.xml to use the jenkins.baseLine property.
+ * Doesn't upgrade the jenkins version or bom version.
+ * If the plugin is not using the bom or using the weekly baseline, it will skip the update.
+ * This will also fix the bom version if it doesn't match the jenkins.version.
+ */
 public class MigrateToJenkinsBaseLineProperty extends Recipe {
 
+    /**
+     * Logger for the class.
+     */
     public static final Logger LOG = LoggerFactory.getLogger(MigrateToJenkinsBaseLineProperty.class);
 
     @Override
     public String getDisplayName() {
-        return "Migrate to Jenkins baseline property";
+        return "Update the pom.xml to use newly introduced jenkins.baseline property";
     }
 
     @Override
     public String getDescription() {
-        return "Migrate to Jenkins baseline property.";
+        return "Update the pom.xml to use newly introduced jenkins.baseline property.";
     }
 
     @Override
@@ -47,8 +56,8 @@ private static class MigrateToJenkinsBaseLinePropertyVisitor extends MavenIsoVis
         public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
             Xml.Document d = super.visitDocument(document, ctx);
 
-            // Skip if not bom or weekly bom
-            PluginMetadata pluginMetadata = new PomVisitor().reduce(document, new PluginMetadata());
+            // Skip if not using bom or using weekly baseline
+            PluginMetadata pluginMetadata = new PomResolutionVisitor().reduce(document, new PluginMetadata());
             if (pluginMetadata.getBomArtifactId() == null || "bom-weekly".equals(pluginMetadata.getBomArtifactId())) {
                 LOG.debug("Project is using Jenkins weekly bom or not bom, skipping");
                 return d;
@@ -58,6 +67,10 @@ public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
             return document;
         }
 
+        /**
+         * Perform the update.
+         * @param document The document to update.
+         */
         private void performUpdate(Xml.Document document) {
 
             Xml.Tag jenkinsVersionTag = document.getRoot()
@@ -71,7 +84,7 @@ private void performUpdate(Xml.Document document) {
 
             // Keep only major and minor and ignore patch version
             String jenkinsVersion = jenkinsVersionTag.getValue().get();
-            String jenkinsBaseline = jenkinsVersionTag.getValue().get();
+            String jenkinsBaseline = jenkinsVersion;
             String jenkinsPatch = null;
 
             // It's a LTS, extract patch
@@ -102,20 +115,31 @@ private void performUpdate(Xml.Document document) {
             }
 
             // Change the bom artifact ID
-            Xml.Tag artifactIdTag = document.getRoot()
+            Xml.Tag bom = document.getRoot()
                     .getChild("dependencyManagement")
                     .flatMap(dm -> dm.getChild("dependencies"))
                     .flatMap(deps -> deps.getChild("dependency"))
                     .filter(dep -> dep.getChildValue("groupId")
                             .map("io.jenkins.tools.bom"::equals)
                             .orElse(false))
-                    .flatMap(dep -> dep.getChild("artifactId"))
                     .orElseThrow();
 
+            Xml.Tag artifactIdTag = bom.getChild("artifactId").orElseThrow();
+            Xml.Tag version = bom.getChild("version").orElseThrow();
+
             LOG.debug("Artifact ID is {}", artifactIdTag.getValue().get());
+            LOG.debug("Version is {}", version.getValue().get());
 
+            // Change the artifact and perform upgrade
             if (!artifactIdTag.getValue().get().equals("bom-${jenkins.baseline}.x")) {
-                doAfterVisit(new ChangeTagValueVisitor<>(artifactIdTag, "bom-${jenkins.baseline}.x"));
+                doAfterVisit(new ChangeManagedDependencyGroupIdAndArtifactId(
+                                "io.jenkins.tools.bom",
+                                artifactIdTag.getValue().get(),
+                                "io.jenkins.tools.bom",
+                                "bom-${jenkins.baseline}.x",
+                                version.getValue().get(),
+                                "\\.v[a-f0-9_]+")
+                        .getVisitor());
             }
         }
     }