-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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 GitHub Action gate/check #5492
Conversation
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage π
Additional details and impacted files@@ Coverage Diff @@
## master #5492 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 540 540
Lines 13969 13934 -35
=========================================
- Hits 13969 13934 -35
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. β View full report at Codecov. |
π Docs preview for commit 0a6abc6 at: https://6347752834a34f37544ccbd1--fastapi.netlify.app |
This is really smart! |
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.
Amazing!
Cool! I have a question, what happens with the smokeshow coverage job that is triggered after the main test workflow finishes? |
Nothing should change in that regard. Or do you have a specific concern on your mind? |
I understand this helps require all the "test" workflow to pass, then I would add this to the branch protection instead of the other individual rules, right? Then, I also have "smokeshow / coverage" as required in branch protection. But that's not part of the same "test" workflow. So this wouldn't cover that and I would still need to add that one manually to branch protection, right? |
@tiangolo the answer to both questions is "yes". By the way, I think you can now use reusable workflows in the same repo so you might be able to make them run in the same workflow run and make a common check gate. That's something someone could try experimenting with in the future. |
0a6abc6
to
18d1a18
Compare
Rebased it. |
π Docs preview for commit 18d1a18 at: https://636ade761a839e309115a5b7--fastapi.netlify.app |
@tiangolo ready to give it a try? There's still no conflicts in the PR, so it's ready to be merged anytime. |
Very cool! I have been trying it in other smaller projects. The only thing I miss is if it could output in the logs what matrix versions it checked. Currently, it only outputs the name of the job, but not the matrix combinations. E.g.
But it doesn't have info about the Python versions tested, to confirm that it's indeed checking all the combinations that I would put on the protection branch rules. |
GitHub does not provide any such job details in the |
@webknjaz I'm checking this and I see that the current file doesn't check for smokeshow, and that's a triggered workflow because it needs access to tokens, so it lives in a different workflow file. How do I add smokeshow as a need in this action? |
You can't add cross-workflow deps. |
@tiangolo extending the explanation: you pass the I'd recommend keeping that check in branch protection, just like the status from pre-commit.ci. If you really want another aggregated check, you could try using https://github.com/WyriHaximus/github-action-wait-for-status which does status polling in a loop and could theoretically watch statuses reported by anything, not just GHA. |
π Docs preview for commit e452ea8 at: https://639ce63dc1082f04e9d1c734--fastapi.netlify.app |
This adds a GHA job that reliably determines if all the required dependencies have succeeded or not. It also allows to reduce the list of required branch protection CI statuses to just one β `check`. This reduces the maintenance burden by a lot and have been battle-tested across a small bunch of projects in its action form and in-house implementations of other people. It is now in use in aiohttp (and other aio-libs projects), CherryPy, attrs, coveragepy, conda, pydantic, some of the Ansible repositories, open edX, pip-tools, spaceship-prompt, all of the jaraco's projects (like `setuptools`, `importlib_metadata`), some PyCQA, PyCA, PyPA and pytest projects, a few AWS Labs projects. The story behind this is explained in more detail at https://github.com/marketplace/actions/alls-green#why.
e452ea8
to
9607b23
Compare
π Docs preview for commit 9607b23 at: https://63b356d7238dde5a4334acf5--fastapi.netlify.app |
Cool, thanks for the explanation @webknjaz! π |
This adds a GHA job that reliably determines if all the required dependencies have succeeded or not.
It also allows to reduce the list of required branch protection CI statuses to just one β
check
. This reduces the maintenance burden by a lot and have been battle-tested across a small bunch of projects in its action form and in-house implementations of other people.It is now in use in aiohttp (and other aio-libs projects), CherryPy, attrs, coveragepy, some of the Ansible repositories, pip-tools, pydantic, spaceship-prompt, all of the jaraco's projects (like
setuptools
,importlib_metadata
), some PyCQA, PyCA, PyPA and pytest projects, a few AWS Labs projects.The story behind this is explained in more detail at https://github.com/marketplace/actions/alls-green#why.