Removing throws Exception. Fixes #194 #200
Conversation
There was a problem hiding this comment.
@honza-kasik thanks, I have just a few tips to improve. There is a lot of code for exception handling but I understand the motivations.
|
|
||
| public void start() throws Exception { | ||
| this.outputPrintingThread = startPrinterThread(dockerRunProcess, this.out, this.name); |
There was a problem hiding this comment.
what about to put this into finally {...} block to log output from starting process in case of fail.
There was a problem hiding this comment.
what do you think about this one?
| public boolean isRunning() { | ||
| Process dockerRunProcess; | ||
| try { | ||
| dockerRunProcess = new ProcessBuilder() |
There was a problem hiding this comment.
I think that runDockerCommand() might be used here as well.
| Process dockerInfoProcess = new ProcessBuilder() | ||
| .redirectErrorStream(true) | ||
| .command(new String[] { "docker", "info" }) | ||
| .start(); |
There was a problem hiding this comment.
I think that runDockerCommand() might be used here as well.
|
@honza-kasik do you think that you might take a look at the test failures, I think they might be related to this PR Edit: I've found the issue there is missing |
|
@mnovak1 Exactly. I removed this calls because it didn't make sense to me. The problem is that the docker start command actually creates and starts container. Adding calls to these back to methods is IMHO a bad idea. I'll try to solve this... |
honza-kasik
force-pushed
the
docker-improv
branch
from
7ac5967
to
dc87210
Compare
honza-kasik
force-pushed
the
docker-improv
branch
from
dc87210
to
8b29e85
Compare
|
Hopefully all fixed. Job is running https://eap-qe-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/job/eap-7.x-microprofile-testsuite/98/ |
|
@mnovak1 All green |
| .command(cmd) | ||
| .start(); | ||
|
|
||
| process.waitFor(10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
I think this should be removed here and leave to handle Process on calling method.
| @@ -205,6 +205,7 @@ public void dropDatabaseSchema() { | |||
| public static void tearDown() throws Exception { | |||
| // stop DB if still running and delete data directory | |||
| postgresDB.stop(); | |||
| postgresDB.remove(); | |||
There was a problem hiding this comment.
I think remove() should be called from stop() and kill() medhod...as this might be source of failures.
There was a problem hiding this comment.
The issue I see here is that the method called stop removes a container. That is IMHO not desired. What I have been thinking about is implementing Closable/AutoClosable interface to have a close method. Which would care about cleanup when the container is no longer needed. WDYT?
There was a problem hiding this comment.
I've found that there can be used --rm option in docker run... which causes that container is automatically removed when stopped (or killed) what about to use it and get rid of remove();
throws ExceptionPlease make sure your PR meets the following requirements: