Add support for JWT Authentication #79
Conversation
| Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
dain left a comment
dain left a comment There was a problem hiding this comment.
The authorization header looks correct to me
| @dain @joshthoward thanks for the suggestions. I am waiting for my CLA to be updated otherwise the cla-bot will fail again. |
| @cla-bot check |
| The cla-bot has been summoned, and re-checked this pull request! |
| @joshthoward @dain I have applied the changes as you suggested. Please take a moment to review again. cc: @findepi |
| I did not use separate commits for the suggestion as I thought it would be unnecessary to add new commits for minor changes. Please let me know if this is the wrong practice so that I can improve in future. |
| LGTM |
| I don't know much about the python client, but the headers looks correct to me. I would suggest squashing into a single commit. |
| @ebyhr Thanks for the detailed review. I will make those changes. I was already using JWT token authentication on my local trino cluster. It is working without any problem. One question though as @dain suggested me to squash commits into one, does it mean that every PR should have single commit only? |
| @aakashnand No, it depends on the situation. We usually don't separate commits for code and docs change, so single commit in this PR is reasonable. |
| @ebyhr Got it. I will make changes soon and will request a final review again from you. Thanks |
| def get_exceptions(self): | ||
| return () | ||
| | ||
| def handle_error(self, handle_error): | ||
| pass |
There was a problem hiding this comment.
This is default behavior of Authentication, so seems can be omitted here
| | ||
| from six import with_metaclass | ||
| from typing import Any, Optional, Text # NOQA | ||
| from requests.auth import AuthBase |
There was a problem hiding this comment.
That's out of scope of this PR, but we should define an ALL here, so that from trino.auth import * doesn't pull things like AuthBase... or we say we don't care.
6 participants
The recent version of trino has the JWT authentication method so this PR adds support for JWT authentication in
trino-python-client. I have updated documentation inREADMEas well for the usage of JWT authentication. Please let me know if any improvement is needed.