-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🎉 Source Github: Add MultipleTokenAuthenticator #5223
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.
Could you please explain how token rotating is implemented while exceeded key limits?
There's a |
@gaart sounds good. Don't forget to run tests and format. |
/test connector=connectors/source-github
|
…-feature-rotate-tokens
…-feature-rotate-tokens # Conflicts: # airbyte-integrations/connectors/source-github/source_github/source.py
/test connector=connectors/source-github
|
…ure-rotate-tokens # Conflicts: # airbyte-cdk/python/unit_tests/sources/streams/http/auth/test_auth.py
/test connector=connectors/source-github
|
Why pass a comma separated list of tokens when you can just pass over a List? |
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.
one requested change but otherwise looks great
|
||
|
||
class MultipleTokenAuthenticator(HttpAuthenticator): | ||
def __init__(self, tokens: str, auth_method: str = "Bearer", auth_header: str = "Authorization"): |
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.
this should take a list of strings rather than a comma separated string
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.
good point, updated
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.
Added some requested improvements, feel free to merge once they are addressed!
airbyte-cdk/python/CHANGELOG.md
Outdated
## 0.1.10 | ||
Upd multiple token support: switch to list of tokens | ||
|
||
## 0.1.9 |
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.
do we need two versions for the same release? can we just make it a single version?
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.
Probably not. But both are released already
@@ -36,3 +37,14 @@ def __init__(self, token: str, auth_method: str = "Bearer", auth_header: str = " | |||
|
|||
def get_auth_header(self) -> Mapping[str, Any]: | |||
return {self.auth_header: f"{self.auth_method} {self._token}"} | |||
|
|||
|
|||
class MultipleTokenAuthenticator(HttpAuthenticator): |
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.
could you add a doc string explaining what this does e.g:
Uses the input list of tokens for authentication in a round-robin fashion. This allows load balancing quota consumption across multiple tokens.
airbyte-integrations/connectors/source-github/source_github/spec.json
Outdated
Show resolved
Hide resolved
@@ -9,7 +9,7 @@ | |||
"properties": { | |||
"access_token": { | |||
"type": "string", | |||
"description": "Log into Github and then generate a <a href=\"https://github.com/settings/tokens\"> personal access token</a>.", | |||
"description": "Log into Github and then generate a <a href=\"https://github.com/settings/tokens\"> personal access token</a>. Separate multiple tokens with \",\"", |
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.
Ideally, type: string
would be an array of strings too. However for backwards compatibility this is not great.
Could you create an issue to update this to a list of strings once #5396 is resolved?
] | ||
if repositories: | ||
repositories_list += repositories | ||
|
||
return list(set(repositories_list)) | ||
|
||
@staticmethod | ||
def _get_authenticator(token: str): | ||
if TOKEN_SEPARATOR not in token: |
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.
do we need a special edge case for this? multiple auth should still work with one token right?
…ec.json Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
…ec.json Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
/test connector=connectors/source-github
|
…-feature-rotate-tokens # Conflicts: # airbyte-integrations/connectors/source-github/source_github/source.py
/test connector=connectors/source-github
|
/test connector=connectors/source-github
|
…-feature-rotate-tokens
/test connector=connectors/source-github
|
/publish connector=connectors/source-github
|
What
Multiple token support added to the CDK.
Solving #3250
How
Now we have MultipleTokenAuthenticator which receives a comma-separated list of tokens and rotates them during API calls.