search

jenkinsci / build-timeout-plugin

Jenkins build-timeout plugin
https://plugins.jenkins.io/build-timeout/
31 stars 80 forks source link

Update dependencies to migrate from Java 8 to 11 #87

Closed krisstern closed 2 years ago

krisstern commented 2 years ago

Description

Fixes #86, plus patches the existing issues and vulnerabilities to make the upgrade work via the following:

  1. Bumped Jenkins version to 2.333 for security reasons. (Needed a version that is >= 2.331 for a plugin. Otherwise there would be some vulnerability issues remained.)
  2. Added minimum Java version to be 11, instead of the previous version 8.
  3. Added dependencies to make the transition from Java 8 to 11 to switch the JVM.
  4. Added plugin configurations for the upgrade while enabling the tests to be passed.
  5. Added https://repo.jenkins-ci.org/incrementals for incrementals downloads. (Also to patch some vulnerability issues.)

Additionally, both the commands mvn verfiy then mvn 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 the pom.xml file, the Jenkinsfile, and the CHANGELOG.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

krisstern commented 2 years ago

Thanks for your review @ikedam! Appreciate it

krisstern commented 2 years ago

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?

krisstern commented 2 years ago

I noticed https://github.com/jenkinsci/plugin-pom/pull/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:

  1. Keep this PR open and modify it later once drop of support of Java 8 is confirmed
  2. Merge this PR now and make the necessary changes once upstream changes are made
krisstern commented 2 years ago

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 pom.xml as a reference and revert some dependency updates where applicable once some upstream changes take place.

krisstern commented 2 years ago

Thanks for the comments and review @oleg-nenashev!

jetersen commented 2 years ago

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: https://github.com/jenkinsci/stashnotifier-plugin/pull/279

krisstern commented 2 years ago

Thanks @jetersen! I will try it out soon.

MarkEWaite commented 2 years ago

@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.

krisstern commented 2 years ago

Hi @MarkEWaite My apologies, wouldn't be so rash next time

MarkEWaite commented 2 years ago

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.

krisstern commented 2 years ago

@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

MarkEWaite commented 2 years ago

@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.

krisstern commented 2 years ago

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

basil commented 2 years ago

Setting <release>11</release> was premature and will break this plugin on Java 8 installations, which are still supported. Please see #90 for a correction.