-
Notifications
You must be signed in to change notification settings - Fork 62
refactor: begin to remove sigstore_protobuf_specs #1470
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
Conversation
I've published https://pypi.org/project/sigstore-models/ to accompany this. |
This is great, I'll try to review tomorrow (although I don't think we need to rush with this one) |
Thanks @jku! I have an internal need for this, but there's no huge rush from me -- I can always pin to the (With that being said, I'd love it if we could land this with v4, if you think that's possible -- I think it's be nice to have a clean major break.) |
This is the part I haven't yet wrapped my head around. Does pydantic still get some actual value from the serializers that are defined in sigstore-models? |
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'll have another look through the PR to make sure understand the serialization changes but I so far it looks good to me
- signed-off-by is apparently missing so tests don't run
- The main disadvantage of not using protobuf-specs (in addition to "someone" having to maintain sigstore-models) might be that dependabot will no longer warn us of changes... not much we can do about that
- Do you have a good feel for whether we end up modifying the public API in ways that need further documentation (like if the serialization issues ever end up visible to users?)
Nope, it's more that Pydantic (which we were already using indirectly, just with less direct control) doesn't have a super clean split between "wire type" and "real type" (for lack of a better term). For example, here's a made-up Protobuf definition: message Hello {
int64 abc = 1;
bytes def = 2;
} when serialized as ProtoJSON, that turns into a "wire" representation like: { "abc": "1234", "def": "aGVsbG8=" } i.e. To then pull those out of JSON and into useful Python types, we need Pydantic type adapters, e.g.: ...which we'd then use like this: class Hello(BaseModel):
abc: ProtoU64
def: ProtoBytes This works really well once the object is actually loaded (since the revealed types for Hello(abc=123, def=b"hello") instead, we have to do: Hello(abc=str(123), def=base64.b64encode(b"hello")) ...so we end up with explicit There might be a way around this, but it's not super clear from the Pydantic docs. I can look into it some more. |
If I'm right (famous last words) there should be no real public API changes from the models themselves here, although in practice this PR does shift the other public APIs quite a bit (like |
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
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.
It's very hard to make sure all of the b64encode(bytes.fromhex(...)
is correct but I've read through it twice and it seems to all make sense, thanks.
I would like to see the lint and test results though, can you rebase or merge main? (sorry about the parallel signing changes and staging changes making this a bit painful)
Yeah, agreed unfortunately -- I think this is ultimately a net improvement but it definitely shows a weak point with how we're (ab)using Pydantic. And no problem -- I'll deconflict this now. |
Hm, looks like Rekor v2 in staging is 503ing:
|
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.
yeah, lgtm.
The rekor 503 did not reproduce (I did realize the rekor-tiles http logging does not seem to be enabled while looking at this, I'll follow up on that elsewhere)
DCO check was still complaining but I will count this as a good faith effort to sign-off-by :) |
Towards #1049.
This is very WIP and won't work in CI yet since I'm using a local editable install of the newsigstore_models
package while I iterate on it.NB: I've also temporarily disabled interrogate because it has some kind of issue with a
cairo
dep.