search

openrewrite / rewrite-jenkins

OpenRewrite recipes to continuously modernize Jenkins plugins.
Apache License 2.0
9 stars 8 forks source link

Complete adoption into OpenRewrite GitHub Org #4

Closed timtebeek closed 1 year ago

timtebeek commented 1 year ago

What problem are you trying to solve?

Because this project started out independently, it diverges from other OpenRewrite modules, making it slightly harder to maintain automatically.

Describe the solution you'd like

Ensure we use the code style, CI and release pipeline from OpenRewrite.

Have you considered any alternatives or workarounds?

No; it's preferred for projects to look alike, such that we can maintain, release and document them with ease.

timtebeek commented 1 year ago

Most of the items above can be ticked off by looking at sibling modules, some such as the @DocumentExample are even available as recipe through Moderne, provided we get CI going first, and port the tests from Kotlin to Java first (I think).

sghill commented 1 year ago

@timtebeek it looks like the detach fork request needs to come from an OpenRewrite admin. I tried it here, but got this message.

Transcript:

timtebeek commented 1 year ago

Thanks! I've put an application as well; last time around it took only a couple hours I believe, so thanks for the reminder and link to get this started. And great to already see so many commits tagging this issue; looking forward to what you'll do with it! :)

timtebeek commented 1 year ago

Back from my travels and realized I'd forgotten to add this module to the platform before I left; your tweet was a nice reminder! I've gone and deployed the latest snapshot to https://app.moderne.io/marketplace/org.openrewrite.jenkins such that you can now also run your Java recipes there easily; let me know if you'd ever want a new snapshot to be deployed; we typically do so weekly.

sghill commented 1 year ago

Thanks @timtebeek! Is slack the best place for a snapshot deployment request?

It looks like the platform already has the logo, so I added the category.yml and checked that task. Looking at the module adding tasks next.

I ran the DocumentExample recipe and was curious why the annotation wasn't added to DisableLocalResolutionForParentPomTest. Should I manually annotate the other tests?

Also had a question about recipe structuring -- should I keep the recipes separate by version? I was modeling this a bit like the spring boot recipes, where the 2.x to 3.x recipe would call all the intermediate recipes (2.1 to 2.2). I liked this for a couple reasons:

I'm now a bit worried that we'll end up with hundreds of these recipes, since Jenkins tooling upgrades happen fast. So an alternative would be to maintain fewer, larger recipes (ModernizePlugin, UpgradeToLastJava8SupportedVersions).

timtebeek commented 1 year ago

Thanks @timtebeek! Is slack the best place for a snapshot deployment request?

Slack works and could be faster; I've gone ahead and deployed the latest snapshot just now!

It looks like the platform already has the logo, so I added the category.yml and checked that task. Looking at the module adding tasks next.

Awesome; merged those two PRs and crossed them off above. Thanks for adding the category.yml; helps feature the recipes in the appropriate place with some context.

I ran the DocumentExample recipe and was curious why the annotation wasn't added to DisableLocalResolutionForParentPomTest. Should I manually annotate the other tests?

That's odd indeed; I'd have expected it to be added there as well; I don't see any issues with ingestion or the test itself, and from the SelectRecipeExamples implementation can't work out why it wasn't applied. 🤔 Even ran it again with the same result for this project; did find and applied a couple other still, so thanks for the reminder!

For this project I guess we're going to either add some of those manually, or try to find and fix the issue with the recipe. :/

timtebeek commented 1 year ago

Also had a question about recipe structuring -- should I keep the recipes separate by version? I was modeling this a bit like the spring boot recipes, where the 2.x to 3.x recipe would call all the intermediate recipes (2.1 to 2.2). I liked this for a couple reasons:

  • recipes remain stable over time
  • easy to compose the last upgrade into the current upgrade
  • easy to see which parent pom required other recipes
  • a very old plugin may be able to go partially up-to-date easily, but the full migration may be better split over future commits. I've mostly seen this so far with upgrading to the last working Java 8 version, and then making the Java 11 jump in a different migration.

I'm now a bit worried that we'll end up with hundreds of these recipes, since Jenkins tooling upgrades happen fast. So an alternative would be to maintain fewer, larger recipes (ModernizePlugin, UpgradeToLastJava8SupportedVersions).

Good question; I don't know too much about the Jenkins internals, but I imagine you have a couple large bump versions, that have proven a hurdle in adoption, for instance by requiring more extensive changes, or upgrades to Java 8, 11, Jakarta, etc.. It could make sense then to treat those as "major" versions that each get their own yaml file, which can contain multiple documents to pull people up to that version, while chaining in any previous versions.

That way plugin developers are able pick from a number of well known reference points to pick up all changes up to that point, and migrate and review in steps. So rather than having potentially hundreds of individual recipes and files for each Jenkins version, instead pick a few reference points and group them that way.

Would that be an approach that sounds feasible here?

timtebeek commented 1 year ago

Looks like we ran through all the tasks set out in the initial issue above; all that's left then is to do an initial release I suppose, such that we can include this module with the next release of OpenRewrite and all it's modules. Are we ready for such a release? Or is there anything else you'd want to cover?

sghill commented 1 year ago

I'd like to try grouping by reference point as you suggested above, but that doesn't need to hold up an initial release if you'd prefer to do that sooner -- I see that rewrite-recipe-markdown-generator is failing due to not having an initial release.

timtebeek commented 1 year ago

I'll ping @mike-solomon to let him now we'd need to create a v0.2.0 tag of this repository before he can run the recipe-markdown-generator main branch again; that'll do for now I suppose. We can do that release at any time, and it'll pick up whatever the state is then. So you have a little while longer, as the docs were last updated Friday, and are on a rough weekly schedule.

We can close this issue after that initial release; and can of course continue the above discussions even after it's closed.

sghill commented 1 year ago

Sounds great! Could we get a snapshot deployed (whenever it's convenient) so I could see how some recent changes look on the platform?

I've made a few changes recently:

[WARNING] Changes have been made to pom.xml by:
[WARNING]     org.openrewrite.jenkins.ModernizePlugin
[WARNING]         org.openrewrite.maven.ChangeParentPom: {oldGroupId=org.jenkins-ci.plugins, oldArtifactId=plugin, newVersion=latest.release, allowVersionDowngrades=false}
[WARNING]         org.openrewrite.jenkins.UpgradeVersionProperty: {key=jenkins.version, minimumVersion=2.375.4}
[WARNING]         org.openrewrite.jenkins.AddPluginsBom
[WARNING] Please review and commit the results.

Locally the changes are looking good. I've upgraded a couple plugins and have seen their builds pass. There are a couple nice-to-haves still on my list:

type: specs.openrewrite.org/v1beta/style
name: io.jenkins.plugins.style
styleConfigs:
  - org.openrewrite.xml.style.TabsAndIndentsStyle:
      useTabCharacter: false
      tabSize: 4
      indentSize: 4
timtebeek commented 1 year ago

Sounds great! Could we get a snapshot deployed (whenever it's convenient) so I could see how some recent changes look on the platform?

Changes look good to me; thanks again! Deployed the latest snapshot to https://app.moderne.io/marketplace/org.openrewrite.jenkins

I've made a few changes recently:

  • Additional docs: displayNames, descriptions on declarative recipes, Option docs on java recipes
  • Retired some unused, experimental recipes
  • Tests and DocumentExample added for remaining recipes
  • Recipe grouping by reference point - this looks much better in the output when I run it locally
[WARNING] Changes have been made to pom.xml by:
[WARNING]     org.openrewrite.jenkins.ModernizePlugin
[WARNING]         org.openrewrite.maven.ChangeParentPom: {oldGroupId=org.jenkins-ci.plugins, oldArtifactId=plugin, newVersion=latest.release, allowVersionDowngrades=false}
[WARNING]         org.openrewrite.jenkins.UpgradeVersionProperty: {key=jenkins.version, minimumVersion=2.375.4}
[WARNING]         org.openrewrite.jenkins.AddPluginsBom
[WARNING] Please review and commit the results.

Locally the changes are looking good. I've upgraded a couple plugins and have seen their builds pass. There are a couple nice-to-haves still on my list:

  • CreateIndexJellyTest is disabled because the recipe is completing in 0 cycles

Not sure why those other three tests are failing, but the first noop test fails with a NPE that might also affect runs on projects that already contain a jelly file; here's what I'd suggest to prevent that NPE:

diff --git a/src/main/java/org/openrewrite/jenkins/CreateIndexJelly.java b/src/main/java/org/openrewrite/jenkins/CreateIndexJelly.java
index fa13391..11db884 100644
--- a/src/main/java/org/openrewrite/jenkins/CreateIndexJelly.java
+++ b/src/main/java/org/openrewrite/jenkins/CreateIndexJelly.java
@@ -52,7 +52,7 @@ public class CreateIndexJelly extends ScanningRecipe<CreateIndexJelly.Scanned> {
     public TreeVisitor<?, ExecutionContext> getVisitor(Scanned acc) {
         return Preconditions.check(acc.isJenkinsPlugin, new CreateTextFile(
                 acc.description,
-                acc.indexJellyPath.toString(),
+                acc.indexJellyPath,
                 true
         ).getVisitor());
     }
@@ -77,7 +77,7 @@ public class CreateIndexJelly extends ScanningRecipe<CreateIndexJelly.Scanned> {
                                 .orElse(pom.getMarkers().findFirst(MavenResolutionResult.class)
                                         .map(maven -> maven.getPom().getArtifactId())
                                         .orElseThrow(() -> new IllegalStateException("Expected to find an artifact id")));
-                        acc.indexJellyPath = sourceFile.getSourcePath().resolve("../src/main/resources/index.jelly").normalize();
+                        acc.indexJellyPath = sourceFile.getSourcePath().resolve("../src/main/resources/index.jelly").normalize().toString();
                         acc.isJenkinsPlugin = true;
                     }
                 }
@@ -89,6 +89,6 @@ public class CreateIndexJelly extends ScanningRecipe<CreateIndexJelly.Scanned> {
     static class Scanned {
         boolean isJenkinsPlugin;
         String description;
-        Path indexJellyPath;
+        String indexJellyPath;
     }
 }
  • This pom is reformatting every line to 2 spaces when I run the ModernizePlugin recipe, even if I activate a style I thought would force 4 spaces. Seems to be unique to this repo for some reason.
type: specs.openrewrite.org/v1beta/style
name: io.jenkins.plugins.style
styleConfigs:
  - org.openrewrite.xml.style.TabsAndIndentsStyle:
      useTabCharacter: false
      tabSize: 4
      indentSize: 4

Hmm; we might need to pass that style in when ingesting; although we typically get away with detecting the style as long as it's consistent. In the case of that pom.xml file the elements under <properties> are indented with two spaces where I would have expected four. It might already help to make it consistent there, although I'm not a 100% certain that's enough. We can try it and otherwise look at alternatives such as passing in the style when ingesting.

timtebeek commented 1 year ago

With the release of https://github.com/openrewrite/rewrite-jenkins/releases/tag/v0.2.0 we can close this issue. Thanks again for starting this effort, and all the work done so far in the adoption, running recipes at scale, and spreading the word about your results. ❤️