-
Notifications
You must be signed in to change notification settings - Fork 503
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
Integration tests use DevOps MI for Cosmos #4369
Conversation
dec47bc
to
db42bec
Compare
cf84ece
to
925dbc4
Compare
06bb04e
to
9743303
Compare
9743303
to
f24ee36
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.
Some small comments but approved. Nice work. 🦾
@@ -692,20 +693,6 @@ | |||
"[resourceId('Microsoft.DocumentDb/databaseAccounts', variables('serviceName'))]" | |||
] | |||
}, | |||
{ | |||
"condition": "[equals(parameters('solutionType'),'FhirServerCosmosDB')]", | |||
"type": "Microsoft.KeyVault/vaults/secrets", |
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.
NIT: Do we even need the keyvault anymore? Could be a new story.
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.
SQL is still using the keyvault too, it is at least consisntent. Might merge this first so I can mitigate having localauth enabled
@@ -39,16 +40,19 @@ public FhirCosmosClientInitializer( | |||
ICosmosClientTestProvider testProvider, | |||
Func<IEnumerable<RequestHandler>> requestHandlerFactory, | |||
RetryExceptionPolicyFactory retryExceptionPolicyFactory, | |||
Lazy<TokenCredential> tokenCredential, |
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.
Does this need to be Lazy? I think the implementations of TokenCredential
already are lazy and the wait is when the token is requested.
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.
...good point, I was thinking to not resolve it at all if the key is still being used. Not sure I have a strong opinion on this
Key = Environment.GetEnvironmentVariable("CosmosDb:Key") ?? CosmosDbLocalEmulator.Key, | ||
DatabaseId = Environment.GetEnvironmentVariable("CosmosDb:DatabaseId") ?? "FhirTests", | ||
Host = Environment.GetEnvironmentVariable("CosmosDb__Host") ?? CosmosDbLocalEmulator.Host, | ||
Key = Environment.GetEnvironmentVariable("CosmosDb__Key") ?? CosmosDbLocalEmulator.Key, |
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.
NIT: Should key be null if we are using MI?
Description
PR and CI pipelines should use MI for Integration Tests
Related issues
Addresses AB#126615
Testing
Existing tests should pass, using new MI credentials for Cosmos
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)