-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Bootstrap test resources when running tests #23652
Conversation
@@ -294,6 +308,11 @@ protected string GetRecordedVariable(string name) | |||
protected string GetRecordedVariable(string name, Action<RecordedVariableOptions> options) | |||
{ | |||
var value = GetRecordedOptionalVariable(name, options); | |||
if (value == null) | |||
{ | |||
BootStrapTestResources(); |
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.
Why do we need these in Get* methods?
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.
How else would we know if bootstrapping is required?
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.
Check that RG exists. Would handle expiring resources as well as non-existant.
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.
That doesn't work - you could have an existent RG and still not have the resources.
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.
You can have an existing variable and no resource as well.
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.
The concern that I have is the experience for external contributors. If I'm trying to get myself running and I encounter this because I'm on Linux or am using the documented/supported approach of environment variables on Windows, the experience isn't awesome. Will I bother to let you know or will I just get frustrated and give up without a word?
I'd prefer that we optimize for not prompting unless we can prove that the environment isn't suitable to run, rather than optimizing for the common use case of the SDK team.
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.
The prompt will only happen on Windows.
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.
Fair enough, but I think the overall concern stands.
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.
Yep, if you are using env vars you may find the behavior annoying if we switch to check for the existence of an env 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.
If the env vars aren't detected (or the file not found - normal behavior now) why not just point them at the instructions on how to run New-TestResources? Adding extra time to check all the time for resources existing will slow tests down even further and some of them cost by the hour. You also can't discern much from incremental deployments. Some resources might support it, while others don't because of the configuration they are in - like resources configured for Customer-Managed Keys (CMK) like sdk/search. You can't redeploy resources in that state incrementally - it messes everything up.
Can we plug this feature in somewhere around
|
We don't know whether to bootstrap until the tests start running. |
What are the scenarios where we don't know? |
Someone has a resouce group env var that is unrelated to the current test suite. The Test Framework thinks the resources are already there. |
Pretty sure we are safe there https://azure.grep.app/search?q=RESOURCE_GROUP&case=true&filter[repo][0]=Azure/azure-sdk-for-net&filter[lang][0]=C%23 Checking for RG also allows us to extend resource expiration time and check for expired resources. I don't think we should overcorrect for people who are advanced enough to be setting env vars. |
Or it should be a parameter to the existing script. |
Did you mean to respond to this comment - #23652 (comment) |
Yep |
[string] $ServiceDirectory, | ||
[string] $BaseName | ||
) | ||
$run = Read-Host "The test resources needed to run the live tests could not be located.`nWould you like to run the resource creation script? [y/n]" |
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.
Is this wrapper even useful? Is it only so you can do the prompting? If so can that be done in the test run 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.
Yes, it is only for the prompting.
I'm not aware of a way to provide an interactive prompt within the test run 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.
If you are not opposed to adding this directly to New-TestResources along with a new switch param, I can do that instead.
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 like keeping it separate for now and I also like it is in a .NET specific script instead of under eng/common.
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them. In order to bootstrap pipelines for a new service, please perform following steps: For data-plane/track 2 SDKs Issue the following command as a pull request comment:
For track 1 management-plane SDKsPlease open a separate PR and to your service SDK path in this file. Once that PR has been merged, you can re-run the pipeline to trigger the verification. |
|
||
private static readonly HashSet<Type> s_bootstrappingAttemptedTypes = new(); | ||
private static readonly object s_syncLock = new(); | ||
private static readonly bool s_isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); |
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.
RuntimeInformation.IsOsPlatform
is not available on net461
or net462
. (ref)
Should we consider using (or falling back to) Envvironment.OSVersion
?
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.
Hmm, I'm not sure how this builds then.
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.
Because we usually don't build tests for those frameworks...but Key Vault because there are differences we need to test for.
4bc3037
to
7bc5784
Compare
Let's try this option as it's a bit safer and see how it goes. |
@@ -0,0 +1,12 @@ | |||
# Copyright (c) Microsoft Corporation. All rights reserved. |
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.
Why is this not in Azure/azure-sdk-tools?
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.
This is only going to be used by the .NET test suite.
|
||
private static readonly HashSet<Type> s_bootstrappingAttemptedTypes = new(); | ||
private static readonly object s_syncLock = new(); | ||
private static readonly bool s_isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); |
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.
Because we usually don't build tests for those frameworks...but Key Vault because there are differences we need to test for.
@@ -294,6 +308,11 @@ protected string GetRecordedVariable(string name) | |||
protected string GetRecordedVariable(string name, Action<RecordedVariableOptions> options) | |||
{ | |||
var value = GetRecordedOptionalVariable(name, options); | |||
if (value == null) | |||
{ | |||
BootStrapTestResources(); |
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.
If the env vars aren't detected (or the file not found - normal behavior now) why not just point them at the instructions on how to run New-TestResources? Adding extra time to check all the time for resources existing will slow tests down even further and some of them cost by the hour. You also can't discern much from incremental deployments. Some resources might support it, while others don't because of the configuration they are in - like resources configured for Customer-Managed Keys (CMK) like sdk/search. You can't redeploy resources in that state incrementally - it messes everything up.
"Make sure the test environment was initialized using the eng/common/TestResources/New-TestResources.ps1 script."; | ||
if (_bootstrappingException != null) | ||
{ | ||
message += Environment.NewLine + "Resource creation failed during the test run. Make sure PowerShell version 6 or higher is installed."; |
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.
Why PowerShell 6? Seems like tight coupling with something else, much like checking if this is Windows. If using env vars, why wouldn't other platforms work?
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.
It says at least powershell 6. This is the same messaging we use in the contributing.md file.
s_bootstrappingAttemptedTypes.Contains(_type) || | ||
Mode == RecordedTestMode.Playback || | ||
GlobalIsRunningInCI) |
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: I'd reorder these with faster checks first - basically, reverse these last three.
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.
GlobalIsRunningInCI does an environment variable lookup whenever invoked.
I know these are cached for the process, but isn't it still a relatively expensive system call?
@heaths replying here as I wasn't able to reply to your comments for some reason:
So why did the core - ci pass? If KeyVault tests run against those frameworks shouldn't they have failed?
This is the current behavior. The idea is to remove the step of manually executing the script and passing the service name.
After the initial prompt to run the script, there should be minimal perf impact - we are just checking a local bool and returning.
If you are dealing with a service like that you can just say that you won't run the script, or when you get to the prompt about an existing resource group, you can just say not to update it. |
In my experience working with partners who've had trouble running this script, doing it automatically won't find much success. First, you need to have permissions to deploy to a sub - something we're fortunate to have but many don't. Even if we went back to the same, slow, user-unfriendly method of documenting non-copy/pastable Azure CLI commands the same is still true. They have to make sure they are logged into the right sub, have the right permissions (might require a sub admin giving them an SP they can use, such as the case with one internal partner), etc. While I understand this PR doesn't fully automate it, I think doing so much cause more confusion if the user isn't at least involved in interactively selecting the right sub they want to use, etc. For example, |
I agree that there the default experience is not always ideal but this feature doesn't make it worse. If users don't have access to create resources both running the script manually or automatically would fail. We are just doing exactly what we are telling customers to do. I agree we should improve the sub selection (make it interactive?) but it's a bit orthogonal. |
Alright, I am going to merge this now. We can keep an eye out for any issues that users experience, and adjust as needed. |
Release of microsoft.network api version: 2022 11 01 (Azure#23652) * Adds base for updating Microsoft.Network from version stable/2022-09-01 to version 2022-11-01 * Updates readme * Updates API version in new specs and examples * remove loadDistributionPolicy from appgw PUT request body example (Azure#23144) * remove loadDistributionPolicy from appgw PUT request body example * remove loadDistributionPolicy from appgw PUT request body example2 * remove loadDistributionPolicy from appgw PUT request body example * remove duplicates * add private link configuration to see if it will fix validation error * remove privateLinkConfig related * revert changes to see if still get validation error * update back to original changes * add response resource id to fix modelValidation error * add connection resource * Selector in Exclusions should not be required as Operator EqualsAny does not need a Selector (Azure#23184) * Application Gateway WAF Rate Limit feature swagger changes (Azure#23021) * Application Gateway WAF Rate Limit feature swagger changes * Fixing lint errors * Updated rate limit feature examples * Fix for applicationGatewayIpConfigurations property and make the priority parameter as required for NSG (Azure#23210) * Fix for applicationGatewayIpConfigurations property This swagger definition is defining property applicationGatewayIpConfigurations, ``` "applicationGatewayIpConfigurations": { "type": "array", "items": { "$ref": "./applicationGateway.json#/definitions/ApplicationGatewayIPConfiguration" }, "description": "Application gateway IP configurations of virtual network resource." } ``` but actual rest api returned applicationGatewayIPConfigurations, the P is upper case while in swagger it is lower case. ``` { "name": "subnet-agw", "id": "/subscriptions/<sub-id>/resourceGroups/<rg-name>/providers/Microsoft.Network/virtualNetworks/<vnet-name>/subnets/subnet-agw", "properties": { "applicationGatewayIPConfigurations": [ { "id": "/subscriptions/<sub-id>/resourceGroups/<rg-name>/providers/Microsoft.Network/applicationGateways/<agw-name>/gatewayIPConfigurations/appGatewayIpConfig" } ] }, "type": "Microsoft.Network/virtualNetworks/subnets" } ``` * Making priority property required [This is the API invoked](https://learn.microsoft.com/en-us/rest/api/virtualnetwork/network-security-groups/create-or-update?tabs=HTTP#code-try-0). For the NSG security rules without priorities, seems like the priority parameter is required. Azure portal works as expected. I don't think that's allowed but according to the schema it is because the priority property is not marked as required. I tried to create a NSG security rule without the priorities and it failed with below. Request Body: ``` { "properties": { "securityRules": [ { "name": "rule1", "properties": { "protocol": "*", "sourceAddressPrefix": "*", "destinationAddressPrefix": "*", "access": "Allow", "destinationPortRange": "80", "sourcePortRange": "*", "direction": "Inbound" } } ] }, "location": "eastus" } ``` Response received: ``` { "error": { "code": "SecurityRuleInvalidPriority", "message": "Security rule has invalid Priority. Value provided: 0 Allowed range 100-4096.", "details": [] } } ``` This PR will make the priority parameter as required. * Update NetworkSecurityGroupDelete.json * Update NetworkSecurityGroupRuleDelete.json * Update NetworkSecurityGroupDelete.json * Undo previous change of making fields optional within Exclusion (Azure#23325) * Nivishenker/http headers to insert (Azure#23295) * new app rule property - http header to insert * prettier * add to 22-11-01 * removed changes from 22-09-01 * fixed example * delete RCG example * fix delete examples * added a Location header * prettier fix * Application Gateway WAF Log scrubbing swagger changes (Azure#23022) * Log scrubbing API changes * Update examples * Addressing comments * Fix * Update property in load balancer for Connection Draining Phase 1 (Azure#23082) * Update property in load balancer for Coneection Draining Phase 1 * Fix model validation * Azure Firewall Packet Capture API. Cancelled PR from 2022-09-01 and created for 2022-11-01 (Azure#23114) * moving packet-capture api to 2022-11-01 release from 2022-09-01 * fixing the LRO response header error for model validation * Changes for Additional nic and corrections (Azure#23522) * Changes for Additional nic and corrections * Fixes for tool reported issues * Fix for SpellCheck, ModelValidation * Defining Location in NetworkVirtualApplianceDelete for async * Minor fix * Minor fix * Minor fix * PrettierCheck fixed * Fixing VirtualApplianceAdditionalNicProperties * ModelValidation fixed * Correcting the location header string * Correcting location header * Prettied Check Fix * Fix * Fix * Add inspection limit changes (Azure#23536) * Fixed headers for examples (Azure#23554) * Fix response of API: List of advertised/learned BGP routes (Azure#23555) * fix response type * fix format * fix example, add x-ms-identifiers * AuxiliarySku on Nic (Azure#23552) * Adding auxiliary sku property on nic * Adding AcceleratedConnections to AuxMode * modifying the auxSku values * Removing accidental change --------- Co-authored-by: Prachi Pravin Bhavsar <prbhavsar@microsoft.com> * migrated latest pr from incorrect branch (Azure#23589) * Changes for NVA connection (Azure#23173) * Changes for NVA connection * Changes to address validation failures * Fix some ModelValidation errors * Fix some ModelValidation errors * Fix some ModelValidation errors * Fix some ModelValidation errors * Fix some ModelValidation errors * Addressing some review comments * Addressing some review comments * Addressing some review comments * Addressing some review comments * Addressing some review comments * Fix camel case for property name * Fixing resourceUri to Camel case in example files * Fix SDK duplication error * Fix one LintDiff error * try fix lintdiff errors * fix lro erro in lintdiff * fix model validation error * Bastion S360 for 2022-11 version (Azure#23595) * Release microsoft.network 2022 11 01 (Azure#23564) * Application Gateway WAF Rate Limit feature swagger changes * Fixing lint errors * Updated rate limit feature examples * rateLimitDuration and rateLimitThreshold are not mandatory. There are only mandatory if ruleType is RateLimitRule. This validation will be done in NRP. * Fix attempt 2. Removed minimum constraint from rateLimitThreshold. Validation will be done in NRP * Remove the minimum number for the option field requestBodyInspectLimitInKB (Azure#23663) * Remove minimum value for optional field requestBodyInspectLimitInKB * Remove exlcusiveMinimum --------- Co-authored-by: htippanaboya <102244326+htippanaboya@users.noreply.github.com> Co-authored-by: tracyMicro <127259533+tracyMicro@users.noreply.github.com> Co-authored-by: tejasshah7 <49326906+tejasshah7@users.noreply.github.com> Co-authored-by: sraghavan-msft <98118843+sraghavan-msft@users.noreply.github.com> Co-authored-by: navba-MSFT <57353862+navba-MSFT@users.noreply.github.com> Co-authored-by: NiviShenker <99583088+NiviShenker@users.noreply.github.com> Co-authored-by: Sindhu Aluguvelli <sindhureddy216@gmail.com> Co-authored-by: phrazfipho <121897152+phrazfipho@users.noreply.github.com> Co-authored-by: nikhilpadhye1 <68977752+nikhilpadhye1@users.noreply.github.com> Co-authored-by: JainRah <108508612+JainRah@users.noreply.github.com> Co-authored-by: karanbazaz <karan.bazaz@gmail.com> Co-authored-by: yeliMSFT <95386492+yeliMSFT@users.noreply.github.com> Co-authored-by: pracsb <78512712+pr-work@users.noreply.github.com> Co-authored-by: Prachi Pravin Bhavsar <prbhavsar@microsoft.com> Co-authored-by: nanditaashok-ms <122581061+nanditaashok-ms@users.noreply.github.com> Co-authored-by: anvrao1 <109107918+anvrao1@users.noreply.github.com> Co-authored-by: baoqihuang0326 <119557638+baoqihuang0326@users.noreply.github.com>
No description provided.