Skip to content

Conversation

@seankane-msft
Copy link
Contributor

No description provided.

@check-enforcer

This comment has been minimized.

InjectedPackages: ${{ parameters.InjectedPackages }}
BuildDocs: ${{parameters.BuildDocs}}
# figure out what's up with the create resource group above
- ${{ if eq(parameters['AllocateResourceGroup'], 'false') }}:
Copy link
Member

Choose a reason for hiding this comment

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

We might want to add another parameter something like DeployArmTemplate or something like that as I don't think AllocateResourceGroup=false really tells folks what is happening. I think a better name will help folks understand what is happening. You can keep the AllocateResourceGroup for back-compat (assuming there is someone explicitly setting it), but in either case we probably should make the deployment model mutually exclusive so we don't end up deploying via both mechanisms.

BuildTargetingString: azure-data-tables
ServiceDirectory: tables
AllocateResourceGroup: 'false'
DeployArmTemplate: 'true'
Copy link
Member

Choose a reason for hiding this comment

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

We should remove the AllocateResourceGroup false here and just by the fact someone set DeployArmTemplate we know not to allocate the resource group. (Similarly in the other tests.yml).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this specific library, both the ResourceGroup and the ArmTemplate are used for testing. I will work to making them all dependent on the ArmTemplate, but there are ~20 test files for Tables that I would need to fix. This will be done in a separate PR unless absolutely adamant about having one or the other.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... so tables is using both the arm template and the allocate resource group? If that is the case then why is allocate resource group set to false? If there are cases were both are needed then I'm find cleaning them up later, but I think it would be good to get everything using one or the other. In fact the next step could be to completely eliminate the AllocationResoureGroup step and replace it with a common template that only setups up a resource group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm...you're right I was misunderstanding. The Preparers go through and create resource groups/storage accounts.
To your other points, libraries will use one or the other (either the ArmTemplate in combo with the PowerShellPreparer or the specialized Preparers for the specific service). I can open tracking issues into what libraries still rely on the AllocateResourceGroup and work on replacing with a common template.

@seankane-msft
Copy link
Contributor Author

/azp run python - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@seankane-msft
Copy link
Contributor Author

/azp run python - scemaregistry- tests

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@seankane-msft seankane-msft merged commit b9a06fe into Azure:master Dec 4, 2020
@seankane-msft seankane-msft deleted the powershell-script branch December 4, 2020 22:33
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Jul 16, 2021
msyyc pushed a commit that referenced this pull request Jul 20, 2021
* CodeGen from PR 15227 in Azure/azure-rest-api-specs
config (#15227)

* version,CHANGELOG

Co-authored-by: SDKAuto <[email protected]>
Co-authored-by: PythonSdkPipelines <PythonSdkPipelines>
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.

4 participants