[SQL] Clean up server update handling for soft-delete - #33933
Open
rambabu-yalla wants to merge 2 commits into
Open
[SQL] Clean up server update handling for soft-delete#33933rambabu-yalla wants to merge 2 commits into
rambabu-yalla wants to merge 2 commits into
Conversation
|
Hi rambabu-yalla, |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
microsoft-github-policy-service
Bot
requested a review
from Yong Zhang (yonzhan)
August 19, 2026 17:37
microsoft-github-policy-service
Bot
added
Auto-Assign
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Azure SQL command module internals to preserve retentionDays (soft-delete retention) during az sql server update when --soft-delete-retention-days is not provided, and adjusts deleted-server listing behavior/tests and their recordings accordingly.
Changes:
- Update
server_updateto preserve the existingretentionDaysvalue unless--soft-delete-retention-daysis explicitly passed. - Adjust
deleted_server_listto treat an explicitly provided empty--location ""as “location-scoped” (instead of falling back to subscription-wide listing). - Refresh/strengthen SQL soft-delete tests and update associated recordings (including location changes and updated CLI/sdk metadata).
Reviewed changes
Copilot reviewed 6 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/sql/custom.py |
Preserves retentionDays on update; changes deleted-server list branching for location. |
src/azure-cli/azure/cli/command_modules/sql/tests/latest/test_sql_commands.py |
Updates/expands soft-delete lifecycle tests and adjusts test locations/expectations. |
src/azure-cli/azure/cli/command_modules/sql/tests/latest/recordings/test_sql_server_create_without_soft_delete.yaml |
Updates recording for create-without-soft-delete scenario (westus + updated headers/timestamps). |
src/azure-cli/azure/cli/command_modules/sql/tests/latest/recordings/test_sql_deleted_server_show_not_found.yaml |
Updates recording headers/timestamps for deleted-server show-not-found. |
src/azure-cli/azure/cli/command_modules/sql/tests/latest/recordings/test_sql_deleted_server_list_empty_location.yaml |
Updates recording payload and metadata for deleted-server list scenario. |
Suppressed comments (1)
src/azure-cli/azure/cli/command_modules/sql/custom.py:4458
- The new
location is not Nonebehavior is intended to change how--location ""behaves, but there is no test asserting that the CLI fails fast with a clear error for an empty location value.
if location is not None:
return client.list_by_location(location)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
4454
to
4459
| # Use `is not None` so that an explicitly provided empty string (e.g. --location "") | ||
| # keeps location-scoped behavior and fails fast at the API, rather than silently | ||
| # switching to the subscription-wide listing. | ||
| if location is not None: | ||
| return client.list_by_location(location) | ||
| return client.list() |
rambabu-yalla
changed the title
Member
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
rambabu-yalla
force-pushed
the
ramyal/FixSoftDeleteRetentionDaysUpdate
branch
from
August 19, 2026 19:35
ae8bafa to
70f2992
Compare
rambabu-yalla
changed the title
rambabu-yalla
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/azure-cli/azure/cli/command_modules/sql/tests/latest/test_sql_commands.py:9469
- The test now asserts
retentionDays == -1on create/show, but elsewhere in this same filetest_sql_server_soft_delete_retention_update_scenariosexplicitly treatsretentionDaysas-1or0when soft delete was not configured. This stricter expectation can make the test flaky across RP/API-version variations and is inconsistent with the existing test semantics; consider asserting membership in[-1, 0, None]instead of a fixed-1.
JMESPathCheck('retentionDays', -1)])
# Verify via show as well: retentionDays must be -1 (never configured)
self.cmd('sql server show -g {} --name {}'
.format(resource_group, server_name),
Member
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
Cooper Cox (coopercox-ms)
added
the
Azure Client Tools Agent Requested
Contributor
Live test results —
|
azure-client-tools-agent
Bot
added
the
azure-client-tools-agent
![azure-client-tools-agent[bot]](https://avatars.githubusercontent.com/in/4205534?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Validation Summary
Automated checks found failures on the current head commit (70f2992):
CI (GitHub Actions):
- ❌
Azure.azure-cli Full Test— build failed (run) - ❌
Azure.azure-cli— build failed (run) - ❌
Azure.azure-cli Full Test (Automation Full Test Python314 Profile Latest instance5)— failed (run) - ❌
Azure.azure-cli Full Test (Automation Full Test Python312 Profile Latest instance5)— failed (run) - ✅ 48/53 other checks passed
Live test (azdev test --live): ❌ failed (run)
please take a look at the failing builds/tests linked above and push a fix. Re-request review once addressed.
azure-client-tools-agent
Bot
removed
the
Azure Client Tools Agent Requested
Contributor
|
🔔 Routing this PR to @Azure/act-codegen-extensibility-squad. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🤖 PR Validation — ❌ Action needed
❌AzureCLI-FullTest
Related command
az sql server update,Description
No customer-facing behavior change
az sql server update: Removed custom handling of--soft-delete-retention-daysand switched to the standard update pattern. Previously the update path unconditionally reassignedinstance.retention_days, which caused the PUT body to carry an empty retention value on unrelated updates. The SQL RP discardsnull, so nothing was actually reset — this just stops the CLI from round-tripping stale state.Testing Guide
No commands are updated
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis 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.