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

upload: add attestations to PackageFile #1098

Merged
merged 5 commits into from
May 1, 2024

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Apr 30, 2024

WIP, needs more tests.

Summary of changes:

  • Updates _make_package to accept an attestations parameter, which receives the list of attestations for the distribution (from _split_inputs).
  • When --attestations is set and attestations is nonempty, the supplied attestations are passed into the constructed PackageFile.
  • Internally, PackageFile reads each attestation as JSON and compounds it onto a JSON array to send as the attestations field during upload, per PEP 740.

See #1094.

Signed-off-by: William Woodruff <william@yossarian.net>
@@ -186,6 +188,9 @@ def metadata_dictionary(self) -> Dict[str, MetadataValue]:
if self.gpg_signature is not None:
data["gpg_signature"] = self.gpg_signature

if self.attestations is not None:
data["attestations"] = json.dumps(self.attestations)
Copy link
Member

Choose a reason for hiding this comment

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

I'm 99% certain I remember that this ends up in a multipart/form-data body. Does PyPI want this to have a content type? Also, does this need to be conditioned on index server? I don't expect any of the third party ones to support this any time soon

Copy link
Member Author

Choose a reason for hiding this comment

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

In the current draft, we expect it to be in that multipart/form-data body: https://peps.python.org/pep-0740/#upload-endpoint-changes. No content-type marker is expected.

(I have no objection to changing this to require a content-type like the main content field, if you think that would be clearer! I'd have to tweak the PEP in that case.)

Also, does this need to be conditioned on index server? I don't expect any of the third party ones to support this any time soon

Everything is currently flagged behind --attestations which is off by default, so in principle this shouldn't need to be conditioned on the index server. That being said I could additionally add that check, although I think third party indices/mirrors may eventually want to support these as well and may be surprised by twine pre-filtering them 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't know. I'm more worried about people enabling this against a not-PyPI index that won't ignore the field and complaining about the errors/failures to upload

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm more worried about people enabling this against a not-PyPI index that won't ignore the field and complaining about the errors/failures to upload

Ah yeah, hadn't thought of that. Support on non-PyPI indices is a distant idea anyways so I'll flag this off 🙂

Copy link
Member

Choose a reason for hiding this comment

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

We can always add another option to enable it on a per-index basis later. But yeah, I just dread every 3rd party index issue we get because it is always a nightmare of the index doing something that violates a specification

twine/package.py Outdated Show resolved Hide resolved
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
@woodruffw woodruffw marked this pull request as ready for review May 1, 2024 18:33
@woodruffw
Copy link
Member Author

Should be good for another look! I've rounded out the coverage as well.

@sigmavirus24 sigmavirus24 merged commit 0ec5d18 into pypa:main May 1, 2024
22 of 23 checks passed
@woodruffw woodruffw deleted the ww/attestations-attach branch May 1, 2024 23:05
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