-
Notifications
You must be signed in to change notification settings - Fork 283
use TokenCredential in services #59
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
Conversation
@@ -34,7 +34,7 @@ async-trait = "0.1.36" | |||
oauth2 = { version = "4.0.0-alpha.2" } | |||
|
|||
[dev-dependencies] | |||
tokio = "0.3" | |||
tokio = "0.2" |
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.
I got some runtime errors regarding the hyper dependency. I think we may need to wait for hyper to upgrade.
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.
- hyper v0.14 Upgrade to Tokio v0.3 hyperium/hyper#2302
- reqwest ? Update
tokio
to 1.0 seanmonstar/reqwest#1060
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.
I created #60 to track upgrading.
services/mgmt/resources/src/lib.rs
Outdated
pub bearer_access_token: Option<String>, | ||
pub bearer_access_token: Option<String>, // TODO remove | ||
pub token_credential: Option<Box<dyn azure_core::TokenCredential>>, | ||
pub token_credential_scope: 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.
Shouldn't it be a list of scopes?
https://docs.microsoft.com/en-us/azure/active-directory/develop/msal-acquire-cache-tokens#scopes-when-acquiring-tokens
For v2.0, it is still in the scope
http query param, but it is a space separated list. Let's looks and see what other SDKs do.
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.
I added an issue to review this in the not too distant future: #61
It may be easiest to review the AutoRust changes for this in ctaggart/autorust#69. |
@rylev, can I get an approval of this one? |
Please review for #53. I made these changes manually to update
cargo run --example group_create
so that it works withTokenCredential
. Once we figure out the design, I'll update the AutoRust code gen.