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

fix --update_goldens for serialization module #250

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

spencerschrock
Copy link
Contributor

Summary

Writing the sharded serializer golden files involves sorting _Shard items, so they must be orderable. Previous attempts to update goldens would result in a TypeError:

Release Note

NONE

Documentation

NONE

Writing the sharded serializer golden files involves sorting _Shard
items, so they must be orderable. Previous attempts to update goldens
would result in a TypeError.

Signed-off-by: Spencer Schrock <sschrock@google.com>
@spencerschrock spencerschrock requested review from a team as code owners July 23, 2024 17:17
Copy link
Collaborator

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

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

Thank you!

Internally this doesn't fail, and I only included the fix in #245 instead of the good PR.

Thanks for catching!

@spencerschrock
Copy link
Contributor Author

spencerschrock commented Jul 23, 2024

Internally this doesn't fail

It does actually. The _Shard dataclass was added after I asked you to regenerate the goldens during review

and I only included the fix in #245

Didn't catch this whoops!

@mihaimaruseac mihaimaruseac merged commit 19a1d45 into sigstore:main Jul 23, 2024
20 checks passed
@mihaimaruseac
Copy link
Collaborator

Oh, I only ran the tests at the top of the chain :| That's on me

@spencerschrock spencerschrock deleted the fix-update-goldens branch July 23, 2024 17:46
@mihaimaruseac mihaimaruseac added this to the V1 release milestone Jul 24, 2024
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