Skip to content
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

Pass authorization token to cosmos client in mock_transport_framework test #382

Merged
merged 2 commits into from
Oct 12, 2021

Conversation

jddarby
Copy link
Contributor

@jddarby jddarby commented Oct 11, 2021

No description provided.

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but we need to adjust how we handle this in REPLAY mode

Comment on lines +19 to +22
let key =
std::env::var("COSMOS_MASTER_KEY").expect("Set env variable COSMOS_MASTER_KEY first!");

AuthorizationToken::primary_from_base64(&key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also don't want to require that the auth key is set when running the tests on replay mode. Similar to how we're first checking for the TESTING_MODE_KEY before getting the account name, we should do the same for the authorization token.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have tried to do something similar. Handling the Result type was a bit more fiddly but let me know if there is a better way to do it.

@@ -24,8 +30,15 @@ pub fn initialize(
== Ok(azure_core::TESTING_MODE_RECORD))
.then(get_account)
.unwrap_or_else(String::new);
let authorization_token = (std::env::var(azure_core::TESTING_MODE_KEY).as_deref()
== Ok(azure_core::TESTING_MODE_RECORD))
.then(|| get_authorization_token().ok())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better for us to just unwrap the error. If the testing mode is set to record but we can't parse the auth token, that's a bug.

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Thanks. There might be some ways we can improve on this, but it's a great start.

@rylev rylev merged commit 0aaf4ee into Azure:main Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants