-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
Expose PEP 740 attestations functionality #236
base: unstable/v1
Are you sure you want to change the base?
Conversation
I've confirmed that the basic version of this works as expected ( |
@woodruffw I just bumped Twine FYI. And pre-commit is fine now. Rebasing should get the blockers out of the way. |
Good timing, so did @facutuesca 😅 |
Signed-off-by: William Woodruff <william@trailofbits.com>
I'm squashing and rebasing this branch now |
46986ef
to
b526ff8
Compare
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
@woodruffw @webknjaz The remaining lint failure is due to an error message string ( |
That's what I was thinking originally, although I think the "clean" thing to do here would be to turn the Python files here into a project structure that gets installed as part of the Docker image's build. But that's a little heavyweight, so @webknjaz may have another idea 🙂 |
I'm leaning towards just being available on |
@woodruffw hey, it looks like GitHub rolled out their own attestations in beta: https://github.com/actions/attest-build-provenance / https://github.com/pypa/gh-action-pypi-publish/attestations / https://github.com/orgs/community/discussions/122028. I wonder if we could somehow integrate with that... And it seems like there's even a new privilege |
(Sorry, was on PTO -- catching up on mentions now)
Yep, I've been thinking about how to make integrate the two -- the last comment on the PEP discussion thread suggests an approach that would allow GitHub-generated attestations to be compatible with this PEP. TL;DR: I think our options here are:
|
Just to update here: I've begun updating PEP 740 to be compatible with what GitHub's artifact attestations feature is doing, and it looks like it'll be pretty smooth. That then leaves the question of how to approach attestation generation here 🙂:
steps:
- name: attest
# does not exist yet!
uses: pypa/gh-action-pypi-attest
with:
# this would be the default; just for illustration
packages-dir: dist/
- name: publish
uses: pypa/gh-action-pypi-publish@release/v1 The benefit to approach (2) is that it's more devolved and can be built up on top of Edit: for context, the reason we can't use |
Awesome stuff! I would much prefer option 1, we should prioritize UX. |
I agree with @ofek that ergonomics is the most important part here, requiring users add another action step means we will see delayed rollout of the feature compared to adding it on to the existing workflow. |
SG, I'll keep going with the current approach then 🙂 |
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
This is good for an initial review! To summarize:
|
@woodruffw Just double checking long-term strategy, is the |
Yes, exactly -- my thinking is that it's already non-default due to the tail of people using older versions of this action, so having it be (temporarily) opt-in makes experimenting easier while giving us the ability to flip the switch once everything is stable. |
Sorry I didn't react earlier. I also think that UX is important. Not sure how well it plays with uploading attestations to GitHub, though. Can we attempt relying on the Another thought: why do we need to make |
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@webknjaz This could use another round of review, when you have time! (Please let me know if there's anything I can do to make the changes more tractable here -- I'm happy to try anything that'll make this easier to review 🙂) |
I'm happy to give this a try once I return to Los Angeles on the 22nd. |
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
I am happy to try this out with some of my projects, should that help 👍 |
Feel free! It should be as easy as replacing the stable v1 ref with my fork + branch and enabling the (You may want to start with just TestPyPI, to avoid spamming the admins with alerts on production.) |
None of my projects officially use the test environment.😂 |
Signed-off-by: William Woodruff <william@trailofbits.com>
Gentle ping for review 🙂 -- we're getting close to enabling attestation persistence on PyPI, so being able to roll this out (with the current toggle) will help us smoke test the functionality there. |
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 was hoping the refactoring PR would be completed by now, but it looks like that ain't happening... I suppose, this is a good patch to accept. Could you address a few comments below before I merge?
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
...to make the linters happy. Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
WIP, still experimenting here. Not ready for review 🙂This adds PEP 740 attestation generation to the workflow: when the Trusted Publishing flow is used, this will generate a publish attestation for each distribution being uploaded. These generated attestations are then fed into
twine
, which newly supports them via--attestations
.Breakdown:
twine --attestations
See: pypi/warehouse#15871