-
Notifications
You must be signed in to change notification settings - Fork 241
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
[DRIVERS-2411] Add behavior for automatic Azure KMS credentials #1291
[DRIVERS-2411] Add behavior for automatic Azure KMS credentials #1291
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.
Looks pretty good. Caching the access token may be a requirement. Consider removing the retry.
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.
Nice, changes look good with the removal of one step and additional behavior to cache the token.
To confirm: are test changes going to be included in a later PR?
I will ask if others team members are planning to implement this soon to get more eyes on the review.
I'm just concerned with the base behavior for now, and creating a good testing method is still something I'm thinking over. I'm really averse to requiring a full Azure setup for the simplest tests, and I'm even wary of mocking it, but that might be the only good way to do it. |
@kevinAlbs Added instructions about token caching. |
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!
Refer: DRIVERS-2411
Please complete the following before merging: