-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI Enable Dependabot and use full length commit SHA for PyPI publishing #29180
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
CI Enable Dependabot and use full length commit SHA for PyPI publishing #29180
Conversation
.github/dependabot.yml
Outdated
- "github-actions" | ||
- "dependencies" | ||
reviewers: | ||
- "jeremiedbb" |
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.
I added @jeremiedbb as a default reviewer as git log -- .github/workflows/
makes it seem like they are responsible for a lot of this already.
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.
I really don't think that I've been more active on the maintenance of the workflows than others. I think it's better to add everyone as suggested in #29180 (comment), or maybe no one if possible. We already have bots submitting regular PRs without any pre-assigned reviewer and we don't miss them.
@@ -39,10 +39,13 @@ jobs: | |||
run: | | |||
python build_tools/github/check_wheels.py | |||
- name: Publish package to TestPyPI | |||
uses: pypa/gh-action-pypi-publish@v1.8.5 | |||
uses: pypa/gh-action-pypi-publish@81e9d935c883d0b210363ab89cf05f3894778450 # v1.8.14 |
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.
This PR is ready for review. |
.github/dependabot.yml
Outdated
patterns: | ||
- "*" | ||
labels: | ||
- "github-actions" |
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.
This label doesn't exist, so I'd drop it.
We could use "Build / CI" though
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.
I think it is a good idea to use hashes for workflows that do critical things like publishing to PyPI, have write
permissions to the repository content or that do the wheel building (but I think these aren't GitHub workflows)
Not sure about assigning a reviewer, but going to wait to see what Jeremie says.
In the past, I remember indeed that there were some issues with Dependabot, one of them being that it was running on forks, for example numpy/numpy#18977 but as @matthewfeickert says this has actually gone away since.
Just to make sure I understand this, that means that:
More generally, I would be curious about other projects experience with dependabot for updating actions hashes. One slight worry I have is that the CI in the PR does not actually test that updating the hashes does not break anything. For many workflows, it is actually quite hard to check this in the PR, for example Maybe it is still OK, because by using a monthly update, you kind of hope bugs have been reported and fixed before you update?
Not sure either. I guess it is not possible to assign a team like |
c8f4a54
to
7db9ca4
Compare
Yes.
It works very well for all of the Scikit-HEP org and Scientific Python in general. c.f. https://learn.scientific-python.org/development/guides/gha-basic/#updating
See the PR I linked where it shows the preview of the changes the hash change introduces. If you don't want to update a particular version, you can always tell Dependabot to skip a version until you figure out why your library can't use the latest version of a tool.
This is doable, but is this what you want? If so, that's an easy fix, but I assumed that you would have code owners for different part of the project. |
Just to be explicit regarding my previous comments, I would be +0.5 for enabling Dependabot, especially if other projects are happy with it, I am mostly trying to imagine/understand the potential issues that can happen. |
I also have mixed feeling w.r.t. dependabot on github actions workflow for the same reasons Loic expressed, but at the same time, barely never updating the versions of github actions manually is also a problem (e.g. if some older github actions have security vulnerabilities). |
* Related to SPEC 08 discussions at the 2024 Scientific Python Developer Summit, for publishing actions use the full length commit SHA. * Update pypa/gh-action-pypi-publish to v1.8.14. - c.f. https://github.com/pypa/gh-action-pypi-publish/releases/tag/v1.8.14
* Enable Dependabot for GitHub Action updates only and group updates together into a single PR. * Note that Dependabot is disabled on forks by default. - c.f. https://github.blog/changelog/2022-11-07-dependabot-pull-requests-off-by-default-for-forks/
7db9ca4
to
0f29f46
Compare
Yes, I mean the release notes that you showed.
From
Note that there is version drift between various workflows and some are fully unpinned. The first block of workflows are GitHub ones that should both be easy to test and very clear what the changes are. The second block is just Do you have experience with your labeling workflows being very fragile and easy to break? If not, then this hopefully would require a verify small amount of review effort to verify things before merge. |
@matthewfeickert thanks for the details! That's my feeling as well that the vast majority of the workflows we use are widely used and things should break quite rarely when updating their versions. Just to make sure, once this PR is merged, the fact that |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
OK let's merge this and see what happens! We can adapt things based on feed-back, mostly reviewers and labels. Thanks @matthewfeickert! |
Dependabot PR is up at #29203 |
Reference Issues/PRs
None that I'm aware of.
What does this implement/fix? Explain your changes.
At the 2024 Scientific Python Developer Summit, work on SPEC 08 (scientific-python/summit-2024#9) includes trying to harden publishing security by pinning GitHub Action workflows to commit hashes.
This PR does three things:
Pinning should be done throughout the publishing workflow, but this PR only implements it for the publishing state to start off and get agreement here first.
Any other comments?
cc @thomasjpfan @betatim given discussion at the summit