[PostgreSQL] az postgres flexible-server: Improved validation logic and style of error messages#33114
Conversation
- Updated comments in test files to improve clarity and consistency. - Enhanced error messages in validators for better user guidance and understanding. - Standardized phrasing and capitalization in error messages across various validators. - Improved validation logic to ensure clearer feedback for incorrect usage.
|
Validation for Azure CLI Full Test Starting...
Thanks for your contribution! |
|
Validation for Breaking Change Starting...
Thanks for your contribution! |
|
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 improves the az postgres flexible-server user experience by standardizing validator behavior (case-insensitive comparisons) and making validator error messages clearer and more consistent, along with updating test comments for clarity.
Changes:
- Standardize/clarify many validator error messages and make several validations case-insensitive.
- Refine validation logic around elastic clusters, SSDv2 storage, HA/zonal resiliency, maintenance window, public access, etc.
- Update PostgreSQL flexible-server scenario tests with clearer step comments and additional negative-test scenarios.
Reviewed changes
Copilot reviewed 19 out of 48 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/postgresql/utils/validators.py | Updates validator logic and standardizes error message wording/capitalization. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands.py | Improves scenario test comments and expands validator negative-test coverage. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_vnet.py | Comment-only clarity improvements for vnet scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_upgrade.py | Comment-only clarity improvements for upgrade scenario. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_ssdv2.py | Comment-only clarity improvements for SSDv2 scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_server_logs.py | Comment-only clarity improvements for server logs scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_restore.py | Comment-only clarity improvements for restore scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_replica.py | Comment-only clarity improvements for replica/virtual endpoint scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_private_dns_zone.py | Comment updates and scenarios around private DNS zone validation/linking. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_migration.py | Comment-only clarity improvements for migration scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_ltr.py | Comment-only clarity improvements for long-term retention scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_identity_microsoft_entra_admin.py | Comment-only clarity improvements for identity/Entra admin scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_identity_cmk.py | Comment-only clarity improvements for CMK scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_geo_restore.py | Comment-only clarity improvements for geo-restore scenario. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_firewall_rule.py | Comment-only clarity improvements for firewall/public access scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_fabric_mirroring.py | Comment-only clarity improvements for fabric mirroring scenario. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_elastic_clusters.py | Comment-only clarity improvements for elastic cluster scenario. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_backup.py | Comment-only clarity improvements for backup scenarios. |
| src/azure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands_advanced_threat_protection.py | Comment-only clarity improvements for ATP setting scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../command_modules/postgresql/tests/latest/test_postgres_flexible_commands_private_dns_zone.py
Show resolved
Hide resolved
...ure-cli/azure/cli/command_modules/postgresql/tests/latest/test_postgres_flexible_commands.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 48 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/azure-cli/azure/cli/command_modules/postgresql/utils/validators.py
Outdated
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/postgresql/utils/validators.py
Outdated
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/postgresql/utils/validators.py
Outdated
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/postgresql/utils/validators.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 48 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def pg_restore_validator(compute_tier, **args): | ||
| is_ssdv2_enabled = args.get('storage_type', None) == "PremiumV2_LRS" | ||
| is_ssdv2_enabled = args.get('storage_type', None) is not None \ | ||
| and args.get('storage_type').lower() == 'premiumv2_lrs' | ||
|
|
||
| if is_ssdv2_enabled and compute_tier.lower() == 'burstable': | ||
| raise ValidationError("Burstable tier is not supported for servers with Premium SSD V2.") | ||
| raise ValidationError('Invalid value for --tier. Burstable tier is not supported for servers with ' | ||
| '--storage-type set to "PremiumV2_LRS".') |
There was a problem hiding this comment.
In pg_restore_validator, the error message says "Invalid value for --tier", but az postgres flexible-server restore doesn't expose a --tier parameter (the tier comes from the source server). This can confuse users who didn't set a tier. Consider rewording to reference the source server tier / the incompatible --storage-type PremiumV2_LRS combination instead of --tier.
| def _cluster_validator(create_cluster, cluster_size, auto_grow, version, tier, instance): | ||
| if create_cluster == 'ElasticCluster' or (instance and instance.cluster and instance.cluster.cluster_size > 0): | ||
| if create_cluster and create_cluster.lower() == 'elasticcluster' or \ | ||
| (instance and instance.cluster and instance.cluster.cluster_size > 0): | ||
| if instance is None and version != '17': |
There was a problem hiding this comment.
The condition in _cluster_validator mixes and/or without explicit parentheses (if create_cluster and ... or (instance and ...)). Although Python precedence makes this work today, it's easy to misread and accidentally change behavior later. Add parentheses to make the intended grouping explicit.
| # Create server with invalid tier | ||
| self.cmd('postgres flexible-server create -g {} -n {} -l {} --tier {}'.format( | ||
| resource_group, server_name, location, invalid_tier), | ||
| expect_failure=True) | ||
|
|
||
| # Create server with invalid version | ||
| self.cmd('postgres flexible-server create -g {} -n {} -l {} --version {}'.format( | ||
| resource_group, server_name, location, invalid_version), | ||
| expect_failure=True) | ||
|
|
||
| # Create server with invalid sku name | ||
| self.cmd('postgres flexible-server create -g {} -n {} -l {} --tier {} --sku-name {}'.format( | ||
| resource_group, server_name, location, valid_tier, invalid_sku_name), | ||
| expect_failure=True) | ||
|
|
||
| # Create server with invalid backup retention days | ||
| self.cmd('postgres flexible-server create -g {} -n {} -l {} --backup-retention {}'.format( | ||
| resource_group, server_name, location, invalid_backup_retention), | ||
| expect_failure=True) | ||
|
|
||
| # Create server with zone redundant high availability in a location that does not support it, because it's a single zone location | ||
| self.cmd('postgres flexible-server create -g {} -n {} -l {} --high-availability {} '.format( | ||
| resource_group, server_name, location, ha_value), | ||
| expect_failure=True) | ||
|
|
||
| # high availability validator | ||
| # Create server with zone redundant high availability with a tier that does not support it | ||
| self.cmd('postgres flexible-server create -g {} -n {} -l {} --tier Burstable --sku-name Standard_B1ms --high-availability {}'.format( | ||
| resource_group, server_name, location, ha_value), | ||
| expect_failure=True) | ||
|
|
||
| # Create server with zone redundant high availability in a location that does not support it, because it's a single zone location | ||
| self.cmd('postgres flexible-server create -g {} -n {} -l {} --tier GeneralPurpose --sku-name Standard_D4ds_v4 --high-availability {}'.format( | ||
| resource_group, server_name, location, ha_value), # single availability zone location | ||
| resource_group, server_name, location, ha_value), | ||
| expect_failure=True) | ||
|
|
||
| # Create server with zone redundant high availability and forcing same zone for primary and standby | ||
| self.cmd('postgres flexible-server create -g {} -n {} -l {} --tier GeneralPurpose --sku-name Standard_D2ads_v5 --high-availability {} --zone 1 --standby-zone 1'.format( | ||
| resource_group, server_name, location, ha_value), # single availability zone location | ||
| resource_group, server_name, location, ha_value), | ||
| expect_failure=True) |
There was a problem hiding this comment.
In _test_mgmt_create_validator, multiple postgres flexible-server create calls reuse the same server_name and only assert expect_failure=True. If any command unexpectedly succeeds (e.g., due to capability changes), subsequent creates may fail due to name collision and still satisfy expect_failure, masking the real regression and potentially leaking a live server. Consider generating a unique name per negative case and/or asserting on the specific validation error text.
| self.cmd('network private-dns link vnet create -g {} -n MyLinkName -z {} -v {} -e False'.format( | ||
| vnet_resource_group, vnet_group_dns_zone, vnet_group_vnet['id'] | ||
| )) | ||
|
|
||
| # Create server with the private DNS zone that is linked to the vnet in a different resource group |
There was a problem hiding this comment.
There appears to be trailing whitespace on the blank line after the network private-dns link vnet create call, which can cause noisy diffs and may fail whitespace-sensitive linters. Remove the extra spaces so the line is truly blank.
Related command
az postgres flexible-serverDescription
Provided consistency to validations (now all are case-insensitive), and consistent style to error messages produced from validators/
Testing Guide
Ran all tests in live mode to get new recordings and make sure that they all pass.
History Notes
[PostgreSQL]
az postgres flexible-server: Improved validation logic and style of error messagesThis 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.