Clean up GitHub actions

[seifertm]

Removes pre-commit steps from the GitHub actions workflow, because the hooks are now executed by pre-commit.ci.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.50%. Comparing base (852b080) to head (40fb737).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #880   +/-   ##
=======================================
  Coverage   91.50%   91.50%           
=======================================
  Files           2        2           
  Lines         506      506           
  Branches      100      100           
=======================================
  Hits          463      463           
  Misses         25       25           
  Partials       18       18           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[webknjaz]

It's usually good to keep a duplicate run because then, it's possible to tie that into a single branch protection check and execute some checks that the external service is unable to run.

[webknjaz]

This change will effectively allow merging PRs with broken linting.

[webknjaz]

Also, this completely removes the integration of showing some linting-related messages as annotations.

[webknjaz]

This also breaks the ability to collect MyPy coverage (though, it's not configured now but would be good to have).

[seifertm]

Thanks for the comments! :)
Although I think I need more explanation for them.

It's usually good to keep a duplicate run because then, it's possible to tie that into a single branch protection check and execute some checks that the external service is unable to run.

I'm sorry, but I cannot follow. Keeping a duplicate run of what?

This change will effectively allow merging PRs with broken linting.

My understanding is that the newly added pre-commit.ci check will block the PR if the linting fails. What am I missing?

Also, this completely removes the integration of showing some linting-related messages as annotations.

Which annotations are you referring to?

[webknjaz]

It's usually good to keep a duplicate run because then, it's possible to tie that into a single branch protection check and execute some checks that the external service is unable to run.

I'm sorry, but I cannot follow. Keeping a duplicate run of what?

I mean the pre-commit.com CLI tool. The pre-commit.ci web service has limitations. It's good in general, and has built-in formatting support. Though, some things will never work in that environment. They aren't currently used, but one example would be things accessing network or needing an externally managed environment (e.g. pylint).

Though, to be fair, I prefer running linters in a separate jobs, not stuffed into one like they are now. It gives a lot move flexibility.

This change will effectively allow merging PRs with broken linting.

My understanding is that the newly added pre-commit.ci check will block the PR if the linting fails. What am I missing?

The only check marked as required right now is the one reported by the GHA job that uses my alls-green action. Nothing else influences people's ability to merge (including auto-merge). Having one check is simpler, so one doesn't have to re-enter the matrix-generated check names all the time. But it doesn't work with checks and statuses reported by things outside GHA.

So to work, it needs to be added to the branch protection additionally (which should be done for all the external checks anyway).

So it's extra maintenance and possible limitations. It'd be nice to enable coverage collection in MyPy, as another example, but you can only retrieve that coverage and send it to Codecov or display elsewhere if it's in GHA. See https://github.com/aio-libs/yarl/actions/runs/9934585463/job/27439204697#step:8:65.

Also, this completely removes the integration of showing some linting-related messages as annotations.

Which annotations are you referring to?

That .github/actionlint-matcher.json integration that you're deleting is capable of showing the errors in some web UI views in PRs (like the diff) on the corresponding lines. I wasn't able to find the official doc for some reason, but here's a random link from google: https://fusectore.dev/2021/11/19/github-action-problem-matchers.html.

[seifertm]

Thanks for the explanations!

I mainly added pre-commit.ci to get automatic version updates for the pre-commit hooks. I wasn't aware of its limitations and the branch protection configuration requirement. I'll revert the removal of pre-commit in the linting step as you suggested. Same goes for removal of coverage during the linting step.

The problem matchers are also new to me! However, it looks like the specific matcher that's deleted here is specifically matching the output of actionlint which is no longer executed in the pre-commit hooks. So it should be safe to delete?

[webknjaz]

@seifertm ah, I didn't realize. Yes, you can delete that one. Though, maybe it's worth resurrecting it and skipping it locally. If the linting invocations are properly wrapped with tox, having tox as an entry-point would give us more control. Then, it'd be as simple as integating SKIP=actionlint-docker pre-commit run --all-files that would skip the docker-based dep on dev machines but would still let us run it in regular CI. Another way would be moving the check to the manual stage so it'd be skipped except when specifically called (though a separate toxenv, for example). I used to have such conditionals for similar things that would be heavy but still desired.

On top of that, I suggest plugging the jsonschema check into pre-commit — it is useful for a similar purpose and includes ready-to-use schemas for GHA workflows as well as just GHAs. Additionally, it enables you to check things against your own custom schemas, which is really useful for preventing commonly seen mistakes: https://github.com/sphinx-contrib/sphinxcontrib-towncrier/blob/750485e/.pre-commit-config.yaml#L99-L116.

[seifertm]

@seifertm ah, I didn't realize. Yes, you can delete that one. Though, maybe it's worth resurrecting it and skipping it locally. If the linting invocations are properly wrapped with tox, having tox as an entry-point would give us more control. Then, it'd be as simple as integating SKIP=actionlint-docker pre-commit run --all-files that would skip the docker-based dep on dev machines but would still let us run it in regular CI. Another way would be moving the check to the manual stage so it'd be skipped except when specifically called (though a separate toxenv, for example). I used to have such conditionals for similar things that would be heavy but still desired.

On top of that, I suggest plugging the jsonschema check into pre-commit — it is useful for a similar purpose and includes ready-to-use schemas for GHA workflows as well as just GHAs. Additionally, it enables you to check things against your own custom schemas, which is really useful for preventing commonly seen mistakes: https://github.com/sphinx-contrib/sphinxcontrib-towncrier/blob/750485e/.pre-commit-config.yaml#L99-L116.

I'm convinced these are good ideas, but I'm reluctant to implement all of them. I followed your suggestion and resurrected the actionlint hook in the manual stage of pre-commit. This way, we don't require Docker for contributing devs, but it's still possible to lint GH actions.