Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Mime types were not set by default, so enable the mime factory #150

Merged
merged 1 commit into from Aug 25, 2020

Conversation

halkeye
Copy link
Member

@halkeye halkeye commented Aug 25, 2020

jenkins.io has tighter restrictions on mime types so i didn't notice it on early testing

Before:

image

After:

image

@halkeye halkeye requested a review from a team as a code owner August 25, 2020 02:01
@martinda
Copy link
Contributor

Thanks @halkeye . Is this feature testable? Could a test be written for it?

@halkeye
Copy link
Member Author

halkeye commented Aug 25, 2020

I don't know. Java is not my strong suit. Is it possible to populate the public directory just for a test? Maybe somewhere in spring boot you could point public to a test directory?

@halkeye
Copy link
Member Author

halkeye commented Aug 25, 2020

Okay I did a bunch of searching. I have no idea how to set that up. Is other at-configuration or MVC factory stuff tested?

@halkeye
Copy link
Member Author

halkeye commented Aug 25, 2020

Reopening torerun

@halkeye halkeye closed this Aug 25, 2020
@halkeye halkeye reopened this Aug 25, 2020
Copy link
Contributor

@sladyn98 sladyn98 left a comment

Choose a reason for hiding this comment

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

@martinda @halkeye One solution is get this merged in and once the controller tests are also merged in we can check the mime type of the files returned in the tests, if we want to really test this.

import org.springframework.context.annotation.Configuration;

@Configuration
public class MimeMapping implements WebServerFactoryCustomizer<ConfigurableServletWebServerFactory> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor question should this be
public class ServletCustomizer implements WebServerFactoryCustomizer<TomcatServletWebServerFactory> since we are using a TomCat server as our default

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretend I know nothing. Cause I know nothing.

This solution seems to work but I make no promises. As the expert I defer to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

TomcatServletWebServerFactory is a subclass of ConfigurableServletWebServerFactory and would probably serve for more specific use cases, but this looks good enough.
CC @martinda @kwhetstone

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a net improvement, I added a issue to track testing later.

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

Successfully merging this pull request may close these issues.

None yet

3 participants