Skip to content

Add an all job for the test matrix #11880

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Add an all job for the test matrix #11880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9aa0220
Add an all job for the test matrix
altendky Jun 11, 2022
2a784fa
Update test.yml
altendky Jun 12, 2022
b5ef387
Update test.yml
altendky Jun 12, 2022
9a05d18
Update test.yml
altendky Jun 12, 2022
4d43541
try it as a reusable workflow
altendky Jul 9, 2022
fe7dfb9
move the if: always() out
altendky Jul 9, 2022
8dde40c
try not passing needs
altendky Jul 9, 2022
0daa165
Revert "try not passing needs"
altendky Jul 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .github/workflows/all.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
name: all

on:
workflow_call:
inputs:
needs:
required: true
type: string

jobs:
all:
name: All
runs-on: ubuntu-latest
steps:
- name: Require all successes
shell: python
env:
RESULTS: ${{ toJSON(fromJSON(inputs.needs).*.result) }}
run: |
import json
import os
import sys

results = json.loads(os.environ["RESULTS"])
sys.exit(0 if all(result == "success" for result in results) else 1)
10 changes: 10 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,13 @@ jobs:
concurrency_name: windows
configuration: ${{ needs.configure.outputs.configuration }}
runs-on: windows-latest

all:
uses: ./.github/workflows/all.yml
if: always()
needs:
- macos
- ubuntu
- windows
with:
needs: ${{ toJSON(needs) }}
Comment on lines +79 to +87
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I noticed you're reinventing the wheel here :)

I've got this https://github.com/re-actors/alls-green implemented a while back. Inspired by PyCA and some other research.

Suggested change
all:
uses: ./.github/workflows/all.yml
if: always()
needs:
- macos
- ubuntu
- windows
with:
needs: ${{ toJSON(needs) }}
all: # This job does nothing and is only used for the branch protection
if: always()
needs:
- macos
- ubuntu
- windows
runs-on: ubuntu-latest
steps:
- name: Decide whether the needed jobs succeeded or failed
uses: re-actors/alls-green@release/v1
with:
jobs: ${{ toJSON(needs) }}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about reinventing... pytest-dev/pytest-twisted@4c75379 :] but yes, thanks for the heads up on your action. I noticed it over in the towncrier comments as well. I'm glad to see this practice has some traction.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we've faced this problem in aiohttp where automerge would merge broken PRs and I've accidentally seen the solution in PyCA/cryptography, then gone ahead and generalized it a bit :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for "one check for branch protection" idea, it's ancient. Coming from CIs like Zuul and Bors.