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

Jenkinsfile JDK Version Check in UpgradeNextMajorParentVersion Recipe #628

Open
gounthar opened this issue Jan 13, 2025 · 8 comments
Open

Comments

@gounthar
Copy link
Collaborator

gounthar commented Jan 13, 2025

What Feature Do You Want to See Added?

Currently, the UpgradeNextMajorParentVersion recipe does not verify if the Jenkinsfile is using JDK 17 or newer. This omission can lead to errors if the plugin is upgraded without updating the Jenkinsfile to the appropriate JDK version. As experienced with the date-parameter plugin, manual modification of the Jenkinsfile was necessary. It would be beneficial to include a check within the recipe to ensure that the Jenkinsfile specifies JDK 17 or a newer version.

Proposed Solution

Enhance the UpgradeNextMajorParentVersion recipe to automatically check the Jenkinsfile for the correct JDK version. If the version is older than 17, the recipe should either update when it's possible, or add it if it's not there.

Upstream Changes

No response

Are You Interested in Contributing This Feature?

@gounthar gounthar changed the title UpgradeNextMajorParentVersion recipe does not check if Jenkinsfile contains the JDK 17 or newer. Jenkinsfile JDK Version Check in UpgradeNextMajorParentVersion Recipe Jan 13, 2025
@gounthar gounthar changed the title Jenkinsfile JDK Version Check in UpgradeNextMajorParentVersion Recipe Jenkinsfile JDK Version Check in UpgradeNextMajorParentVersion Recipe Jan 13, 2025
@jonesbusy
Copy link
Collaborator

Probably the best is to not do any change. At the end even if modyfing the Jenkinsfile you will get build failure until a maintainer replay the pipeline or do the update him/herself.

@gounthar
Copy link
Collaborator Author

That's certainly true, but in my opinion, it would be better to propose the change in the Jenkinsfile with an accompanying explanation rather than simply letting the build fail without any context.
Let's agree to disagree. 😉

@jonesbusy
Copy link
Collaborator

So when #531 is implemented it just need to be added on the list of declarative recipe.

In the meanwhile do you prefer to not do any change ? But my concern is that such checks must be added on all recipes of the declarative or you will end up with partial changes (for example a parent on 5.x but the jenkins version untouched)

So my opininion is to not do anything until #531 is implemented

@jonesbusy
Copy link
Collaborator

Or an other option is to add on the recipe

  - org.openrewrite.text.CreateTextFile:
      relativeFileName: Jenkinsfile
      overwriteExisting: true // <-- This
      fileContents: |
        /*
         See the documentation for more options:
         https://github.com/jenkins-infra/pipeline-library/
        */
        buildPlugin(
          forkCount: '1C', // Run a JVM per core in tests
          useContainerAgent: true, // Set to `false` if you need to use Docker for containerized tests
          configurations: [
            [platform: 'linux', jdk: 21],
            [platform: 'windows', jdk: 17],
        ])

If we agree to completly override Jenkinsfile rather than just inserting the correct JDKs

@jonesbusy
Copy link
Collaborator

Which might cause other issue like disable docker test as mentionned #531 (comment) So I'm also not super agree with a destructive approach

@gounthar
Copy link
Collaborator Author

I don't know if we'll be able to parse the Jenkinsfile and "just" change the JDK versions we detected during the metadata phase, or linked to the recipe we're applying. 🤔

@jonesbusy
Copy link
Collaborator

I think it's possible using rewrite-groovy and GroovyIsoVisitor. It would be similar to the Jenkinsfile visitor. First we collect what we find, then update the arguments depending on the context. Either updating the LST since it's rather simple arguments or using JavaMethodTemplate (or something similar).

@gounthar
Copy link
Collaborator Author

Sounds cool. 👍

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

No branches or pull requests

2 participants