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

Use the detected JDK to use the right values in SetupJenkinsfile. #598

Open
gounthar opened this issue Jan 8, 2025 · 17 comments · May be fixed by #679
Open

Use the detected JDK to use the right values in SetupJenkinsfile. #598

gounthar opened this issue Jan 8, 2025 · 17 comments · May be fixed by #679
Labels
good first issue Good for newcomers

Comments

@gounthar
Copy link
Collaborator

gounthar commented Jan 8, 2025

What feature do you want to see added?

For the time being, we are using the same values for everyone: JDK 21 under Linux and JDK 17 under Windows.

For the plugins that do not yet have a Jenkinsfile, this transition might be quite abrupt, as they are most likely still using JDK 8 or older. Would it be possible to use a parameterized recipe to incorporate the values detected during the metadata gathering?

In this way, the Jenkinsfile would serve as a template, allowing us to dynamically insert the detected JDK instead of using fixed values like 17 or 21.

We do not need to modify the existing recipe; we could create a new one instead.

Upstream changes

No response

Are you interested in contributing this feature?

No response

@jonesbusy
Copy link
Collaborator

This could be a first step before #531 is fully implemented

Until we have more logic to maintains the Jenkinsfile and make them evolve (For example Java 25)

@jonesbusy jonesbusy added the good first issue Good for newcomers label Jan 16, 2025
@jonesbusy
Copy link
Collaborator

jonesbusy commented Jan 16, 2025

My thought if a contributor want to take this task (it's a rather easy recipe since we have already the pieces to assemble it. like the JDK model or PomResolutionVisitor)

  • Replace the org.openrewrite.text.CreateTextFile by our own scanning recipe io.jenkins.tools.pluginmodernizer.core.AddJenkinsfile
  • Create the scanner public TreeVisitor<?, ExecutionContext> getScanner(PlatformConfigAccumulator acc) (similar to https://github.com/jenkins-infra/plugin-modernizer-tool/pull/630/files) but that will visit the pom.xml and run PomResolutionVisitor
  • Use List jenkinsJdks = JDK.get(minimumVersion) to get JDK version possible for a Jenkins version
  • Create new text file with 2 latest JDK
  • If the Jenkinsfile already exist, do nothing since it will be done on an other recipe (or this one extended)

Similar to but with adapted version (this can be a simple string format with placeholder)

        /*
         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],
        ])

Pseudo-code

@Language("groovy")
public static String JENKINSFILE_TEMPLATE = """
        /*
         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: %s],
            [platform: 'windows', jdk: %s],
        ])
""".formatted(hightestCompatibleJdk, nextjdK);

@nagu165
Copy link
Contributor

nagu165 commented Jan 16, 2025

My thought if a contributor want to take this task (it's a rather easy recipe since we have already the pieces to assemble it. like the JDK model or PomResolutionVisitor)

  • Replace the org.openrewrite.text.CreateTextFile by our own scanning recipe io.jenkins.tools.pluginmodernizer.core.AddJenkinsfile
  • Create the scanner public TreeVisitor<?, ExecutionContext> getScanner(PlatformConfigAccumulator acc) (similar to https://github.com/jenkins-infra/plugin-modernizer-tool/pull/630/files) but that will visit the pom.xml and run PomResolutionVisitor
  • Use List jenkinsJdks = JDK.get(minimumVersion) to get JDK version possible for a Jenkins version
  • Create new text file with 2 latest JDK

Hello sir, I'm interested in working on this.
Question : 2 latest versions meaning 1 for linux and 1 for windows correct?

  • If the Jenkinsfile already exist, do nothing since it will be done on an other recipe (or this one extended)

Similar to but with adapted version (this can be a simple string format with placeholder)

        /*
         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],
        ])

Pseudo-code

@Language("groovy")
public static String JENKINSFILE_TEMPLATE = """
        /*
         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: %s],
            [platform: 'windows', jdk: %s],
        ])
""".formatted(hightestCompatibleJdk, nextjdK);

@gounthar
Copy link
Collaborator Author

JDK 17 is the current recommended version, but JDK 21 is just around the corner. So "two latest versions" means (to me at least) JDK 17 and JDK 21. We don't have to target both Windows and Linux if the Jenkinsfile already exists and targets only one platform. If there is no Jenkinsfile, it's reasonable to target JDK 21 for Linux and JDK 17 for Windows.

Thanks.

@nagu165
Copy link
Contributor

nagu165 commented Jan 16, 2025

Understood sir,
Thank you for the response.

@jonesbusy
Copy link
Collaborator

jonesbusy commented Jan 16, 2025

For this issue it would be for plugin not using Jenkinsfile. To allow creating a Jenkinsfile corresponding on to the current version

The modification of an existing jenkinsfile will be done by other recipes (for example the UpgradeJenkinsVersion) which will be done by #628 and #531 (which are a bit more complex)

@nagu165
Copy link
Contributor

nagu165 commented Jan 16, 2025

For this issue it would be for plugin not using Jenkinsfile. To allow creating a Jenkinsfile corresponding on to the current version

Jenkinsfile need to be created for this right?

The modification of an existing jenkinsfile will be done by other recipes (for example the UpgradeJenkinsVersion) which will be done by #628 and #531 (which are a bit more complex)

@gounthar
Copy link
Collaborator Author

My bad, @jonesbusy, I mixed the two in my attempt to explain. 😊
Thanks for pointing it out. 👍

So, a new Jenkinsfile with both platforms, Linux and Windows, using the detected Java version. Am I right this time? 🤞

@jonesbusy
Copy link
Collaborator

Jenkinsfile need to be created for this right?

Not it will be created. Given a jenkins version we know what are the supported version (CF JDK.java)

@jonesbusy
Copy link
Collaborator

My bad, @jonesbusy, I mixed the two in my attempt to explain. 😊 Thanks for pointing it out. 👍

So, a new Jenkinsfile with both platforms, Linux and Windows, using the detected Java version. Am I right this time? 🤞

Yes correct. On the archetype we use Linux platform on most recent JDK and Windows for the previous now. I don't think we need to change this behavior

@nagu165
Copy link
Contributor

nagu165 commented Jan 17, 2025

My thought if a contributor want to take this task (it's a rather easy recipe since we have already the pieces to assemble it. like the JDK model or PomResolutionVisitor)

  • Replace the org.openrewrite.text.CreateTextFile by our own scanning recipe io.jenkins.tools.pluginmodernizer.core.AddJenkinsfile

Hello sir, Did you mean 'io.jenkins.tools.pluginmodernizer.SetupJenkinsfile' since i couldn't find a file named 'AddJenkinsfile'

  • Create the scanner public TreeVisitor<?, ExecutionContext> getScanner(PlatformConfigAccumulator acc) (similar to https://github.com/jenkins-infra/plugin-modernizer-tool/pull/630/files) but that will visit the pom.xml and run PomResolutionVisitor
  • Use List jenkinsJdks = JDK.get(minimumVersion) to get JDK version possible for a Jenkins version
  • Create new text file with 2 latest JDK
  • If the Jenkinsfile already exist, do nothing since it will be done on an other recipe (or this one extended)

Similar to but with adapted version (this can be a simple string format with placeholder)

    /*
     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],
    ])

Pseudo-code

@language("groovy")
public static String JENKINSFILE_TEMPLATE = """
/*
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: %s],
[platform: 'windows', jdk: %s],
])
""".formatted(hightestCompatibleJdk, nextjdK);

@jonesbusy
Copy link
Collaborator

io.jenkins.tools.pluginmodernizer.SetupJenkinsfile is a declarative recipe

io.jenkins.tools.pluginmodernizer.core.AddJenkinsfile doesn't exist because it's the purpose of this PR..

@nagu165
Copy link
Contributor

nagu165 commented Jan 17, 2025

io.jenkins.tools.pluginmodernizer.SetupJenkinsfile is a declarative recipe

io.jenkins.tools.pluginmodernizer.core.AddJenkinsfile doesn't exist because it's the purpose of this PR..

Makes sense now
Thank you sir.

@nagu165
Copy link
Contributor

nagu165 commented Jan 17, 2025

Hello sir,
Do i need to make AddJenkinsfile as a seperate file or can we incorporate the logic into the recipe itself.

@jonesbusy
Copy link
Collaborator

Hello sir,
Do i need to make AddJenkinsfile as a seperate file or can we incorporate the logic into the recipe itself.

Try to determine which approach is better. How the recipe must be implemented? Is it Java ? Is is a standard Recipe or ScanningRecipe? Can we achieve it with declarative yaml only ? How are implemented other complex recipes? How this recipe can be tested?

I gave some tips and idea here : #598 (comment)

@nagu165
Copy link
Contributor

nagu165 commented Jan 20, 2025

Hello sir, I'm extremely sorry for asking this but could you please help with this one issue i have
I have implemented the recipe and tests for dynamically inserting the JDK versions found via pomresolutionvisitor as you have suggested, but when i run the tests
logs show that the Recipe successfully creates the JenkinsFile but the test is still failing, so i was thinking the tests might be wrong.
I will sincerely appreciate your input on the test
Test is shown below:

Image
Thank you so much.

@jonesbusy
Copy link
Collaborator

With just a screenshot and no logs or code it's just impossible to help.

I think the jenkins version bust be taken from the pom.xml from the scanner of the scanning recipe. Then the visitor creating the new file using correct values. Hope it helps

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

Successfully merging a pull request may close this issue.

3 participants