-
Notifications
You must be signed in to change notification settings - Fork 130
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
[Fix] Initiate a new OAuth external browser login flow in case of refresh token exception #823
base: main
Are you sure you want to change the base?
[Fix] Initiate a new OAuth external browser login flow in case of refresh token exception #823
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.
Miles is OOO.
Please add renaudhartert-db to the reviewer.
Anything I can do to get this prioritized? We've been stuck on 0.17.0 for more than a year, and this is the last piece needed for us to finish our migration to fully relying on SDK for dbt-databricks auth and being able to use latest versions of SDK. |
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 for the PR @eric-wang-1990! The overall logic looks good to me. Though, I would ideally like to see some form of regression test for this.
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
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.
@eric-wang-1990, I see you've re-assigned the PR to me but I cannot find any additional test in the code. Did you forget to commit the tests or is it that you need guidance to write them?
Just saw this, yeah I did not find any existing test about this cached token and I am not very familiar with this sdk codebase so I do not know where to add it. Can we land this fix first and then you guys can write the tests? Since dbt upgrade version is depending on this. |
Hi, we need this fix too, as we are currently using a workaround by try-excepting and deleting the cached token file. If tests are necessary and I can assist, you would need to point me in the right direction :) |
What changes are proposed in this pull request?
This PR change the OAuth token cache that when the cached oauth token cannot be refreshed, we still wanna go trigger a new OAuth login flow instead of just throwing an exception.
This help customers who use OAuth U2M, when their cached access token expire they have a way to continue rather than stuck with exception.
How is this tested?
Manual test from dbt-databricks.