openrewrite / rewrite-jenkins

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

Jenkinsfile is not overwritten #52

Closed gounthar closed 8 months ago

gounthar commented 8 months ago

What version of OpenRewrite are you using?

I'm using it through Moderne.io, so I don't know, sorry.

How are you running OpenRewrite?

I'm using Moderne.io. I don't think you will be able to see the results, but I used my latest recipe of recipes that gave: https://app.moderne.io/results/Dm3ZuYTUW

The plugin I tried this on is multibranch-scan-webhook-trigger-plugin. You can see the first commit there: https://github.com/jenkinsci/multibranch-scan-webhook-trigger-plugin/pull/17/commits/fde4557597676025b291585f9908cbb53faf8f0d

You can replay the recipe of recipes here: https://app.moderne.io/recipes/builder/j0daJWhoh?organizationId=SmVua2lucyBDSQ%3D%3D .

What is the smallest, simplest way to reproduce the problem?

Re-run the recipe. A friend of mine tried only the modernize Jenkinsfile on his machine with the same project thanks to the maven command, and it failed to change the file too.

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run \
  -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-jenkins:RELEASE \
  -Drewrite.activeRecipes=org.openrewrite.jenkins.ModernizeJenkinsfile

Once the file was deleted, the recipe created the file correctly.

What did you expect to see?

Well, at least a rewritten Jenkinsfile, even if the complexity of the Jenkinsfile was lost (some of them are not just buildplugin(), but lots of instructions like in https://raw.githubusercontent.com/jenkinsci/kubernetes-plugin/master/Jenkinsfile.

What did you see instead?

Nothing, until we deleted the local Jenkinsfile.

What is the full stack trace of any errors you encountered?

No error.

Are you interested in [contributing a fix to OpenRewrite]

I'd love to contribute if possible. While I may not have all the required skills, I'm enthusiastic about the project and willing to learn.

timtebeek commented 8 months ago

Thanks for reporting your issue here and offer to help! I've had a quick look, adding some diagnostics from the serialized LST manifest.csv:

n,sourcePath,sourceFileType,buildTool,buildToolVersion,checksum
1,pom.xml,Xml.Document,moderne-maven-plugin,2.4.3,35afca4aa95d5230b61a9dac0ee7e038
2,src/main/java/com/igalg/jenkins/plugins/mswt/ComputedFolderWebHookRequestReceiver.java,J.CompilationUnit,moderne-maven-plugin,2.4.3,f8fab40063f5e38bc384455dc8958de4
3,src/main/java/com/igalg/jenkins/plugins/mswt/trigger/ComputedFolderWebHookTriggerResult.java,J.CompilationUnit,moderne-maven-plugin,2.4.3,6919234fdcf6c0f02d37de48e60fa741
4,src/main/java/com/igalg/jenkins/plugins/mswt/trigger/ComputedFolderWebHookTrigger.java,J.CompilationUnit,moderne-maven-plugin,2.4.3,c8f276ddacf0359ab40224cb7fd23307
5,src/main/java/com/igalg/jenkins/plugins/mswt/locator/JenkinsInstanceComputedFolderLocator.java,J.CompilationUnit,moderne-maven-plugin,2.4.3,b8f628482d4f9af09de8f95427cb3333
6,src/main/java/com/igalg/jenkins/plugins/mswt/locator/ComputedFolderLocator.java,J.CompilationUnit,moderne-maven-plugin,2.4.3,1c544f45f9300552b9bc7c7172a49875
7,src/main/java/com/igalg/jenkins/plugins/mswt/locator/LocatedComputedFolder.java,J.CompilationUnit,moderne-maven-plugin,2.4.3,a6d1674f09e2c3ad3c7faf5d9f79422f
8,src/main/java/com/igalg/jenkins/plugins/mswt/locator/ComputedFolderWebHookTriggerLocator.java,J.CompilationUnit,moderne-maven-plugin,2.4.3,685d718498d611c44a00b27042a9cd64
9,src/main/resources/index.jelly,Quark,moderne-maven-plugin,2.4.3,d08d63152a65ac77e6f43ec6fb67c194
10,src/main/resources/com/igalg/jenkins/plugins/mswt/trigger/ComputedFolderWebHookTrigger/help-token.html,Quark,moderne-maven-plugin,2.4.3,4ffff340bf411198149254dfd164b4c6
11,src/main/resources/com/igalg/jenkins/plugins/mswt/trigger/ComputedFolderWebHookTrigger/config.jelly,Quark,moderne-maven-plugin,2.4.3,52e4259cdd96d3eeef3697ab71037e67
12,src/main/resources/com/igalg/jenkins/plugins/mswt/trigger/ComputedFolderWebHookTrigger/help.html,Quark,moderne-maven-plugin,2.4.3,fff04db5168fb000d58fca3bf29fc422
13,src/test/java/com/igalg/jenkins/plugins/mswt/ComputedFolderWebHookRequestReceiverTest.java,J.CompilationUnit,moderne-maven-plugin,2.4.3,27e1e81245baa61c1403b9ac0cb589d7
14,src/test/java/com/igalg/jenkins/plugins/mswt/locator/ComputedFolderWebHookTriggerLocatorTest.java,J.CompilationUnit,moderne-maven-plugin,2.4.3,2cb32359672c6b6e05005881d0cef615
15,Jenkinsfile,G.CompilationUnit,mod-java,v0.1.1-SNAPSHOT,82c77d7c20e52b57a2b5a886e4a22803
16,.gitignore,PlainText,native,v1.6.0,3b37ecfb3c733c8bcd5bc46c842fcd58
17,LICENSE,Quark,native,v1.6.0,a8c90949a803f50e7f2359fc05a0fa9d
18,README.md,PlainText,native,v1.6.0,13978609ca77aa53a2298b5b32f85fff
19,images/configure-token.png,Quark,native,v1.6.0,3b577d022f94f9cab92a069022cce4ea

I don't yet know what's going on; especially since you're reporting the same issue locally. 🤔

I had thought there might be a parse issue, but from the above that does not immediately seem the case.

timtebeek commented 8 months ago

Gave it a go here as well just now; seeing the same behaviour, and can trace that back to how the recipe is implemented: https://github.com/openrewrite/rewrite-jenkins/blob/8914ccd62655d65f2c54e2e40ed6289f13f69542/src/main/resources/META-INF/rewrite/rewrite.yml#L53-L70 The recipe right now does not override files of a different type, as evidenced by this failing test:

    @Test
    void shouldOverrideExisting() {
        String after = """
          /*
           See the documentation for more options:
           https://github.com/jenkins-infra/pipeline-library/
          */
          buildPlugin(
            useContainerAgent: true, // Set to `false` if you need to use Docker for containerized tests
            configurations: [
              [platform: 'linux', jdk: 21],
              [platform: 'windows', jdk: 17],
          ])
          """;
        rewriteRun(
          spec -> spec.recipe(new CreateTextFile(after, "Jenkinsfile", true)),
          groovy(
            """
              #!groovy
              buildPlugin()
              """,
            after,
            spec -> spec.path("Jenkinsfile")
          )
        );
    }

I'll see if I can fix that in OpenRewrite/rewrite itself

timtebeek commented 8 months ago

Picked up in

The problem is a side effect of now parsing Jenkinsfiles as Groovy rather than PlainText that we did before. Thanks for bringing this to our attention!

timtebeek commented 8 months ago

The underlying issue has been fixed, and should roll out to our snapshot versions soon. We deploy the latest changes to app.moderne.io such that it should be usable for the next batch of projects again.

I see you've already made the required changes to your jdk-21-prerequisites-and-powermock branch, such that I think we don't have to expedite a deploy & release, right?

Thanks again for pointing out this issue!

gounthar commented 8 months ago

That's right, @timtebeek , but I will try it on other repositories. Thanks a bunch for the super fast fix. 🙏