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

Add JAVA_MAJOR_VERSION #219

Merged
merged 1 commit into from
Mar 19, 2019

Conversation

muff1nman
Copy link
Contributor

In combination with fabric8io-images/run-java-sh#78, this should resolve #213.

@vorburger
Copy link
Collaborator

vorburger commented Feb 4, 2019

@muff1nman I think this is going in the right direction and I'm in favour of this, but:

Similarly to the previous discussion over in #217, you cannot (only) change the java/images/**/Dockerfile ... if you run the ./test.sh at the root which runs fish-pepper then you'll see that the changes you propose here would just get overwritten (soon) and lost again.

Instead, change the s2i/java/templates/Dockerfile and use a variable you set in s2i/java/images.yml.

Perhaps you'll just want to use the already existing (which I introduced...) javaMajor ? But that's used to construct the package name install, and is 1.8.0 not 8... if that's NOK for you then you could also just introduce another variable. If you can re-use the same it's probably better though.

Does this make sense and do you think you can do this?

@vorburger vorburger self-requested a review February 4, 2019 17:36
Copy link
Collaborator

@vorburger vorburger left a comment

Choose a reason for hiding this comment

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

see comments above for why this needs some adjustments until we can (and want to!) merge it

@rhuss
Copy link
Contributor

rhuss commented Feb 6, 2019

I agree with @vorburger . Installing fish-pepper might be a pain, so I recommend to run it from within a Docker container like in

docker run -it --rm -v $(pwd):/fp fabric8/fish-pepper

(of course you need to able to use volume mounts)

See also https://github.com/fabric8io-images/fish-pepper/tree/master/docker

@muff1nman
Copy link
Contributor Author

@vorburger Ended up going with a second variable to be explicit.

@vorburger
Copy link
Collaborator

@vorburger Ended up going with a second variable to be explicit.

OK for me... in an ideal world, you would also contribute an extension to test.sh to cover for future non-regression of #213 .. do you think you may have time to add that here?

In combination with fabric8io-images/run-java-sh#78, this should resolve #213.

but don't you have to bump the version of run-java-sh in java/fish-pepper.yml as part of this PR now? Or is fabric8io-images/run-java-sh#78 in 1.3.0 already? FYI v1.3.1 is broken, watch #123 .. we'll probably have a 1.3.2.

@vorburger
Copy link
Collaborator

@muff1nman do you want to finish this one up? I think following #123 which bumped run-java-sh you just have to debase this on the latest master now, and then it will work..

@muff1nman
Copy link
Contributor Author

@vorburger Rebased. I tested it and after increasing the app_test sleep time it worked. Probably a slightly starved VM and/or older cpu with not very good single thread perfomance. So I think this is good to go.

Signed-off-by: Andrew DeMaria <[email protected]>
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.

JAVA_DIAGNOSTICS for s2i java 11 image uses deprecated and removed JVM options
3 participants