Skip to content

Update and pin GitHub Actions + pre-commit hooks#2744

Open
agriyakhetarpal wants to merge 3 commits intopypa:mainfrom
agriyakhetarpal:pin-actions
Open

Update and pin GitHub Actions + pre-commit hooks#2744
agriyakhetarpal wants to merge 3 commits intopypa:mainfrom
agriyakhetarpal:pin-actions

Conversation

@agriyakhetarpal
Copy link
Member

@henryiii
Copy link
Contributor

I'm not a huge fan of fully pinned GHA's. Since the runner images can't be pinned, things change under you anyway. So pinning has negatives:

  • Keeps you from getting fixes quickly - including fixes for breakages in runners, like our own cibuildwheel v2.16.5, which fixed a change in powershell!
  • Adds churn to the CI since they have to be updated regularly.
  • Means old commits can stop building since you need fixes only in new commits, which is a problem if you ever have to backport fixes.

The positives:

  • Protects against intentional breakages. This is pretty rare, especially for the official actions.
  • Protects against short-lived bugs. Also pretty rare for official actions.
  • Protects against malicious releases. But do we really check every hashed release?

I'd argue the negatives outweigh the positives for the official actions most of the time. I'd go with fully pinned for a) less-trustworthy/used actions, b) critical parts of a workflow like the release part, and/or c) for really critical packages.

I'm against this, but I'm also not for it.

@agriyakhetarpal
Copy link
Member Author

I'm not a huge fan of fully pinned GHA's. Since the runner images can't be pinned, things change under you anyway. So pinning has negatives:

  • Keeps you from getting fixes quickly - including fixes for breakages in runners, like our own cibuildwheel v2.16.5, which fixed a change in powershell!
  • Adds churn to the CI since they have to be updated regularly.
  • Means old commits can stop building since you need fixes only in new commits, which is a problem if you ever have to backport fixes.

The positives:

  • Protects against intentional breakages. This is pretty rare, especially for the official actions.
  • Protects against short-lived bugs. Also pretty rare for official actions.
  • Protects against malicious releases. But do we really check every hashed release?

I'd argue the negatives outweigh the positives for the official actions most of the time. I'd go with fully pinned for a) less-trustworthy/used actions, b) critical parts of a workflow like the release part, and/or c) for really critical packages.

I'm against this, but I'm also not for it.

I agree, pins are not the best solution. However, in my understanding, they are the only way by which I can enable "Require actions to be pinned to a full-length commit SHA" to my downstream repositories where I use cibuildwheel. It's a shame that GitHub stopped working on github/roadmap#1103.

Your stance is similar to that of other projects in PyPA-land, and is also coupled with your experience maintaining cibuildwheel throughout its history, so I don't have sound reasons to disagree with it.

I can change this to pin a few selected workflows. In particular:

  • we should pin action.yml (so that downstream users like me and others can enable the actions pinning in their settings) – it only uses actions/setup-python@v6, which is not that prone to breakage.
  • we should pin release.yml, of course, as a critical workflow
  • update-dependencies.yml is also a "safe" workflow to keep pinned, and so is update-major-minor-tag.yml.
  • we should remove pins from all other actions, especially GitHub's own actions (I assume we can place them on a higher pedestal in terms of trust rating).

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.

2 participants

Comments