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

[JENKINS-73137] Adapt Jira plugin for jetty 12 EE8 #613

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

BorisYaoA
Copy link

@BorisYaoA BorisYaoA commented Aug 20, 2024

see https://issues.jenkins.io/browse/JENKINS-73137

The goal of this PR is to adapt Jira-plugin for jetty 12 (EE8).
The most of the work is about adapting ProxyTestHandler and TestHandler classes to use Handler.Abstract from jetty12

🚨 Please review the guidelines for contributing to this repository.

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

@BorisYaoA BorisYaoA closed this Aug 21, 2024
@BorisYaoA BorisYaoA reopened this Aug 29, 2024
@BorisYaoA BorisYaoA changed the title [JENKINS-73137] Adapt Jira plugin for jetty 12 ee8 [JENKINS-73137] Adapt Jira plugin for jetty 12 EE8 Aug 30, 2024
pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

I have replayed the CI build with the Jenkinsfile changes here: https://ci.jenkins.io/job/Plugins/job/jira-plugin/job/PR-613/10/

For the benefit of the plugin maintainers, I wanted to explain why this PR raises the baseline to a weekly release version, which is atypical as described in https://www.jenkins.io/doc/developer/plugin-development/choosing-jenkins-baseline and elsewhere. We generally try to preserve compatibility as much as possible, but in this case the incompatible change was made upstream in Jetty. We generally try to test with the same version of Jetty as is used in production. As of recent weeklies, that is Jetty 12, but Jetty 12 requires compilation changes to tests.

While this change could be postponed in order to keep the baseline low, it needs to be made eventually. It might be simplest to get it out of the way now to prevent bitrot and improve development efficiency. It does not have to be released right away. It could be released in a few months, once Jetty 12 is in LTS.

@BorisYaoA BorisYaoA marked this pull request as ready for review August 30, 2024 18:05
@BorisYaoA BorisYaoA requested a review from a team as a code owner August 30, 2024 18:05
HttpServletRequest request,
HttpServletResponse response)
throws IOException, ServletException {
public boolean handle(
Copy link
Member

Choose a reason for hiding this comment

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

changes looks good.
But I wonder if it would be better to simply use Servlet API filter to have some implementation of this not depending on Jetty core API (which may change soon again for 12.1.x)

@@ -1,7 +1,6 @@
// Builds a module using https://github.com/jenkins-infra/pipeline-library
buildPlugin(useContainerAgent: true, configurations: [
[platform: 'linux', jdk: 11],
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not dropping JDK11 until there is an LTS release that is no longer supporting Java11.

<!-- jenkins -->
<jenkins.version>2.440.3</jenkins.version>
<!-- TODO: Remove when 2.472 is available in LTS -->
<jenkins.version>2.472</jenkins.version>
Copy link
Contributor

@rantoniuk rantoniuk Aug 31, 2024

Choose a reason for hiding this comment

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

We are supporting only LTS line, so let's make this PR a draft until there is an LTS released >=2.472
https://www.jenkins.io/changelog-stable/

<spotless.check.skip>false</spotless.check.skip>
<maven.compiler.release>17</maven.compiler.release>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is/should be coming from parent-pom, we don't want an override here

Copy link
Member

Choose a reason for hiding this comment

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

@rantoniuk There is not yet a release of plugin parent POM that requires Java 17, but there will be one around the October timeframe, at which point this temporary code can be deleted. We aren't releasing a plugin parent POM that requires Java 17 right now because most of the ecosystem is still targeting Jenkins core releases that support Java 11.

@basil
Copy link
Member

basil commented Aug 31, 2024

Hi @rantoniuk, that would imply that this PR would need to sit around for a few more months before it could be merged, by which point the staffing for the current initiative would have ended, and the author of the PR may not necessarily be available to rebase the PR, deal with merge conflicts, address review feedback, etc. So I think there are two options:

  • Accept this PR now in its current form, but delay a release of the Jira plugin until the October LTS release of Jenkins is available. This takes advantage of current staffing and the current state of this PR (passing CI build, no conflicts, etc)
  • Block this PR and accept that since there may not be staffing to complete it later you will need to complete the work later yourself

@rantoniuk
Copy link
Contributor

Accept this PR now in its current form, but delay a release of the Jira plugin until the October LTS release of Jenkins is available. This takes advantage of current staffing and the current state of this PR (passing CI build, no conflicts, etc)

That's not an option, since this would conflict with the agreed LTS-only support policy. If we would merge this PR today, someone would need to do another PR later to fix/revert the non-LTS compatible changes - and that's why I would prefert this stays as a completed draft (i.e. green build) as it will easier to pick it up and update only the properties accordingly when new LTS line is available.

@basil
Copy link
Member

basil commented Aug 31, 2024

That's not an option

Understood, @rantoniuk. Does that imply you are accepting the second option; namely, to complete this work yourself at such a time as it is available in LTS?

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

Successfully merging this pull request may close these issues.

5 participants