Repository navigation
[Storage] az storage account create/update: Add --zones and --zone-placement-policy to support setting zones and availability zone pinning policy - #32192
Conversation
…nt-policy`, zone-placement-policy testing fails now
️✔️AzureCLI-FullTest
|
|
Hi Zhiyi Huang (@calvinhzy), |
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| storage account create | cmd storage account create added parameter zone_placement_policy |
||
| storage account create | cmd storage account create added parameter zones |
||
| storage account update | cmd storage account update added parameter zone_placement_policy |
||
| storage account update | cmd storage account update added parameter zones |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds availability zone support to Azure Storage account commands by introducing two new parameters: --zones and --zone-placement-policy for both az storage account create and az storage account update commands.
- Adds zone configuration options to enable availability zone support for storage accounts
- Implements zone placement policy to control how storage accounts are distributed across availability zones
- Includes comprehensive test coverage for both creation and update scenarios with zone configurations
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| _params.py | Adds parameter definitions for --zones and --zone-placement-policy arguments |
| account.py | Implements zone and placement policy handling in create/update operations |
| test_storage_account_scenarios.py | Adds comprehensive test cases for zone and placement policy functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def test_storage_account_zone_with_placement(self): | ||
| self.kwargs.update({ | ||
| 'rg': 'yifantestzp', |
There was a problem hiding this comment.
The hardcoded resource group name 'yifantestzp' appears to be a personal test resource group. This should be replaced with a dynamically created or properly named test resource group to ensure tests are portable and don't depend on specific environments.
| def test_storage_account_zone_with_placement(self): | |
| self.kwargs.update({ | |
| 'rg': 'yifantestzp', | |
| @ResourceGroupPreparer(name_prefix='cli_test_storage_zone_placement') | |
| def test_storage_account_zone_with_placement(self, resource_group): | |
| self.kwargs.update({ | |
| 'rg': resource_group, |
| t_account_type, t_sku_name, t_kind, t_tls_version, t_dns_endpoint_type, t_zone_placement_policy = \ | ||
| self.get_models('AccountType', 'SkuName', 'Kind', 'MinimumTlsVersion', 'DnsEndpointType', | ||
| 'ZonePlacementPolicy', | ||
| resource_type=ResourceType.MGMT_STORAGE) |
There was a problem hiding this comment.
[nitpick] The tuple unpacking on line 325 is getting very long with 6 variables. Consider breaking this into multiple lines or using separate get_models calls for better readability and maintainability.
| t_account_type, t_sku_name, t_kind, t_tls_version, t_dns_endpoint_type, t_zone_placement_policy = \ | |
| self.get_models('AccountType', 'SkuName', 'Kind', 'MinimumTlsVersion', 'DnsEndpointType', | |
| 'ZonePlacementPolicy', | |
| resource_type=ResourceType.MGMT_STORAGE) | |
| t_account_type = self.get_models('AccountType', resource_type=ResourceType.MGMT_STORAGE) | |
| t_sku_name = self.get_models('SkuName', resource_type=ResourceType.MGMT_STORAGE) | |
| t_kind = self.get_models('Kind', resource_type=ResourceType.MGMT_STORAGE) | |
| t_tls_version = self.get_models('MinimumTlsVersion', resource_type=ResourceType.MGMT_STORAGE) | |
| t_dns_endpoint_type = self.get_models('DnsEndpointType', resource_type=ResourceType.MGMT_STORAGE) | |
| t_zone_placement_policy = self.get_models('ZonePlacementPolicy', resource_type=ResourceType.MGMT_STORAGE) |
| c.argument('zones', nargs='+', | ||
| help='Describes the available zones for the product where storage account resource can be created.') |
There was a problem hiding this comment.
The help text for 'zones' is duplicated between create and update contexts (lines 440-441 and 538-539). Consider extracting this to a constant to avoid duplication and ensure consistency.
| c.argument('zone_placement_policy', arg_type=get_enum_type(t_zone_placement_policy), | ||
| help='The availability zone pinning policy for the storage account.') |
There was a problem hiding this comment.
The help text for 'zones' is duplicated between create and update contexts (lines 440-441 and 538-539). Consider extracting this to a constant to avoid duplication and ensure consistency.
Related command
Description
Add
--zonesand--zone-placement-policywhen creating/updating storage account to support setting zones and availability zone pinning policyTesting Guide
History Notes
[Storage]
az storage account create/update: Add--zonesand--zone-placement-policyto support setting zones and availability zone pinning policyThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.