-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: improve NetHSM API exception handling #445
feat: improve NetHSM API exception handling #445
Conversation
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, just some comments.
pynitrokey/nethsm/__init__.py
Outdated
# 405 "Method Not Allowed" mostly happens when the UserID or KeyID contains a character | ||
# - that ends the path of the URL like a question mark '?' : | ||
# /api/v1/keys/?/cert will hit the keys listing endpoint instead of the key/{KeyID}/cert endpoint | ||
# - that doesn't cound as a phath parameter like a slash '/' : | ||
# /api/v1/keys///cert will be interpreted as /api/v1/keys/cert with cert as the KeyID |
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.
Is that something that we should validate before sending the request?
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.
That would be the best solution yes
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.
Please create a ticket for it (or a PR ;)).
d34c352
to
bf6be1e
Compare
bf6be1e
to
659b308
Compare
This PR improves the handling of API errors in the nethsm commands.
Checklist
Make sure to run
make check
andmake fix
before creating a PR, otherwise the CI will fail.Test Environment and Execution