[repo] Always execute build-test step #2054
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2054 +/- ##
==========================================
- Coverage 73.91% 72.47% -1.44%
==========================================
Files 267 314 +47
Lines 9615 11827 +2212
==========================================
+ Hits 7107 8572 +1465
- Misses 2508 3255 +747 Flags with carried forward coverage won't be shown. Click here to find out more. |
Kielek
force-pushed
the
always-execute-build-test
branch
6 times, most recently
from
3a3426c
to
e2a31c0
Compare
Kielek
force-pushed
the
always-execute-build-test
branch
from
e2a31c0
to
066393c
Compare
| if: always() && !cancelled() | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - run: echo 'build complete ✓' | ||
| - run: | | ||
| if ( ${{ contains(needs.*.result, 'failure') }} == true ); then echo 'build failed ✗'; exit 1; else echo 'build complete ✓'; fi |
There was a problem hiding this comment.
@Kielek I'm kind of confused by this 😄 build-test is a required status. If it doesn't run (because something failed), that should block the merge by design. Having it run always seems like it would pass the status check even when there was a failure?
There was a problem hiding this comment.
Current status:
When ${{ contains(needs.*.result, 'failure') is true (one of the required step is failing) build-status step is not executed. It has skipped status. For some reasons in this case, GitHub unblocks Merge button. Eg. #2052 there were possibility to merge traditional way, without forcing it.
After changes:
build-test is always executed, but when ${{ contains(needs.*.result, 'failure') is true it calls exit 1. It means that step is failing. The merge should be not possible. I was testing it, and seems it is working correctly.
I agree that the GitHub behavior is odd.
There was a problem hiding this comment.
Sounds good thanks for the explanation! We should probably do same over here eh? https://github.com/open-telemetry/opentelemetry-dotnet/blob/37535a5607ee7e4056c0e274ec01d1e0111a64be/.github/workflows/ci.yml#L189
There was a problem hiding this comment.
That's the plan. Same for auto-instrumentation repo.
|
Some random fail in this PR. Works as expected: |
Changes
Always execute build-tests step. It is used to determine if all steps passed.
Before changes there were possibility to merge changes even if previous steps were failing.
Merge requirement checklist
[ ] Unit tests added/updated[ ] AppropriateCHANGELOG.mdfiles updated for non-trivial changes[ ] Changes in public API reviewed (if applicable)