-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Text Analytics ARM template for Live tests #7638
Conversation
…into arm-temp-ta
The provided ARM template was successfully able to deploy and create the required resources https://dev.azure.com/azure-sdk/internal/_build/results?buildId=251491&view=logs&j=4d5db6ce-0b7f-527e-b115-2367ee6e1fef&t=4d6ba9ee-eb4f-5b17-ddd3-b2e0809fa897&l=93. Although, the tests are still failing due to service side bugs, that they are currently working on. |
"AZURE_TENANT_ID": { | ||
"type": "String", | ||
"value": "[parameters('tenantId')]" | ||
}, | ||
"AZURE_CLIENT_ID": { | ||
"type": "String", | ||
"value": "[parameters('testApplicationId')]" | ||
}, | ||
"AZURE_CLIENT_SECRET": { | ||
"type": "String", | ||
"value": "[parameters('testApplicationSecret')]" | ||
}, |
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.
Let's continue to use the tests.yml
to handle loading these, no need to output these here.
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 think the more recommended approach is to only maintain test variables in the tests-resources file as that can serve as a central place for all that info.
@danieljurek Can add more context.
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.
In the case of the variables being passed through the ARM template, this gives us one location to specify environment variables for a live test (compared with mixing environment variables defined in the ARM template and those defined in the tests.yml
file).
Since the ARM template is meant to be used with New-TestResources.ps1
it is more convenient for contributors to specify the environment variables in one place. The script outputs the set of environment variables needed to run tests and doesn't require the contributor to know to consult the tests.yml
file.
Either approach works but over time I've convinced myself that it makes sense to collect the environment variables in one place for the benefit of contributor experience.
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.
As of now, I have used test-resources.json
to declare variables. Few other languages are also doing it. I think this is one think does not need to differ from other languages and have one common place for variables. So I will vote for keeping in this file.
"AZURE_TENANT_ID": { | ||
"type": "String", | ||
"value": "[parameters('tenantId')]" | ||
}, | ||
"AZURE_CLIENT_ID": { | ||
"type": "String", | ||
"value": "[parameters('testApplicationId')]" | ||
}, | ||
"AZURE_CLIENT_SECRET": { | ||
"type": "String", | ||
"value": "[parameters('testApplicationSecret')]" | ||
}, |
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.
As of now, I have used test-resources.json
to declare variables. Few other languages are also doing it. I think this is one think does not need to differ from other languages and have one common place for variables. So I will vote for keeping in this file.
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.
resource provisioning changes LGTM.
Of note: the number of test failures appears to have decreased, possibly a result of this change.
- Nightly build from this morning not including these changes (84.2% passed): https://dev.azure.com/azure-sdk/internal/_build/results?buildId=252409&view=ms.vss-test-web.build-test-results-tab
- Manual build from yesterday which includes these changes (94.6% passed): https://dev.azure.com/azure-sdk/internal/_build/results?buildId=251491&view=ms.vss-test-web.build-test-results-tab
We have #7069 open to investigate the live test's failure. |
This PR introduces an ARM template for running live tests for Text Analytics client.
The pipeline will run ARM template and create common resources using the variables mentioned in the JSON. Then use the output environment variables for testing. This ARM template should also delete the unwanted resources after the pipeline is successfully completed.
Closes: #7513