-
Notifications
You must be signed in to change notification settings - Fork 76
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-73126] Upgrade Winstone from Jetty 10.x to 12.x (EE 8) #383
Conversation
31a91d5
to
a07086b
Compare
01a7059
to
e8a0d5a
Compare
1a58a64
to
2ac4f2e
Compare
c86a891
to
fc72a87
Compare
8f9940a
to
760fa67
Compare
abde36f
to
4ac932a
Compare
Co-authored-by: Olivier Lamy <[email protected]>
@@ -55,7 +52,6 @@ public class HostConfiguration { | |||
private Map<String, String> args; | |||
private Map<String, WebAppContext> webapps; | |||
private ClassLoader commonLibCL; | |||
private MimeTypes mimeTypes = new MimeTypes(); |
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.
Note that we are no longer replacing the default MIME types, but appending to them. As a result, we have deleted any MIME mappings that are already present in upstream Jetty and added a test to ensure that we are only adding to the existing MIME types rather than attempting to replace them.
public ErrorCollector errors = new ErrorCollector(); | ||
|
||
@Test | ||
public void mimeTypes() throws IOException { |
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.
This test ensures that we are only ever adding to the Jetty default MIME types, never replacing them. When upstream Jetty has a MIME type, we want to use it whenever possible. This is not only so that we have less code to maintain, but also so that we avoid bugs like JENKINS-73381.
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.
Looks great.
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.
Security-wise, LGTM
Replace dependencies with EE 8 versions in
pom.xml
and update all relevant imports.Testing done
Full PCT/ATH