-
Notifications
You must be signed in to change notification settings - Fork 126
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-73260] Forward compatibility with EE 9 cores #770
Conversation
62922fb
to
3955c1f
Compare
51030cd
to
ae20325
Compare
5aeb382
to
00bd915
Compare
03cf5e3
to
333d317
Compare
@@ -583,6 +657,52 @@ protected ClassLoader configureClassLoader(ClassLoader loader) { | |||
return context.getServletContext(); | |||
} | |||
|
|||
/** |
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.
Not new code, so please only review the diff of this method with the createWebServer2
method (left as an exercise for the reader).
* | ||
* @author Kohsuke Kawaguchi | ||
*/ | ||
public class JavaNetReverseProxy2 extends HttpServlet { |
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.
Not new code, so please only review the diff of this class with the JavaNetReverseProxy
class (left as an exercise for the reader).
* | ||
* @author Kohsuke Kawaguchi | ||
*/ | ||
public class NoListenerConfiguration2 extends AbstractLifeCycle { |
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.
Not new code, so please only review the diff of this class with the NoListenerConfiguration
class (left as an exercise for the reader).
@@ -895,6 +973,130 @@ protected ClassLoader configureClassLoader(ClassLoader loader) { | |||
return context; | |||
} | |||
|
|||
/** |
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.
Not new code, so please only review the diff of these methods with the "2" versions above (left as an exercise for the reader).
@@ -1763,7 +1883,7 @@ public String getContextPath() throws IOException { | |||
public WebRequest addCrumb(WebRequest req) { | |||
NameValuePair crumb = new NameValuePair( | |||
jenkins.getCrumbIssuer().getDescriptor().getCrumbRequestField(), | |||
jenkins.getCrumbIssuer().getCrumb( null )); | |||
jenkins.getCrumbIssuer().getCrumb((javax.servlet.ServletRequest) null)); |
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.
Here and elsewhere, newer cores will have two versions of this method, one for javax
and one for jakarta
, so to be forward compatible we have to disambiguate the method call. Here we choose the older import, since this change does have to work on non-Jakarta cores.
Problem
We plan to eventually migrate from Jetty 12 EE 8 to Jetty 12 EE 9, but the test harness only supports cores with Jetty 12 EE 8.
Solution
Bundle both the Jetty 12 EE 8 and EE 9 modules into the test harness, and dynamically select the module to load at runtime based on the Jenkins core in use.
This change does not increase the minimum core version supported by the test harness. To detect newer cores with EE 9 support and invoke the appropriate methods, reflection is used.
Implementation
This plugin finds any places where we invoked EE 8 functionality in Jenkins core and turns them into an
if
statement. The existing code is in theelse
block, and theif
block contains new code that invokes the relevant EE 9 functionality through reflection.JavaNetReverseProxy2
andNoListenerConfiguration2
classes have been created that duplicate the existing classes but with EE 9 imports instead.Testing done
Core and plugin test suites have been executed successfully with this PR, with no known failures.
Notes for reviewers
This change is somewhat challenging to review. The changes to
JenkinsRule
andHudsonTestCase
look confusing, because the imports have changed, and it looks like a lot of new code has been added, but that new code is really preserving the existing code but with the old imports. The "existing" code has now been changed to add a 2 to the method names and referring to the new imports. So the existing code looks the same but means something different, and the "new" code is actually the existing code, but for compatibility.Deployment
This change requires Java 17 or newer, so it will not be backported to the 2225.x branch of the test harness or released in a plugin parent POM until such a time that we require Java 17 or newer for Jenkins plugin development.
It will be used in the core release that adopts Jetty 12 EE 9.
Future work
Once we drop support for EE 8 in the test harness, the code in this change can be simplified. The EE 8 branches of
if
statements can all be deleted, and the reflection-based code can be transformed to directly invoke the relevant methods, much like the EE 8 code does today.