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

Fail the build if postStart/preStop commands return a non-zero exit code #914

Merged
merged 3 commits into from Jan 18, 2018
Merged

Fail the build if postStart/preStop commands return a non-zero exit code #914

merged 3 commits into from Jan 18, 2018

Conversation

ghost
Copy link

@ghost ghost commented Dec 27, 2017

Extend #272 to check the exit code of the command and if non-zero fail the build.

Use case: start a DB server and in create the db schema which might fail; fail-early when the container is created

I can look further into adding a test for it if the approach looks fine.

@codecov
Copy link

codecov bot commented Dec 27, 2017

Codecov Report

Merging #914 into master will decrease coverage by 0.32%.
The diff coverage is 21.25%.

@@             Coverage Diff              @@
##             master     #914      +/-   ##
============================================
- Coverage     51.89%   51.57%   -0.33%     
  Complexity     1232     1232              
============================================
  Files           142      144       +2     
  Lines          7182     7250      +68     
  Branches        972      981       +9     
============================================
+ Hits           3727     3739      +12     
- Misses         3141     3195      +54     
- Partials        314      316       +2
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/io/fabric8/maven/docker/StopMojo.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...o/fabric8/maven/docker/model/ContainerDetails.java 65.78% <ø> (ø) 18 <0> (ø) ⬇️
.../io/fabric8/maven/docker/service/WatchService.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../fabric8/maven/docker/wait/HealthCheckChecker.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ava/io/fabric8/maven/docker/model/ExecDetails.java 0% <0%> (ø) 0 <0> (?)
...c/main/java/io/fabric8/maven/docker/StartMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...va/io/fabric8/maven/docker/AbstractDockerMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../io/fabric8/maven/docker/access/ExecException.java 0% <0%> (ø) 0 <0> (?)
...ven/docker/access/hc/DockerAccessWithHcClient.java 13.66% <0%> (-0.38%) 12 <0> (ø)
...ava/io/fabric8/maven/docker/access/UrlBuilder.java 67.22% <0%> (-1.15%) 21 <0> (ø)
... and 7 more

erlyfall and others added 2 commits January 18, 2018 10:02
…ro exit code.

Signed-off-by: Ion Savin <comp_@gmx.net>
This will, if set, break the build if the pre/post command fails
(instead of only logging an error)

Based on top of #914
@rhuss
Copy link
Collaborator

rhuss commented Jan 18, 2018

Hey @isavin, thanks a lot ! (and sorry for the delay).

I just rebased it to the current master and added an option 'breakOnError' for the wait exec condition, which can be set to break the build if an error occurs. Just wanted to add this as I don't want to break backwards compatibility for now.

For version 1.0 we could consider to switch to true by default.

Do you have any chance to try this out and check whether it fits your needs ? thanks!

* in case breakOnError is disabled log on WARN instead of ERROR
* fixed a typo "preStart" vs "postStart"
* logged a similar message for preStop
* fixed the expected configuration parameter from 'isBreakOnError' to 'breakOnError'
* removed the IllegalArgument exception thrown by the ExecException constructor and fixed the exception message which was null

Signed-off-by: Ion Savin <comp_@gmx.net>
@ghost
Copy link
Author

ghost commented Jan 18, 2018

Thank you for the feedback @rhuss! I've tested the changes and made a few fixes.

I've pushed a commit which includes these fixes and posted the sample project I've used to test them.

I didn't know what was the expected commit message for a fixup so I've just listed the fixed. Should I use the same msg as your last commit or something else? Please let me know and I'll updated it alternatively if it can be merged into yours as a fixup please feel free to do that.

@rhuss
Copy link
Collaborator

rhuss commented Jan 18, 2018

Thanks, looks good to me ! wrt. to the commit message: There is no standard established yet for dmp, but I'm used to use commitizen like commit logs --> https://commitizen.github.io/cz-cli/

I'm not dogmatic here, though ;-)

Let's merge it and many thanks for the contribution ! Feel free to add more if you feel fancy ;-)

@rhuss rhuss merged commit 4f9a721 into fabric8io:master Jan 18, 2018
@ghost
Copy link
Author

ghost commented Jan 19, 2018

Thank you for the cz pointer and for merging this!

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.

2 participants