Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Azure Signer support #588
Add Azure Signer support #588
Changes from all commits
f6ffa19
618b4de
a5317d1
1502d94
c88c7f9
ba237b8
90f15a4
8fe44e7
3239a7e
d7437bd
df9b5c2
a820bcf
942a229
041be38
473648c
41a89ae
ca0dad4
1ac6bc9
4184d0b
f2c354f
fe326b0
a692a5e
fa4f443
64a536e
4272655
d6e60b6
a702b39
80bf9a2
8fb25cc
9232e44
960b741
688555e
c00d3df
3c99967
b17370e
2d7c05b
f540090
e905b27
ca60489
eeabc68
36b65a4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should UnsupportedKeyType be public API? Currently it isn't, so a user can't really handle it. Would a built-in exception type also do the trick?
Also, is it really worth to catch, log and re-raise? Doesn't the original error message, which you control, provide enough info? This pattern seems more useful below, where you contextualise the 3rd-party HttpResponseError, although even there I wonder if the stack trace alone might be enough. 🤷
FYI: we have a discussion ticket about Signer exceptions in #468. Curious about your opinions.
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 left a note about this: #588 (comment)
TLDR: I think it's not really public API, it's just part of the larger group of exceptions that we know we may raise that callers are not really meant to handle: GCPSigner handles similar cases by just allowing KeyErrors from dict lookups go through...
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.
Agreed. I was actually trying to challenge the need for a custom exception and the way it is handled internally. But I'm okay with it. The code is still very readable and extra info for the caller is nice.