ci: notify other build status #521
Conversation
| def f | ||
|
|
||
| @Override | ||
| @Before | ||
| void setUp() throws Exception { | ||
| super.setUp() | ||
| f = new File("src/test/resources/console-100-lines.log") | ||
| env.TEST = "test" |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
|
💛 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errorsExpand to view the tests failures
Log outputExpand to view the last 100 lines of log output
|
| - wildcard: "*.*" | ||
| - wildcard: ".ci/**" | ||
| - wildcard: ".mvn/**" | ||
| - wildcard: "local/**" | ||
| - wildcard: "src/test/**" | ||
| - wildcard: "target/**" | ||
| - wildcard: "test-infra/**" |
There was a problem hiding this comment.
Unrelated with the aim of this particular PR, it will help to run the pipelines locally a bit faster
| @@ -1,4 +1,17 @@ | |||
| ## ${statusSuccess ? ':green_heart: Build Succeeded' : ':broken_heart: Build Failed'} | |||
| <%if (statusSuccess) {%> | |||
| ## :green_heart: Build Succeeded | |||
There was a problem hiding this comment.
Identation is not allowed otherwise the markdown will do something like:
## :green_heart: Build Succeeded
rather than
💚 Build Succeeded
| <%} else {%> | ||
| <%if (buildStatus?.equals('ABORTED')) {%> | ||
| ## :grey_exclamation: Build Aborted | ||
| ${(build?.description?.toLowerCase().contains('aborted')) ? '> There is a new build on-going so the previous on-going builds have been aborted.' : ''} |
There was a problem hiding this comment.
This particular validation works since
is in charge to add the reason of the failure
…on for other kind of aborted builds
| ## :grey_exclamation: Build Aborted | ||
| ${(build?.description?.toLowerCase()?.contains('aborted')) ? '> There is a new build on-going so the previous on-going builds have been aborted.' : ' > Either there was a build timeout or someone aborted the build.'} | ||
| <%} else if (buildStatus?.equals('UNSTABLE')) {%> | ||
| ## :yellow_heart: Tests Failed |
There was a problem hiding this comment.
@cachedout , what do you think?
There was a problem hiding this comment.
The more I think about this, the more I wonder if we should switch these around. Here is my reasoning:
Tests Failed
Here, we would use a red heart. For a developer, this is a condition that needs their attention, because something they did is probably wrong and needs to be fixed. Therefore, it should be given the strongest color (red).
Build did not succeed
Here we would use yellow heart. This means that things are still not quite right, but it's not something that the developer did wrong, so we use a less-severe color.
Build succeeded
Still a green heart. :)
WDYT?
There was a problem hiding this comment.
A bit of context, the unstable/success/abort/failed status that I followed where based on the Jenkins build terminology (here and here)
I'm not saying it's clear though, neither we do need to implement it. Glad in fact you have suggested to review it.
There are certain use cases, we might need to consider:
- What if a particular pipeline gets unstable for a non-critical but cosmetic linting?
- What if a particular pipeline gets unstable if the JUnit/coverage reporting got over certain threshold?
OTOH, the unstable status is handled by the pipeline itself, in other words, it's up to the developer/build engineer to decide what the pipeline should do if a particular stage got a linting/warning issue, in some cases, the pipeline should fail fast, or should move forward, or should be reported as success.
Maybe the GH PR comment should be more generic to support other kind of CI tools in the future, and here we got the chance to normalise the status to be clear among all the projects.
There was a problem hiding this comment.
I think that the historical CI status indicators (provided by Jenkins) are more helpful to Jenkins administrators than they are to actual end users. I don't think too many developers know (or care) what an "unstable" build means, for example.
So, I agree that we can and should handle specific uses cases even better. If developers are used to seeing a green heart as "ready to merge" and any other status as "something that needs more investigation", then we can add more failure categories that can help a developer quickly glance at the output to see what's wrong.
I like the idea of the PR comment statuses being more generic and free of the implementation of the CI underneath. I think in doing so that we can give developers feedback that's more tailored to an action that they might need to take to get a PR in a merge-able state.
There was a problem hiding this comment.
The Build did not succeed status will report two different things "failed" and "abort" that are really different things. the thing is, Do we want to keep it simple? we should use a binary result (whatever names we use), or Do we want to keep it fine-tune? so we add some intelligence to analyze the result and provide more explanatory messages.
There was a problem hiding this comment.
For simplicity I think we can get rid of the unstable status, and ensure the pipeline will always report the below three status:
- 💚
success. - 💔
failed(either compilation or test failures) Thereforeunstablewill always behave as a failed. - ❕
aborted(either manually aborted, timeout issue or cancel by the new build, or CI approval required). Timeout could be potentially something to be reported differently, but for the time being, I'd say let's keep it simple.
Regarding the fine granularity, we could potentially use the build.description to gather further details, and therefore to expose it , for instance see #522 (comment)
In other words, the result analyser could be done as a post build action to gather what's the high level information to be provided to the PR.
@elastic/observablt-robots , what do you think?
There was a problem hiding this comment.
Build and test errors are now reported as 💔
even when the tests are not blocking the build but reported as unstable:
There was a problem hiding this comment.
Big 👍 on these changes, @v1v . I think this strikes a really good balance! Nicely done! 💯
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errorsExpand to view the tests failures
Log outputExpand to view the last 100 lines of log output
|
| ## :grey_exclamation: Build Aborted | ||
| ${(build?.description?.toLowerCase()?.contains('aborted')) ? '> There is a new build on-going so the previous on-going builds have been aborted.' : ' > Either there was a build timeout or someone aborted the build.'} | ||
| <%} else {%> | ||
| ## :broken_heart: ${(testsSummary?.failed != 0) ? 'Tests Failed' : 'Build Failed'} |
There was a problem hiding this comment.
unstable and failed builds are equally the same, the only difference might be if there are failed tests, and if so the icon will remain for both but the description will be more accurate
| @@ -6,10 +6,10 @@ | |||
| "href": "/blue/rest/organizations/jenkins/pipelines/apm-shared/pipelines/apm-apm-pipeline-library-mbp/pipelines/develop/runs/49/blueTestSummary/" | |||
| } | |||
| }, | |||
| "existingFailed": 1, | |||
| "failed": 1, | |||
| "existingFailed": 0, | |||
There was a problem hiding this comment.
Test reporting does require this file with the failed tests, therefore it's required another file for testing purposes
…orting * upstream/master: ci: CI approval aborted rather than failed (#522) ci: avoid template when no PRs (#526) test: move to apm-ci folder (#524) test: use a different port (#523)
…-library into feature/abort-reporting * 'feature/abort-reporting' of github.com:v1v/apm-pipeline-library:
| <%if (build?.description?.toLowerCase()?.contains('aborted')) {%> | ||
| > There is a new build on-going so the previous on-going builds have been aborted. | ||
| <%} else if (build?.description?.toLowerCase()?.contains('allowed')) {%> | ||
| > ${build.description} | ||
| <%} else {%> | ||
| > Either there was a build timeout or someone aborted the build.'} | ||
| <%}%> |
There was a problem hiding this comment.
Identify the reason of the aborted build using the build description field.
What does this PR do?
Enable the build status reporting for some other use cases:
See the tests to get an overall view of the new reporting
Why is it important?
This will help to distinguish what's the build status easily from the comment
Related issues
Closes #506
Notifies elastic/beats#18317
Tasks
Tests
Aborted build (from following builds)
Caused by:
Expand to view screenshots
Aborted build (some other use cases)
Caused by:
Timeout has been exceededExpand to view screenshots
Success
Caused by:
Expand to view screenshots
Test failures
Caused by:
Expand to view screenshots
Caused by:
Expand to view screenshots
Some other failures
Caused by:
Expand to view screenshots