-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
attestion: make InvalidAttestationError
non-fatal in CI
#18485
Conversation
Huh, interesting. Not even code I touched though. Would love a suggestion here. CC @dduugg ? |
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 appreciate the intent here but 👎🏻 as-is.
If we're going to make this non-fatal it feels like it defeats the entire point of having attestation.
I'd like to have us try a couple of retries with an exponential backoff before we do this.
Try pairing your change with an explicit |
I don't think I've seen an `InvalidAttestationError` that wasn't some sort of network problem (e.g., rate limit, connection timeout, 503). Let's emit a warning instead of erroring out. Note that `MissingAttestationError` is still fatal, and that will still produce errors in CI.
24b3045
to
ba98b0a
Compare
Implemented retry logic instead. |
ba98b0a
to
6b63660
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.
Thanks @carlocab! Have suggested speeding up failure time so we can evaluate if this is something we should back out entirely for now. Feel free to merge as-is if preferred.
Let's keep the retry timing as is. The case for backing this out if we need to will be stronger if we were more patient about retries. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?I don't think I've seen an
InvalidAttestationError
that wasn't somesort of network problem (e.g., rate limit, connection timeout, 503).
Let's emit a warning instead of erroring out.
Note that
MissingAttestationError
is still fatal, and that will stillproduce errors in CI.