-
Notifications
You must be signed in to change notification settings - Fork 48
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
TrustUpdater
error handling
#525
TrustUpdater
error handling
#525
Conversation
CC @jku as well 🙂 |
eed4a94
to
fd05543
Compare
fd05543
to
7548f41
Compare
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.
LGTM
Hmm, not sure why the lint broke all of a sudden:
|
Yeah, it's working on my local machine (??) Can you trigger a rerun? |
Yep, triggered. My guess is that it's a version thing -- IIRC we run the lints against 3.7 locally, but the latest stable Python in CI. Edit: Spoke too soon -- we also use 3.7 in CI. Maybe a |
Yep, this is it -- |
Blocked on #530. |
7548f41
to
915f8de
Compare
Signed-off-by: Andrew Pan <a@tny.town>
915f8de
to
7455239
Compare
Signed-off-by: William Woodruff <william@yossarian.net>
/gcbrun |
Lint is failing on some APIs missing docstrings (we might need to add an exclude rule somewhere, since these are private APIs?)
|
Signed-off-by: Andrew Pan <a@tny.town>
Took a cursory glance at the interrogate documentation. It seems like we already have I'm adding documentation to stuff added by this PR to pass the lint, but maybe we should look into this in the future. |
Yeah, that smells like a bug to me (or, at least a documentation deficiency). Could you raise an upstream issue with them? |
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.
LGTM structurally, needs a CHANGELOG
entry 🙂
Will do!
Added to #525 (comment) :) Do we want a different entry for #531? |
Either a separate entry or adding to this one works for me! |
/gcbrun |
Signed-off-by: Andrew Pan <a@tny.town>
/gcbrun |
Summary
As discussed on #357,
TrustUpdater
does not have a coherent error handling scheme. The following exceptions are exposed to users:OSError
, when file read/write issues occurDownloadError
, when TUF fails to download metadata / target filesRepositoryError
, when TUF metadata is invalidException
, raised when we don't find the entries we expect in TUF metadataThis PR wraps TUF's error types with our own
TUFError
, which will help us provide useful diagnostics to users when a TUF-internal issue occurs. Additionally,MetadataError
is raised when theget
methods onTrustUpdater
don't find what they need.Also in this PR is the structure for a new global error handling system. The
Error
type implements utilities, namelyprint_and_exit
, for providing diagnostics to users.TUFError
extends from this type.Further work might include adapting existing error types, e.g.
CertificateVerificationFailure
, to this model. Doing this might make it easier for API consumers to handle Sigstore exceptions and allow us to export all of our error types from one place.Resolves #357.
Release Note