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
Update dependencies to migrate from Java 8 to 11 #87
Update dependencies to migrate from Java 8 to 11 #87
Conversation
01264f5
to
d2c0e13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you don't need my review to add changes.
I added some comments that might help you, but you can decide what to do as you like.
</dependency> | ||
<dependency> | ||
<groupId>org.jenkins-ci.plugins</groupId> | ||
<artifactId>token-macro</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
token-macro is declared twice.
@@ -31,13 +31,72 @@ | |||
</scm> | |||
|
|||
<properties> | |||
<jenkins.version>2.222.1</jenkins.version> | |||
<java.level>8</java.level> | |||
<!-- Dependency org.jenkins-ci.plugins:mailer:jar:412.v7deeda_155287 requires Jenkins 2.331 or higher. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds strange to me that mailer-plugin requires so newer core.
- The latest stable mailer-plugin looks 408.vd726a_1130320 and requires 2.289.1.
- What requires mailer-plugin? I believe mailer-plugin isn't used from build-timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend target core is described here: https://www.jenkins.io/doc/developer/plugin-development/choosing-jenkins-baseline/
<groupId>org.jenkins-ci.plugins</groupId> | ||
<artifactId>script-security</artifactId> | ||
<version>1.78</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might just want to resolve RequireUpperBoundDeps
.
It's not good way to add explicit dependencies to resolve that.
Following documents may help you:
- https://www.jenkins.io/doc/developer/plugin-development/dependency-management/
- https://github.com/jenkins-infra/docker-confluence-data/blob/a336b7af88e56d32e11fce0aac1f966f74e279c5/content/JENKINS/How-to-fix-RequireUpperBoundDeps.html
Targetting LTS core and using BOM is the easiest way.
Thanks for your review @ikedam! Appreciate it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe most of it should be in Plugin POM.
To some extent jenkinsci/plugin-pom#133 which needs to be updated
Yeah @oleg-nenashev completely agreeing with you there. Maybe I should open a PR there before making changes to this PR reviewing it again. Should definitely properly be upstream changes. Do you think it is a good idea? |
I noticed jenkinsci/plugin-pom#478 might be more relevant... But I think I could polish up this PR then keep it open for myself and other contributors to rebase against any new PR can run smoothly. Looks like a consensus is still out of reach within the Jenkins community until the dropping of support for Java 8 is confirmed. Hopefully that will happen quite soon. So two options now:
|
d2c0e13
to
f7678db
Compare
I believe that the best way forward is to merge this PR first, so that I can start giving #81 a proper review. Otherwise the plugin as is cannot be built and pass all the tests on my laptop. So once the redundant dependency is removed and all the CI/CD tests are passed again I will merge this ASAP. I will keep a backup of the |
Thanks for the comments and review @oleg-nenashev! |
f7678db
to
dbe4dc1
Compare
Just noticed this PR :) Seems like this plugin could benefit from the plugin BOM. Making dependency management easier. https://github.com/jenkinsci/bom#usage A lot of work goes into compatibility checking 😅 See a recent plugin where I added plugin BOM: jenkinsci/stashnotifier-plugin#279 |
Thanks @jetersen! I will try it out soon. |
@krisstern I'm surprised that you're requiring Java 11 in the plugin before Jenkins requires Java 11. Are you intentionally excluding Jenkins users that are running Java 8? That will prevent roughly half of the current 225 000 installations of this plugin from using the new release. |
Hi @MarkEWaite My apologies, wouldn't be so rash next time |
Thanks for the reply @krisstern ! I should have asked the broader questions related to what I was trying to understand. Is there something in the development of the plugin that is motivating you to switch to Java 11? If so, I'm wondering if that same motivation will be important to other plugin maintainers. I am also curious how the Jenkins update center and plugin manager will present this plugin upgrade to users. Will it hide it from users that are running Jenkins 2.337 on Java 8 or will it be listed as released but not allowed for this Jenkins installation or will it be listed as released and available but then won't be able to run when downloaded? I may need a discussion with the experts in Jenkins update center like @daniel-beck to understand how the update center will behave as we transition to eventually require Java 11 in Jenkins core. |
@MarkEWaite I was thinking I will need to delay until the official transition to Java 11 before cutting a new release, so the damage would not be as great |
Thanks for the clarification. When Jenkins core requires Java 11 (version not yet determined, much work to be done to prepare for it), then I expect that plugins that depend on that Jenkins version will need to compile with Java 11. |
Yeah @MarkEWaite as there are some new features we would like to add before the official transition, it seemed like a good idea to switch to Java 11 now so that we can take the time to develop/review these features in preparation for the transition |
Setting |
Description
Fixes #86, plus patches the existing issues and vulnerabilities to make the upgrade work via the following:
https://repo.jenkins-ci.org/incrementals
forincrementals
downloads. (Also to patch some vulnerability issues.)Additionally, both the commands
mvn verfiy
thenmvn hpi:run
have been run sequentially and vulnerabilities checked to make sure everything runs smoothly to the best of one's knowledge. No new tests have been added due to the "upgrading" nature of this PR, as only thepom.xml
file, theJenkinsfile
, and theCHANGELOG.adoc
file at the root have been changed.Rationale for the upgrade can be found in Issue #86, with links to source documentations there.
Checklist