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

Implemented a small version for issue#688 #702

Closed
wants to merge 22 commits into from

Conversation

nagu165
Copy link
Contributor

@nagu165 nagu165 commented Jan 24, 2025

updated updatecli.d/jenkins-lts.yaml to update the jenkins recommended version

Testing done

Added 2 more targets in the updatecli.d/jenkins-lts.yaml to update the jenkins versions in recipes.yml
With the further input from the maintainers would like to work on it further.

Submitter checklist

  • [x ] Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • [x ] Ensure that the pull request title represents the desired changelog entry
  • [x ] Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@nagu165 nagu165 requested a review from jonesbusy as a code owner January 24, 2025 17:28
@nagu165
Copy link
Contributor Author

nagu165 commented Jan 24, 2025

Hello sir, Just wanted your input regarding these changes, would like to work on this further.
Please do tell me if i'm missing something.
Thank you.

@nagu165 nagu165 marked this pull request as draft January 24, 2025 17:30
@gounthar
Copy link
Collaborator

Changing it there is risky, as we might use the same keyword in different situations. I prefer to define a property in versions.property, keep it updated via updatecli, and then define a variable to use this property.

Thanks!

@jonesbusy
Copy link
Collaborator

What if we use a comment to match the version? "#recommanded version managed by updatecli"?

But I think a maven property with filtering on the resource looks more clean

@nagu165
Copy link
Contributor Author

nagu165 commented Jan 26, 2025

Hello sir, As per your suggestion i'm trying to use maven filtering on the recipes.yml to dynamically update the version but filtering isn't working as expected, Please share any info on why this is so.
Thank you

@@ -19,4 +19,5 @@ private RecipesConsts() {
public static final String OLD_BOM_VERSION_PATTERN =
""; // Old release line like 2.150.x, no need to filter by pattern
public static final String INCREMENTAL_REPO_ID = "incrementals";
public static final String JENKINS_CORE_VERSION = "${jenkins.core.minimum.version}";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not Settings.getJenkinsMinimumVersion() instead of filtering java source?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry sir, I missed it.
updating now.
Thank you.

@@ -148,7 +148,7 @@ recipeList:
artifactId: plugin
newVersion: 5.X
- io.jenkins.tools.pluginmodernizer.core.recipes.UpgradeJenkinsVersion:
minimumVersion: 2.479.1
minimumVersion: "${jenkins.core.minimum.version}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the minimum version. This value should not be changed since it's the first LTS supporting Java 17 and required for parent 5.x upgrade.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the example that sir gave i.e #685 this was also modified so i was testing with this.
Now i understand the issue.
Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only 2.452.4 was incremented to 2.462.3. The rest of value must not be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sir, I misunderstood previously
Now i understand my mistake
Thank you.

@@ -442,7 +442,7 @@ recipeList:
artifactId: plugin
newVersion: 4.51 # See https://www.jenkins.io/blog/2022/12/14/require-java-11/
- io.jenkins.tools.pluginmodernizer.core.recipes.UpgradeJenkinsVersion:
minimumVersion: 2.346.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is NOT the minimum version. It's the last LTS supporting Java 8. This value will never changes

<directory>src/main/resources/META-INF/rewrite</directory>
<includes>
<include>recipes.yml</include>
</includes>
</resource>
<resource>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be removed

@jonesbusy
Copy link
Collaborator

Hello sir, As per your suggestion i'm trying to use maven filtering on the recipes.yml to dynamically update the version but filtering isn't working as expected, Please share any info on why this is so.
Thank you

I do not know if you don't share details

@@ -416,7 +416,7 @@ recipeList:
- io.jenkins.tools.pluginmodernizer.UpgradeParentVersion
- io.jenkins.tools.pluginmodernizer.AddPluginsBom
- io.jenkins.tools.pluginmodernizer.core.recipes.UpgradeJenkinsVersion:
minimumVersion: 2.462.3
minimumVersion: "${jenkins.core.minimum.version}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see it in the pom

@nagu165
Copy link
Contributor Author

nagu165 commented Jan 27, 2025

Hello sir, in versions.properties - openrewrite.maven.plugin.version = ${openrewrite.maven.plugin.version} is causing circular refernce error so i replaced it with a constant for now, How to modify it properly without any breaking changes.
Thank you.

@nagu165
Copy link
Contributor Author

nagu165 commented Jan 28, 2025

Is this change okay?

@jonesbusy
Copy link
Collaborator

Tests are failing

@@ -1,4 +1,4 @@
openrewrite.maven.plugin.version = ${openrewrite.maven.plugin.version}
openrewrite.maven.plugin.version = 5.10.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. We need to continue filtering from the pom

@@ -227,6 +227,26 @@
</resources>
<plugins>

<plugin>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason to override the properties-maven-plugin rather than adding a new ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But i did add a unique execution id so, won't it run concurrently instead of overriding?

@nagu165
Copy link
Contributor Author

nagu165 commented Jan 30, 2025

Hello sir, The tests are failing on linux
Maybe because of the change intoduced in the pom, do i need to modify the test cases in CommandLineITCase.java or is there another approach as i'm stuck troubleshooting this and couldn't find a better solution yet.
Thank you.

@nagu165
Copy link
Contributor Author

nagu165 commented Jan 31, 2025

Hello sir, Some errors seems to be introduced recently, tests are failing in "UpgradeJenkinsVersionTest", could you please take a look at that.

@jonesbusy jonesbusy mentioned this pull request Feb 6, 2025
6 tasks
@jonesbusy
Copy link
Collaborator

Superseded by #776

@jonesbusy jonesbusy closed this Feb 6, 2025
@nagu165 nagu165 deleted the issue#688 branch February 6, 2025 05:59
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

Successfully merging this pull request may close these issues.

3 participants