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

Define addJava in ash-template #942

Closed
wants to merge 1 commit into from

Conversation

mattroberts297
Copy link

Previously, there was no addJava function defined in the ash-template.
This resulted in warnings when the ash script ran because template
declares expects the function to be defined.

Now, the addJava function is defined. It adds options to java_opts. As a
convenience, if JAVA_OPTS is set then it is added to java_opts. This
makes the ash script behave more like the bash script. The result is
then passed to java at the end of the script.

Previously, there was no addJava function defined in the ash-template.
This resulted in warnings when the ash script ran because template
declares expects the function to be defined.

Now, the addJava function is defined. It adds options to java_opts. As a
convenience, if JAVA_OPTS is set then it is added to java_opts. This
makes the ash script behave more like the bash script. The result is
then passed to java at the end of the script.
@lightbend-cla-validator

Hi @mattroberts297,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@mattroberts297
Copy link
Author

I tested the behaviour using this dockerised play app:

git clone [email protected]:mattroberts297/play-minimal-2.5.git
cd play-docker-minimal
git checkout play-docker-minimal
sbt docker:publishLocal
docker run --name play-docker-minimal -p 9000:9000 -e PLAY_CRYPTO_SECRET="pVj5VOLpp/dpexXHGDMjSPDiBPCk3oZ0RQ1hOrs0c45TFFsHDXGyGUBWPG3t+rpYojpnKPN4IS0ZXDmlwVlfPw==" -e JAVA_OPTS="-Xmx1g" -d play-docker-minimal:0.1.0-SNAPSHOT
docker exec -i -t play-docker-minimal /bin/sh
/opt/docker $ ps -ef | grep java
    1 daemon     0:21 java -Xmx1g

@mattroberts297
Copy link
Author

CLA signed.

@kardapoltsev
Copy link
Member

LGTM.

Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

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

Thanks @mattroberts297 for your PR.

The changes are looking good. Could you add a small test case in sbt-test/ash that demonstrates this is working.

You can run the tests with sbt "scripted ash/*". For more information we have a Developer Guide

@muuki88
Copy link
Contributor

muuki88 commented Feb 15, 2017

Thanks for taking a look @kardapoltsev . You can add a review by going to the Files changed tab and use the Review changes button. At least one maintainer has to accept a PR so it can be merged without admin rights.

@muuki88
Copy link
Contributor

muuki88 commented Feb 16, 2017

@mattroberts297 , if you have no time or experience with writing a test, it's okay. We can take over. The change is necessary and good, so I'll would like to merge this. However we try to cover as much as possible with tests.

@muuki88
Copy link
Contributor

muuki88 commented Feb 19, 2017

I added a test and opend a new PR #944

@muuki88 muuki88 closed this Feb 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants