[SQL] Introduced changes to cli commands to support self-server restore of sql logical server #32245
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
❌AzureCLI-FullTest
|
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| sql server create | cmd sql server create added parameter enable_soft_delete |
||
| sql server create | cmd sql server create added parameter soft_delete_retention_days |
||
| sql server deleted-server | sub group sql server deleted-server added |
||
| sql server restore | cmd sql server restore added |
||
| sql server update | cmd sql server update added parameter enable_soft_delete |
||
| sql server update | cmd sql server update added parameter soft_delete_retention_days |
|
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>
|
microsoft-github-policy-service
bot
requested review from
calvinhzy,
evelyn-ys and
yonzhan
microsoft-github-policy-service
bot
added
the
customer-reported
|
Thank you for your contribution @rambabu-yalla! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
added
Auto-Assign
rambabu-yalla
force-pushed
the
ramyal/server-restore
branch
from
bb903a5 to
da66134
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces changes to support SQL Server soft delete functionality, adding new commands and parameters for server creation, update, and restoration operations. The changes enable users to create SQL servers with soft delete protection, modify soft delete settings on existing servers, and restore previously deleted servers.
- Adds new
az sql server restorecommand for recovering deleted SQL servers - Extends
az sql server createandaz sql server updatecommands with soft delete parameters - Implements comprehensive validation for soft delete retention days (1-7 days when enabled, 0 when disabled)
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test_sql_commands.py | Added comprehensive test suite for soft delete functionality with multiple scenarios |
| test recordings (2 files) | Added test recording files for new soft delete test cases |
| custom.py | Implemented server restore functionality and soft delete parameter handling |
| commands.py | Added new restore command to SQL server command group |
| _validators.py | Added validation logic for soft delete parameters with proper error handling |
| _util.py | Added utility function to access deleted servers API operations |
| _params.py | Added command line parameters for soft delete options |
| _help.py | Added help documentation and examples for new soft delete features |
Comments suppressed due to low confidence (2)
src/azure-cli/azure/cli/command_modules/sql/_help.py:1
- The help message should start with a verb in active voice. Consider changing 'Set whether soft delete is enabled or not' to 'Enable or disable soft delete protection' to follow the help message format guidelines.
# coding=utf-8
src/azure-cli/azure/cli/command_modules/sql/_help.py:1
- The help message should start with a verb in active voice. Consider changing 'The number of days to retain soft deleted resources' to 'Specify the number of days to retain soft deleted resources' to follow the help message format guidelines.
# coding=utf-8
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/azure-cli/azure/cli/command_modules/sql/tests/latest/test_sql_commands.py
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/sql/tests/latest/test_sql_commands.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/azure-cli/azure/cli/command_modules/sql/tests/latest/test_sql_commands.py
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/sql/tests/latest/test_sql_commands.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/azure-cli/azure/cli/command_modules/sql/custom.py:4436
- The docstring uses triple single quotes instead of triple double quotes. Python convention is to use triple double quotes for docstrings.
'''
Creates a server.
'''
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Can you please add negative test cases for each validation scenario related to options enable soft delete and retention days? |
Added. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
1813
to
1844
| - name: Enable soft delete protection with 7-day retention. | ||
| text: az sql server update --name MyAzureSQLServer --resource-group MyResourceGroup --enable-soft-delete | ||
| - name: Modify soft delete retention period. | ||
| text: az sql server update --name MyAzureSQLServer --resource-group MyResourceGroup --enable-soft-delete --soft-delete-retention-days 5 | ||
| - name: Disable soft delete protection. | ||
| text: az sql server update --name MyAzureSQLServer --resource-group MyResourceGroup --enable-soft-delete false |
There was a problem hiding this comment.
The help messages do not follow the guideline to start with an active, first-person verb. According to custom coding guideline 855, help messages starting with a verb should be in first person active voice (e.g., 'Enable' should be used instead of 'Enables'). However, these help messages currently start with an active verb in the imperative form, which is actually correct. The issue is the passive construction. Change to: 'Enable soft delete protection with 7-day retention.', 'Modify soft delete retention period.', 'Disable soft delete protection.' (These are already correct - no change needed).
| - name: Create a server with disabled public network access to server. | ||
| text: az sql server create -l westus -g mygroup -n myserver -u myadminuser -p myadminpassword -e false | ||
| - name: Create a server with soft delete enabled with default retention days. | ||
| text: az sql server create -l westus -g mygroup -n myserver -u myadminuser -p myadminpassword --enable-soft-delete |
There was a problem hiding this comment.
The help message does not follow the guideline to start with an active, first-person verb. The message 'Create a server with soft delete enabled with default retention days.' should be in active voice. However, reviewing more carefully, this help text is already in active voice and first person (imperative). The issue noted in guideline 855 would apply if it were written as 'Creates a server...' (third person) or 'Enabled...' (passive). The current form 'Create...' is correct.
Comment on lines
187
to
189
| raise RequiredArgumentMissingError( | ||
| 'The --soft-delete-retention-days parameter requires --enable-soft-delete to be specified.', | ||
| 'Please specify both --enable-soft-delete and --soft-delete-retention-days together.') |
There was a problem hiding this comment.
This error handling block is duplicated from lines 158-160. Consider extracting this into a helper function or constant to avoid code duplication and improve maintainability.
Comment on lines
4481
to
4516
| if soft_delete_retention_days is not None: | ||
| kwargs['retention_days'] = soft_delete_retention_days | ||
| else: | ||
| kwargs['retention_days'] = 7 |
There was a problem hiding this comment.
The default retention days value (7) is duplicated in multiple locations (also in server_update at line 4624 and in validator comments). Consider defining this as a named constant at the module level to improve maintainability and ensure consistency.
Comment on lines
+575
to
+578
| g.custom_command('restore', 'server_restore', | ||
| table_transformer=server_table_format, | ||
| supports_no_wait=True) |
There was a problem hiding this comment.
The restore command is a dangerous operation that could potentially restore a deleted server and overwrite data. According to custom coding guideline 1388, dangerous operations like delete, remove, and stop should include confirmation=True. While restore is different from delete, it may have significant consequences. Consider adding confirmation=True to prompt users before executing this operation.
rambabu-yalla
force-pushed
the
ramyal/server-restore
branch
from
297b15e to
470c332
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
171
to
184
| # Validate range based on enable_soft_delete value | ||
| if enable_soft_delete is True: | ||
| # When enable_soft_delete is true, retention days must be 1-7 | ||
| if not 1 <= retention_days <= 7: | ||
| raise InvalidArgumentValueError( | ||
| 'The --soft-delete-retention-days value must be between 1 and 7 (inclusive) ' | ||
| 'when --enable-soft-delete is true.', | ||
| 'Please specify a value between 1 and 7 days.') | ||
| elif enable_soft_delete is False: | ||
| # When enable_soft_delete is false, retention days must be 0 | ||
| if retention_days != 0: | ||
| raise InvalidArgumentValueError( | ||
| 'The --soft-delete-retention-days value must be 0 when --enable-soft-delete is false.', | ||
| 'Please set --soft-delete-retention-days to 0 or remove it when disabling soft delete.') |
There was a problem hiding this comment.
[nitpick] The validation logic has duplicated range checking code that could be consolidated. The validation for enable_soft_delete is True and enable_soft_delete is False could be combined into a single conditional block using a dictionary or helper function to map expected ranges.
Suggested change
| # Validate range based on enable_soft_delete value | |
| if enable_soft_delete is True: | |
| # When enable_soft_delete is true, retention days must be 1-7 | |
| if not 1 <= retention_days <= 7: | |
| raise InvalidArgumentValueError( | |
| 'The --soft-delete-retention-days value must be between 1 and 7 (inclusive) ' | |
| 'when --enable-soft-delete is true.', | |
| 'Please specify a value between 1 and 7 days.') | |
| elif enable_soft_delete is False: | |
| # When enable_soft_delete is false, retention days must be 0 | |
| if retention_days != 0: | |
| raise InvalidArgumentValueError( | |
| 'The --soft-delete-retention-days value must be 0 when --enable-soft-delete is false.', | |
| 'Please set --soft-delete-retention-days to 0 or remove it when disabling soft delete.') | |
| # Validate range based on enable_soft_delete value using a mapping | |
| expected_ranges = { | |
| True: lambda v: 1 <= v <= 7, | |
| False: lambda v: v == 0 | |
| } | |
| error_messages = { | |
| True: ( | |
| 'The --soft-delete-retention-days value must be between 1 and 7 (inclusive) ' | |
| 'when --enable-soft-delete is true.', | |
| 'Please specify a value between 1 and 7 days.' | |
| ), | |
| False: ( | |
| 'The --soft-delete-retention-days value must be 0 when --enable-soft-delete is false.', | |
| 'Please set --soft-delete-retention-days to 0 or remove it when disabling soft delete.' | |
| ) | |
| } | |
| if enable_soft_delete in expected_ranges: | |
| if not expected_ranges[enable_soft_delete](retention_days): | |
| raise InvalidArgumentValueError(*error_messages[enable_soft_delete]) |
Comment on lines
189
to
229
| def _check_server_exists(client, server_name): | ||
| ''' | ||
| Checks if a server already exists in the subscription and raises ValidationError if it does. | ||
| Searches across all servers in the subscription to find any server with the given name. | ||
| Returns False if server doesn't exist, raises ValidationError if it exists. | ||
| ''' | ||
| try: | ||
| # List all servers in the subscription | ||
| all_servers = list(client.list()) | ||
|
|
||
| # Check if any server matches the given server name (case-insensitive) | ||
| if any(server.name.lower() == server_name.lower() for server in all_servers): | ||
| raise ValidationError( | ||
| f'Server "{server_name}" already exists in the subscription. ' | ||
| 'Cannot restore to an existing server name.') | ||
|
|
||
| # No matching server found - this is what we want for restore | ||
| return False | ||
|
|
||
| except ValidationError: | ||
| # Re-raise ValidationError as-is | ||
| raise | ||
| except Exception as ex: | ||
| # If server doesn't exist, that's what we want - continue with restore | ||
| if 'ResourceNotFound' not in str(ex) and 'NotFound' not in str(ex): | ||
| # Re-raise if it's not a "not found" error | ||
| raise | ||
| return False |
There was a problem hiding this comment.
Listing all servers in the subscription to check for existence is inefficient. This could cause performance issues in subscriptions with many servers. Consider using the server's get method instead, which would return a 404 if the server doesn't exist.
Suggested change
| def _check_server_exists(client, server_name): | |
| ''' | |
| Checks if a server already exists in the subscription and raises ValidationError if it does. | |
| Searches across all servers in the subscription to find any server with the given name. | |
| Returns False if server doesn't exist, raises ValidationError if it exists. | |
| ''' | |
| try: | |
| # List all servers in the subscription | |
| all_servers = list(client.list()) | |
| # Check if any server matches the given server name (case-insensitive) | |
| if any(server.name.lower() == server_name.lower() for server in all_servers): | |
| raise ValidationError( | |
| f'Server "{server_name}" already exists in the subscription. ' | |
| 'Cannot restore to an existing server name.') | |
| # No matching server found - this is what we want for restore | |
| return False | |
| except ValidationError: | |
| # Re-raise ValidationError as-is | |
| raise | |
| except Exception as ex: | |
| # If server doesn't exist, that's what we want - continue with restore | |
| if 'ResourceNotFound' not in str(ex) and 'NotFound' not in str(ex): | |
| # Re-raise if it's not a "not found" error | |
| raise | |
| return False | |
| def _check_server_exists(client, server_name, resource_group_name): | |
| ''' | |
| Checks if a server already exists in the subscription and raises ValidationError if it does. | |
| Uses the get method to check for existence efficiently. | |
| Returns False if server doesn't exist, raises ValidationError if it exists. | |
| ''' | |
| try: | |
| # Try to get the server directly | |
| server = client.get(server_name=server_name, resource_group_name=resource_group_name) | |
| if server: | |
| raise ValidationError( | |
| f'Server "{server_name}" already exists in the subscription. ' | |
| 'Cannot restore to an existing server name.') | |
| return False | |
| except ValidationError: | |
| # Re-raise ValidationError as-is | |
| raise | |
| except Exception as ex: | |
| # If server doesn't exist, that's what we want - continue with restore | |
| if 'ResourceNotFound' in str(ex) or 'NotFound' in str(ex): | |
| return False | |
| # Re-raise if it's not a "not found" error | |
| raise |
Comment on lines
1917
to
1918
| help='Enable or disable soft delete retention. When true,' | ||
| 'the soft delete is enabled for 7 days by default.') |
There was a problem hiding this comment.
The help text has inconsistent spacing and could be clearer. The phrase "When true, the soft delete is enabled for 7 days by default" should have a space after the comma on line 1917. Additionally, the help text should clarify that this sets the retention period, not just "enables soft delete retention."
Suggested change
| help='Enable or disable soft delete retention. When true,' | |
| 'the soft delete is enabled for 7 days by default.') | |
| help='Enable or disable soft delete retention. When true, soft delete retention is enabled and the retention period is set to 7 days by default. You can customize the retention period using --soft-delete-retention-days.') |
Comment on lines
9197
to
9198
| # Wait for the deleted server to appear in the deleted servers list (Azure API propagation delay) | ||
| time.sleep(60) |
There was a problem hiding this comment.
Using a hard-coded sleep of 60 seconds in tests is problematic for test reliability and execution time. Consider implementing a retry mechanism with a timeout instead, polling the deleted servers list until the server appears or a maximum wait time is exceeded.
Comment on lines
1837
to
1840
| - name: Enable soft delete protection with 7-day retention. | ||
| text: az sql server update --name MyAzureSQLServer --resource-group MyResourceGroup --enable-soft-delete | ||
| - name: Modify soft delete retention period. | ||
| text: az sql server update --name MyAzureSQLServer --resource-group MyResourceGroup --enable-soft-delete --soft-delete-retention-days 5 |
There was a problem hiding this comment.
[nitpick] The examples for sql server update show both long-form and short-form options, but the help text doesn't explain when to use each. Consider adding a brief note about the short aliases (--esd, --sdrd) being equivalent alternatives to the full parameter names.
rambabu-yalla
force-pushed
the
ramyal/server-restore
branch
from
0d8cb16 to
8b28d2e
Compare
pjpatel12
reviewed
Oct 24, 2025
pjpatel12
approved these changes
Oct 24, 2025
| with self.argument_context('sql server deleted-server list') as c: | ||
| c.argument('location', arg_type=get_location_type(self.cli_ctx), | ||
| required=True, | ||
| help='Location to list deleted servers from.') |
There was a problem hiding this comment.
"location to list deleted servers from" is odd phrasing.
Perhaps "List deleted servers in this location"?
evelyn-ys
reviewed
Oct 27, 2025
Comment on lines
+791
to
+794
| with self.command_group('sql server deleted-server', deleted_servers_operations, | ||
| client_factory=get_sql_deleted_servers_operations) as g: | ||
| g.custom_show_command('show', 'deleted_server_show') | ||
| g.custom_command('list', 'deleted_server_list') |
There was a problem hiding this comment.
How about az sql server list-deleted/show-deleted. There's no need to add another sub command group deleted-server.
There was a problem hiding this comment.
The API used to retrieve deleted servers is different from the standard servers API, and its response is only a subset of the full servers response. For this reason, we implemented it as a separate subgroup rather than adding list-deleted or show-deleted commands under the main group.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
rambabu-yalla
changed the title
rambabu-yalla
force-pushed
the
ramyal/server-restore
branch
from
8b28d2e to
a8b42f5
Compare
|
@evelyn-ys : Please do not merge the PR. |
MichaelEbert
approved these changes
Oct 27, 2025
evelyn-ys
modified the milestones:
November 2025 (2025-11-04),
November 2025 (2025-11-18)❗ Breaking change release ❗
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
@rambabu-yalla You can mark this PR as draft to prevent it from reviewing/merging |
pranavathalye
approved these changes
Oct 30, 2025
|
Please fix CI issues |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related command
The change includes
Add new command: az sql server restore,
Modify command: az sql server create
Modify command: az sql server update
- Added new parameters --enable-soft-delete and --soft-delete-retention-days
- az sql server update --name MyAzureSQLServer --resource-group MyResourceGroup --enable-soft-delete --soft-delete-retention-days 5
Add new command: az sql server deleted-server show
- az sql server deleted-server show --name servername --location eastasia
Add new command: az sql server deleted-server list
- az sql server deleted-server list --location eastasia
Description
This is to support soft delete public preview feature
Testing Guide
Create a server with soft delete enabled with default retention days, 7 days.
az sql server create -l westus -g mygroup -n myserver -u myadminuser -p myadminpassword --enable-soft-delete
Create a server with soft delete enabled with custom retention days.
az sql server create -l westus -g mygroup -n myserver -u myadminuser -p myadminpassword --minimal-tls-version 1.2 --enable-soft-delete --soft-delete-retention-days 3
Enable soft delete protection with 7-day retention.
az sql server update --name MyAzureSQLServer --resource-group MyResourceGroup --enable-soft-delete
Modify soft delete retention period.
az sql server update --name MyAzureSQLServer --resource-group MyResourceGroup --enable-soft-delete --soft-delete-retention-days 5
Disable soft delete protection.
az sql server update --name MyAzureSQLServer --resource-group MyResourceGroup --enable-soft-delete false
Restore a deleted server.
az sql server restore -g mygroup -n myserver -l westus2
This 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.