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

Minor build updates #90

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

Conversation

ywei2017
Copy link

A few minor touches -- using JDK 11 as default, graceful shutdown, and http healthcheck.

Thanks
Yansheng

- Enable Graceful shutdown, and use JDK 11 as default
@ywei2017
Copy link
Author

@scottfrederick , can you kindly give a view of the PR? Thanks.

@scottfrederick
Copy link
Member

@ywei2017 Thanks for the ideas. This PR includes several distinct unrelated changes, and I'm not inclined to accept them all as submitted.

What's the motivation for making Java 11 the default in manifest.yml? I'd prefer to let the platform determine the default JDK (via the Java Buildpack installed on the platform) and allow users to modify the default themselves instead of having this project override the platform default.

Likewise, it is better to let the platform choose the Java Buildpack to use instead of forcing java-buildpack-offline as in this PR, since java-buildpack-offline may not be available in all deployments of Cloud Foundry.

The gradle-wrapper.properties file change is unnecessary.

The health check and shutdown changes in manifest.yml look fine. I'm happy to merge those changes without the other suggestions.

@ywei2017
Copy link
Author

@scottfrederick, thanks for the quick response.

  • I have been reading a lot recently, concerning Java app behaviors in CF, and have come to the conclusion that Java 11 is much better suited for containerized workloads -- primarily concerning GC. I was thinking of reaching out to the Java Buildpack project proposing to change the default to Java 11, which will a long short. We can leave it as is (commented out)

  • I concede that the java-buildpack-offline is not universally valid, but I also see the inefficiency of autodetecting each time. How about if we include them, but leave them commented out? So anybody can enable it if they see fit?

  • I will remove the grade-wrapper.properties change.

If you are cool with the proposal, I will update the commit.

Thanks
Yansheng

@ywei2017
Copy link
Author

@scottfrederick I made some of the suggested changes, and commented out some of optional settings for the manifest.yml. Let me know how you think.

Thanks
Yansheng

@ywei2017
Copy link
Author

ywei2017 commented Jun 3, 2022

@scottfrederick - Is the PR still worthy of merging? If so, I will resolve the conflict. If not, I will delete it.

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

Successfully merging this pull request may close these issues.

2 participants