Java container test for challenges SUM and CHK

neomatrix369 commented 6 years ago

a skeleton test put in place based on the descriptions on https://gist.github.com/julianghionoiu/7d00a280ae1a8beeb60d9c929500a636
README in the /dpnt-coverage/container/README.md folder may not have all the details
was unable to run this instance on local machine due to the below (might be missing setup):
echo '~~ Clone and switch to TAG ~~'
LOCAL_REPO_DESTINATION=/srv/tdl-runner
[[ https://github.com/julianghionoiu/tdl-runner-java == s3://* ]]
echo 'Assuming Git repo'
git clone https://github.com/julianghionoiu/tdl-runner-java /srv/tdl-runner Cloning into '/srv/tdl-runner'... <==== cloning in the /srv/... folder is undesired, does not work well on local machines
local_git='git --git-dir=/srv/tdl-runner/.git --work-tree=/srv/tdl-runner'
git --git-dir=/srv/tdl-runner/.git --work-tree=/srv/tdl-runner checkout master Already on 'master'
git --git-dir=/srv/tdl-runner/.git --work-tree=/srv/tdl-runner pull origin master From https://github.com/julianghionoiu/tdl-runner-java
- branch master -> FETCH_HEAD
COVERAGE_SCRIPT=/srv/tdl-runner/get_coverage_for_challenge.sh
chmod a+x /srv/tdl-runner/get_coverage_for_challenge.sh
sed -i 's/\r//g' /srv/tdl-runner/get_coverage_for_challenge.sh
echo '~~ START run external script ~~'
/srv/tdl-runner/get_coverage_for_challenge.sh SUM
set -e
set -u
set -o pipefail +++ dirname /srv/tdl-runner/get_coverage_for_challenge.sh ++ cd /srv/tdl-runner ++ pwd
SCRIPT_CURRENT_DIR=/srv/tdl-runner
CHALLENGE_ID=SUM
JACOCO_TEST_REPORT_XML_FILE=/srv/tdl-runner/build/reports/jacoco/test/jacocoTestReport.xml
mkdir -p /srv/tdl-runner/target
JAVA_CODE_COVERAGE_INFO=/srv/tdl-runner/target/java-code-coverage.txt
./gradlew -q clean test jacocoTestReport /srv/tdl-runner/get_coverage_for_challenge.sh: line 15: ./gradlew: No such file or directory
true
'[' -e /srv/tdl-runner/target/java-code-coverage.txt ']'
'[' -f /srv/tdl-runner/build/reports/jacoco/test/jacocoTestReport.xml ']'
echo 'No coverage report was found'
exit -1 Test failed due to exit code mismatch Actual exit code: 255 Expected exit code: 0 Test failed due to result mismatch Actual result: 'Compute language specific name+version Running accelerate-io/dpnt-coverage-java:0.1 from the local docker registry Assuming Git repo Your branch is up-to-date with 'origin/master'. Already up-to-date. No coverage report was found' Expected result: '100'

docker java image is installing components that could have been installed in the base image (image building is taking a bit long):

Get:123 http://deb.debian.org/debian/ jessie/main libsisu-guice-java all 3.2.4-1 [614 kB]
Get:124 http://deb.debian.org/debian/ jessie/main libsisu-ioc-java all 2.3.0-5 [458 kB]
Get:125 http://deb.debian.org/debian/ jessie/main libaether-java all 1.13.1-2 [495 kB]
Get:126 http://deb.debian.org/debian/ jessie/main libjackrabbit-java all 2.3.6-1+deb8u2 [276 kB]
Get:127 http://deb.debian.org/debian/ jessie/main libjsch-agent-proxy-java all 0.0.7-1 [23.1 kB]
Get:128 http://deb.debian.org/debian/ jessie/main libmaven-parent-java all 21-2 [5744 B]
Get:129 http://deb.debian.org/debian/ jessie/main libwagon2-java all 2.7-1 [2021 kB]
Get:130 http://deb.debian.org/debian/ jessie/main maven all 3.0.5-3 [1281 kB]
Get:131 http://deb.debian.org/debian/ jessie/main libgradle-core-java all 1.5-2 [4000 kB]

neomatrix369 commented 6 years ago

Difference between makeLatest and buildDockerImage scripts and its purpose isn't clear.

julianghionoiu commented 6 years ago

To address your comments:

Failing Java container "./gradlew: No such file or directory"

It is a problem with the coverage script. It assumes that it will run in the same directory as gradlew. which is false. A principle that I follow when writing scripts is never to assume the current directory location. The reason for doing this is that the logs will contain each individual command including it's full path. It makes it easier to investigate a failure.

Action: We need to revisit the Java coverage script.

Java image is installing components

The language image should install components that are specific for the language. In this case: "gradle". The base image should contain everything needed for running itself and nothing more.

No action needed.

Difference between makeLatest and buildDockerImage scripts and its purpose isn't clear.

The latest tag is used by the local-ecs simulator to figure out which version of the container image to run. I think you are right, we will always want to tag the latest buildDockerImage.sh as latest.

Action: Add the tagging into buildDockerImage.sh and remove the makeLatest.sh script.

neomatrix369 commented 6 years ago

Btw this isn't correct in my view, inside the container its best to have folders under the user#s home folder - https://github.com/julianghionoiu/dpnt-coverage/blob/cff426a73fcc9b467351d01f69c0319604fd91ab/container/images/base/Dockerfile#L11

Which also brings me to the suggestion to use a root-less user inside the container.

Both of these are Docker best practices I do elsewhere and supported by other developers.

After a run last evening I saw a /srv/ folder in the root of my file-system, docker should never be able to do this, not sure how it is happening when I run the script.

neomatrix369 commented 6 years ago

https://github.com/julianghionoiu/dpnt-coverage/blob/master/container/images/java/Dockerfile and the scala counterpart might not work - we are missing the line that sets the default JDK to Java 8.

I can only check this bit after we fix the container issue from above (pointed out earlier)

julianghionoiu commented 6 years ago

I appreciate your suggestion and I understand why you would want to run a root-less user. I am currently racing against time so any unneeded extra complexity will slow us down.

neomatrix369 commented 6 years ago

Not sure if its additional complexity, although we had a working script previously that had it baked in, so we can continue to use it

julianghionoiu commented 6 years ago

It was getting in my way because I had to also worry about why user am I running against and how that impacts the language specific containers.

Less things to worry and the security benefits are not justified at this point.

On Fri, 25 May 2018 at 15:38 mani notifications@github.com wrote:

Not sure if its additional complexity, although we had a working script previously that had it baked in, so we can continue to use it

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/julianghionoiu/dpnt-coverage/pull/5#issuecomment-392078405, or mute the thread https://github.com/notifications/unsubscribe-auth/AE2qKQyFnY-rtuj0SN2bJYcw6WgFmurkks5t2BdUgaJpZM4UNGYj .

neomatrix369 commented 6 years ago

The refactor makes things look neater but I would have waited for one more or two additional instances before doing a full refactor, but we are here now, so I'll make use of it for the next language implementation.

neomatrix369 commented 6 years ago

https://github.com/julianghionoiu/dpnt-coverage/pull/5#issuecomment-392079333: Have you had successful runs with the new refactored version? Did all the programs run in the root context? Maybe we can revisit, as it might be important at some point. We are going to be running applications written by others in our infra - so its foreign code that could do anything. I'll leave it for you to decide in any case.

julianghionoiu commented 6 years ago

Would you also be able to add a container test for the hmmm language? https://github.com/julianghionoiu/tdl-runner-hmmm

On Fri, 25 May 2018 at 15:41 mani notifications@github.com wrote:

The refactor makes things look neater but I would have waited for one more or two additional instances before doing a full refactor, but we are here now, so I'll make use of it for the next language implementation.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/julianghionoiu/dpnt-coverage/pull/5#issuecomment-392079429, or mute the thread https://github.com/notifications/unsubscribe-auth/AE2qKXaLz1_lTHIH85KOSiC6Kx5u7pL-ks5t2BgQgaJpZM4UNGYj .

julianghionoiu commented 6 years ago

Yes. I am using the "hmmm" language in the Acceptance test and it is also deployed to my Dev cluster.

The Java and Scala containers might need some tweaking.

On Fri, 25 May 2018 at 15:43 Iulian Ghionoiu iuli.constantin.ro@gmail.com wrote:

Would you also be able to add a container test for the hmmm language? https://github.com/julianghionoiu/tdl-runner-hmmm

On Fri, 25 May 2018 at 15:41 mani notifications@github.com wrote:

The refactor makes things look neater but I would have waited for one more or two additional instances before doing a full refactor, but we are here now, so I'll make use of it for the next language implementation.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/julianghionoiu/dpnt-coverage/pull/5#issuecomment-392079429, or mute the thread https://github.com/notifications/unsubscribe-auth/AE2qKXaLz1_lTHIH85KOSiC6Kx5u7pL-ks5t2BgQgaJpZM4UNGYj .

neomatrix369 commented 6 years ago

Is the structure of the current test layout correct? I wasnt sure till I raised the PR

julianghionoiu commented 6 years ago

Current test layout looks good.

The "hmmm" acceptance test can use two tags: TCH_R1/done - 33% coverage TCH_R2/done - 44% coverage

https://github.com/julianghionoiu/tdl-runner-hmmm https://github.com/julianghionoiu/tdl-runner-hmmm

On Fri, 25 May 2018 at 15:44 Iulian Ghionoiu iuli.constantin.ro@gmail.com wrote:

Yes. I am using the "hmmm" language in the Acceptance test and it is also deployed to my Dev cluster.

The Java and Scala containers might need some tweaking.

On Fri, 25 May 2018 at 15:43 Iulian Ghionoiu iuli.constantin.ro@gmail.com wrote:

Would you also be able to add a container test for the hmmm language? https://github.com/julianghionoiu/tdl-runner-hmmm

On Fri, 25 May 2018 at 15:41 mani notifications@github.com wrote:

The refactor makes things look neater but I would have waited for one more or two additional instances before doing a full refactor, but we are here now, so I'll make use of it for the next language implementation.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/julianghionoiu/dpnt-coverage/pull/5#issuecomment-392079429, or mute the thread https://github.com/notifications/unsubscribe-auth/AE2qKXaLz1_lTHIH85KOSiC6Kx5u7pL-ks5t2BgQgaJpZM4UNGYj .

neomatrix369 commented 6 years ago

https://github.com/julianghionoiu/dpnt-coverage/pull/5#issuecomment-391975542: Just looking at this, previously this worked fine, and is now broken due to the refactorings that took place.

neomatrix369 commented 6 years ago

https://github.com/julianghionoiu/dpnt-coverage/pull/5#issuecomment-392199174: now fixed, applied suggestion from https://github.com/julianghionoiu/dpnt-coverage/pull/5#issuecomment-391975542

julianghionoiu / dpnt-coverage

Java container test for challenges SUM and CHK #5