-
Notifications
You must be signed in to change notification settings - Fork 84
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
Change gc log option for java11 #217
Conversation
#216 can test this - do you want to try running that against this? |
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.
There is a bigger small 😈 problem here.. the java/images/*/run-java.sh
files are generated, so you cannot change them here in this project with a PR like this (if we did merge this, we would loose it again very soon when re-generating all those files via fish-pepper
, as ./test.sh
also does) ... as noted here, quote: we need a similar PR to this but instead against run-java-sh; then @rhuss will need to do a new release of run-java-sh and a bump the version to it here in this project (like in #214), and then a rebase #216 which should then pass. Hope this makes sense?
Hmm, so the logic added here does not work unless the user sets |
Okay since I enjoy a good bush whacking, I wrote the following snippet that I think could work to grab the
Again, I'm not sure I enjoy having to execute the java binary to get the version, but I can't think of a more robust way to do so. IMO it would make more sense to ship different versions of run-java.sh with the different docker images. |
@muff1nman your approach looks fine to me. Let me briefly explain the motivation behind https://github.com/rhuss/run-java-sh :
So it would be perfectly fine to support a JAVA_MAJOR_VERSION set from the outside of the script (e.g. when using in this image we can set this env var accordingly to this image), and possibly fallback to autodetection if not set. wdyt, does this make sense ? |
So you would propose that an |
yup, I think that's exactly what @rhuss is suggesting - and I would agree with him that just from a "performance of start-up time" point of view that would be much better than to have to run
or may be you could even ditch that idea completely? It would seem OK to me if |
Closing in favor of #219. @vorburger @rhuss I like the idea of aborting if JAVA_MAJOR_VERSION isn't set to be defensive, but decided against it to be consistent with the other places it is used in run-java-sh. |
Addresses #213. Not yet tested.