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

sigstore, test: drastically simplify error types #959

Merged
merged 7 commits into from
Apr 5, 2024

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Apr 4, 2024

WIP.

Another chore/refactor PR: this drastically reduces the number of error types we have floating around the codebase, in favor of a single unified VerificationError (which we can specialize in the future, as necessary).

Additionally, this changes the verify(...) API from returning a VerificationResult to using exceptional error handling. There are three justifications for this:

  1. It allows us to remove the VerificationResult model and its subclasses, which are mostly just duplication over existing error types
  2. It reduces the likelyhood of VerificationErrors leaking through the API accidentally
  3. It aligns the API with that the DSSE/in-toto API will do, which is use a return value on the success case and exceptions for all errors.

In the process, this also brings our coverage up a bit.

@woodruffw woodruffw added refactoring Refactoring tasks. chore labels Apr 4, 2024
@woodruffw woodruffw requested a review from jku April 4, 2024 16:26
@woodruffw woodruffw self-assigned this Apr 4, 2024
@woodruffw woodruffw marked this pull request as ready for review April 4, 2024 19:19
@woodruffw woodruffw mentioned this pull request Apr 4, 2024
3 tasks
Signed-off-by: William Woodruff <[email protected]>
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

Nice,

  • Totally agree about the exception simplification
  • Verifier.verify() API change is a little annoying but it does seem to make a lot of code more straight forward

left a question on the argument parsing, otherwise LGTM.

sigstore/_cli.py Show resolved Hide resolved
sigstore/errors.py Show resolved Hide resolved
@woodruffw woodruffw merged commit 721ddcb into main Apr 5, 2024
25 checks passed
@woodruffw woodruffw deleted the ww/cleanup-verify-errors branch April 5, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore refactoring Refactoring tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants