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.

Sign up for GitHub

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

Jump to bottom

gen, protos: remove ExtendedVerificationMaterials, embed its members #36

Merged
merged 5 commits into from
Dec 15, 2022
Merged

gen, protos: remove ExtendedVerificationMaterials, embed its members #36

merged 5 commits into from
Dec 15, 2022

Conversation

woodruffw
Copy link
Member

See #30.

Signed-off-by: William Woodruff [email protected]

@woodruffw woodruffw changed the title gen, protos: rename VerificationData to `ExtendedVerificationMateri… gen, protos: rename VerificationData to ExtendedVerificationMaterials Dec 12, 2022
@woodruffw
Copy link
Member Author

cc @znewman01 and @kommendorkapten

@woodruffw
Copy link
Member Author

Sidenote: I'm trustworthy (pinky promise!) but it isn't ideal that the generated code gets folded by GitHub's code review view...

@woodruffw
gen, protos: remove ExtendedVerificationMaterials entirely
8fc62af 
...and embed its contents into the `Bundle` message.

Signed-off-by: William Woodruff <[email protected]>
haydentherapper
haydentherapper reviewed Dec 12, 2022
View reviewed changes
protos/sigstore_bundle.proto Outdated
oneof content {
dev.sigstore.common.v1.MessageSignature message_signature = 4;
dev.sigstore.common.v1.MessageSignature message_signature = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the bundle format is now being used by sigstore-js, so we can't renumber the tags (@kommendorkapten ?)

@woodruffw woodruffw changed the title gen, protos: rename VerificationData to ExtendedVerificationMaterials gen, protos: removeExtendedVerificationMaterials, embed its members Dec 12, 2022
@woodruffw woodruffw changed the title gen, protos: removeExtendedVerificationMaterials, embed its members gen, protos: remove ExtendedVerificationMaterials, embed its members Dec 12, 2022
@kommendorkapten
Copy link
Member

Yes, sigstore-js is using this, and some other components related to that work.

I think I would prefer to have it grouped as is, as they are a bit related. And also, for future extensions like e.g. RoughTime, that can be added to the Extended/VerificationMaterials which IMHO is a good thing to be able to group relevant attributes together.

@woodruffw
Copy link
Member Author

I agree 100% that they're related, but I think (per what @haydentherapper said) that they're key to the "value ad" provided by Sigstore vs. ordinary signing systems. Given that, I think we want to shy away from branding these as Extended in any way, since that implies more optionality than we'd like.

Perhaps instead of putting them in Bundle, we can embed them in VerificationMaterial instead? That matches their actual role more closely.

@kommendorkapten
Copy link
Member

Yes, that sounds like a good idea!

@woodruffw
Copy link
Member Author

Awesome! I'll update the PR again.

@woodruffw
Copy link
Member Author

Done; I had to move VerificationMaterials into sigstore_bundle.proto to keep the imports working, but otherwise this just embeds the ExtendedVerificationMaterials into that message.

@kommendorkapten
Copy link
Member

Awesome, I'll take a look later today or tomorrow, busy with meetings rest of the day!

haydentherapper
haydentherapper previously approved these changes Dec 13, 2022
View reviewed changes
Copy link
Collaborator

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

lgtm, i'll leave it open for a little in case anyone else has any comments

kommendorkapten
kommendorkapten previously approved these changes Dec 14, 2022
View reviewed changes
Copy link
Member

@kommendorkapten kommendorkapten left a comment

Choose a reason for hiding this comment

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

🚀

@kommendorkapten
Copy link
Member

lgtm, i'll leave it open for a little in case anyone else has any comments

Makes sense. I can merge early next week if no more comments.

@woodruffw
Copy link
Member Author

Thanks both!

@woodruffw woodruffw mentioned this pull request Dec 14, 2022
Merged
@woodruffw
Copy link
Member Author

Deconflicted! Sorry for the review request churn; I thought I could request two at once 😅

kommendorkapten
kommendorkapten approved these changes Dec 15, 2022
View reviewed changes
Copy link
Member

@kommendorkapten kommendorkapten left a comment

Choose a reason for hiding this comment

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

🚀

@znewman01 znewman01 merged commit 57e35a7 into sigstore:main Dec 15, 2022
@znewman01
Copy link
Contributor

Sidenote: I'm trustworthy (pinky promise!) but it isn't ideal that the generated code gets folded by GitHub's code review view...

Fortunately the tests would fail if it didn't match: https://github.com/sigstore/protobuf-specs/blob/main/.github/workflows/generate.yml

@woodruffw woodruffw deleted the ww/rename-verificationdata branch December 15, 2022 16:01
@woodruffw
Copy link
Member Author

Fortunately the tests would fail if it didn't match: https://github.com/sigstore/protobuf-specs/blob/main/.github/workflows/generate.yml

Good to know! I'm glad that's being cross-checked.

@codysoyland codysoyland mentioned this pull request Dec 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kommendorkapten kommendorkapten kommendorkapten approved these changes

@haydentherapper haydentherapper haydentherapper left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@woodruffw @kommendorkapten @znewman01 @haydentherapper