-
Notifications
You must be signed in to change notification settings - Fork 28
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 option to suppress log in job summary #93
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks right
There's a leak in our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for the test failures and pmd warnings.
Not sure on those test failures given it works on my machine and Jenkins test infra; I think GitHub got confused by the ninja force push. |
Codecov Report
@@ Coverage Diff @@
## master #93 +/- ##
============================================
+ Coverage 85.90% 85.94% +0.03%
- Complexity 150 151 +1
============================================
Files 17 17
Lines 731 733 +2
Branches 54 55 +1
============================================
+ Hits 628 630 +2
Misses 85 85
Partials 18 18
Continue to review full report at Codecov.
|
316d53f
to
7060892
Compare
Passed on my local machine, but still failed in java 11 build after merging this. Maybe it's a java 11 build problem, I'll test it out. |
Fixes #92
I ummed and ahhed a bit over whether to have
suppressLogs
vsincludeLogs
but I went withtrue
== override default behaviour (with the assumption that most people do want logs).