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

GEODE-7107: Make jetty-server optional in geode-core pom #3968

Conversation

jdeppe-pivotal
Copy link
Contributor

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

@prettyClouds
Copy link
Contributor

Can you share the why behind this change for some context? Under what circumstances would this jar not be included?

@upthewaterspout
Copy link
Contributor

I share @prettyClouds concerns - how do we know this is really optional? Are we testing that somehow? IMO we should be getting rid of optional dependencies in geode-core in favor of having separate geode modules with the dependencies - like we've done with geode-lucene, geode-redis, etc.

@jdeppe-pivotal
Copy link
Contributor Author

The dependency on jetty-server also pulls in a transitive dependency on javax-servlet-api. When running in a Spring Boot context, the presence of the servlet api implies that the application is a web application which will attempt to launch an embedded Tomcat server. This may not be what the user intended.

@jdeppe-pivotal jdeppe-pivotal merged commit d853adf into apache:develop Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants