-
Notifications
You must be signed in to change notification settings - Fork 7
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
Enable TNU to use Azure auth for DRS operations #399 #401
Enable TNU to use Azure auth for DRS operations #399 #401
Conversation
963a76c
to
621e3b6
Compare
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.
Overall looks good. I like the tagging of execution_platform
and execution_context
.
Were you able to test in a real Azure workspace notebook against an Azure DRS URL? It looks like there is still some GCP-specific logic in drs.py
(such as checking requester-pays, and trying to resolve the file a GCS object if the signed url doesn't work). But hopefully the auth piece is enough to simply make it work for Azure signed URLs.
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.
Couple of thoughts but overall looks good!
Add support for using Microsoft Azure default credentials when running in a Terra Azure Interactive Analysis Cloud Environment. These credentials are used to auth with Terra backend services, including Martha/terra-drs-hub and Rawls. When running in a Terra Google IA Cloud Environment, the behavior is as before, with no changes. This initial implementation is thought to be functionally complete, yet tests and documentation remain to be added.
Currently, there is not enough reliable information available to accurately determine the execution environment and execution platform. Until that is available, change get_execution_context to provide better values for the most common cases.
11756a3
to
37e4018
Compare
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 good to me!
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.
Overall, this looks good and I'd be fine merging it as is. I included some comments (only nits though).
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 making all of the extra changes!
For a description of the change, see: #399
Status