8000 πŸ‘· Add GitHub Action gate/check by webknjaz Β· Pull Request #5492 Β· fastapi/fastapi Β· GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
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

Merged
merged 1 commit into from
Jan 7, 2023

Conversation

webknjaz
Copy link
Contributor
@webknjaz webknjaz commented Oct 13, 2022

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.

@codecov
Copy link
codecov bot commented Oct 13, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage πŸ‘

Coverage data is based on head (0a6abc6) compared to base (cf73051).
Patch has no changes to coverable lines.

❗ Current head 0a6abc6 differs from pull request most recent head 9607b23. Consider uploading reports for the commit 9607b23 to get more accurate results

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     
Impacted Files Coverage Ξ”
fastapi/utils.py 100.00% <0.00%> (ΓΈ)
fastapi/routing.py 100.00% <0.00%> (ΓΈ)
tests/test_datastructures.py 100.00% <0.00%> (ΓΈ)
tests/test_additional_responses_router.py 100.00% <0.00%> (ΓΈ)

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.
πŸ“’ Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

πŸ“ Docs preview for commit 0a6abc6 at: https://6347752834a34f37544ccbd1--fastapi.netlify.app

@adriangb
Copy link
Contributor

This is really smart!

Copy link
Contributor
@iudeen iudeen left a comment

Choose a reason for hiding this comment

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

Amazing!

@tiangolo
Copy link
Member
tiangolo commented Nov 7, 2022

Cool! I have a question, what happens with the smokeshow coverage job that is triggered after the main test workflow finishes?

@webknjaz
Copy link
Contributor Author
webknjaz commented Nov 7, 2022

Nothing should change in that regard. Or do you have a specific concern on your mind?

@tiangolo
Copy link
Member
tiangolo commented Nov 7, 2022

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?

@webknjaz
Copy link
Contributor Author
webknjaz commented Nov 8, 2022

@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.

@webknjaz webknjaz force-pushed the maintenance/gha-check branch from 0a6abc6 to 18d1a18 Compare November 8, 2022 22:52
@webknjaz
Copy link
Contributor Author
webknjaz commented Nov 8, 2022

Rebased it.

@github-actions
Copy link
Contributor
github-actions bot commented Nov 8, 2022

πŸ“ Docs preview for commit 18d1a18 at: https://636ade761a839e309115a5b7--fastapi.netlify.app

@webknjaz
Copy link
Contributor Author

@tiangolo ready to give it a try? There's still no conflicts in the PR, so it's ready to be merged anytime.

@tiangolo
Copy link
Member

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.

πŸ“ test β†’ βœ“ success [required to succeed]

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.

@webknjaz
Copy link
Contributor Author
webknjaz commented Nov 25, 2022

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.

GitHub does not provide any such job details in the ${{ needs }} context for the matrix jobs. It only exposes the names and the outcomes basically. Implementing heuristics seems fragile. You can control which matrix subjobs are allowed to fail via continue-on-error but other than that β€” if you don't use anything to mangle with the job outcomes β€” the whole matrix would be required. There's really no way of lurking into the workflow.

@tiangolo
Copy link
Member
tiangolo commented Nov 27, 2022

@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?

@webknjaz
Copy link
Contributor Author

You can't add cross-workflow deps.

@webknjaz
Copy link
Contributor Author
webknjaz commented Nov 30, 2022

@tiangolo extending the explanation: you pass the ${{ needs }} context which alls-green uses to analyze job outcomes. It is not only limited but also only exists in the job context of a workflow. Another workflow run will happen independently and won't have anything from that context.
Besides that workflow job's status does not even appear in the PR checks. Instead, it uses Checks API Statuses API to report a custom check not matching the job name.

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.
Note that you'd need to do this in a yet another separate workflow since it'd deadlock if you were to add this to the check job introduced here. I haven't tried doing this, though.

8000

@github-actions
Copy link
Contributor

πŸ“ 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.
@webknjaz webknjaz force-pushed the maintenance/gha-check branch from e452ea8 to 9607b23 Compare January 2, 2023 22:09
@github-actions
Copy link
Contributor
github-actions bot commented Jan 2, 2023

πŸ“ Docs preview for commit 9607b23 at: https://63b356d7238dde5a4334acf5--fastapi.netlify.app

@tiangolo tiangolo changed the title Introduce a gate/check GHA job πŸ‘· Add GitHub Action gate/check Jan 7, 2023
@tiangolo
Copy link
Member
tiangolo commented Jan 7, 2023

Cool, thanks for the explanation @webknjaz! πŸš€

@tiangolo tiangolo merged commit 2583a83 into fastapi:master Jan 7, 2023
JonasKs pushed a commit to JonasKs/fastapi that referenced this pull request Jan 12, 2023
axel584 pushed a commit to axel584/fastapi that referenced this pull request Mar 5, 2023
mahenzon pushed a commit to mahenzon/fastapi that referenced this pull request Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0