[Sql] Updated soft-delete test and added preview tag to cmdlets#29176
[Sql] Updated soft-delete test and added preview tag to cmdlets#29176rambabu-yalla wants to merge 3 commits intoAzure:mainfrom
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Pull request overview
Updates Azure SQL Server soft-delete preview signaling and tightens a scenario test expectation around the API-returned SoftDeleteRetentionDays value.
Changes:
- Mark
Get-AzSqlDeletedServerandRestore-AzSqlServeras preview viaCmdletPreview. - Add
"[Preview]"prefixes to soft-delete-related parameter help text inNew-AzSqlServer/Set-AzSqlServer. - Update
ServerCrudTests.ps1to assertSoftDeleteRetentionDaysis-1(instead of0or-1) when no soft-delete parameters are provided.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/Sql/Sql/Server/Cmdlet/SetAzureSqlServer.cs |
Adds "[Preview]" labeling to soft-delete parameter help text. |
src/Sql/Sql/Server/Cmdlet/RestoreAzureSqlServer.cs |
Adds CmdletPreview attribute for soft-delete preview signaling. |
src/Sql/Sql/Server/Cmdlet/NewAzureSqlServer.cs |
Adds "[Preview]" labeling to soft-delete parameter help text. |
src/Sql/Sql/Server/Cmdlet/GetAzSqlDeletedServer.cs |
Adds CmdletPreview attribute for soft-delete preview signaling. |
src/Sql/Sql.Test/ScenarioTests/ServerCrudTests.ps1 |
Changes default/unset retention-days assertion to require -1. |
| /// <summary> | ||
| /// Defines the Restore-AzSqlServer cmdlet | ||
| /// </summary> | ||
| [CmdletPreview("Server soft-delete feature is currently in Public Preview")] |
There was a problem hiding this comment.
Same as Get-AzSqlDeletedServer: marking Restore-AzSqlServer with CmdletPreview is a user-facing change. Consider documenting it under src/Sql/Sql/ChangeLog.md → ## Upcoming Release so consumers can see the preview status change in release notes.
| /// </summary> | ||
| [Parameter(Mandatory = false, | ||
| HelpMessage = "Specify whether to enable soft-delete retention for the server. When enabled, a dropped server can be restored within the retention window (defaults to 7 days if not specified). To set a custom retention period use -SoftDeleteRetentionDays.")] | ||
| HelpMessage = "[Preview] Specify whether to enable soft-delete retention for the server. When enabled, a dropped server can be restored within the retention window (defaults to 7 days if not specified). To set a custom retention period use -SoftDeleteRetentionDays.")] |
There was a problem hiding this comment.
The HelpMessage is being tagged with a literal "[Preview]" prefix. In this repo preview signaling is typically done via the [CmdletPreview(...)] attribute (e.g., src/Sql/Sql/Database Backup/Cmdlet/SetAzureRmSqlDatabaseLongTermRetentionBackupLegalHold.cs:25, src/Network/Network/LoadBalancer/Probe/AddAzureRmLoadBalancerProbeConfigCommand.cs:27), which also produces a consistent preview warning experience. Consider adding CmdletPreview to New-AzSqlServer with messaging that only the soft-delete parameters are preview, and keep the parameter HelpMessage text free of the manual prefix (or otherwise align with the established approach).
| /// </summary> | ||
| [Parameter(Mandatory = false, | ||
| HelpMessage = "Specify whether to enable soft-delete retention for the server. When enabled, a dropped server can be restored within the retention window (defaults to 7 days if not specified).")] | ||
| HelpMessage = "[Preview] Specify whether to enable soft-delete retention for the server. When enabled, a dropped server can be restored within the retention window (defaults to 7 days if not specified).")] |
There was a problem hiding this comment.
Same as New-AzSqlServer: tagging parameters as preview by prefixing the HelpMessage with "[Preview]" is inconsistent with the repo’s established preview mechanism ([CmdletPreview(...)]). If soft-delete is preview, consider putting the preview warning on the cmdlet via CmdletPreview (with text noting which parameters/behavior are preview) rather than encoding preview state into the HelpMessage string.
| Assert-AreEqual $server6.SqlAdministratorLogin $serverLogin | ||
| Assert-StartsWith ($server6.ServerName + ".") $server6.FullyQualifiedDomainName | ||
| Assert-True {$server6.SoftDeleteRetentionDays -eq 0 -or $server6.SoftDeleteRetentionDays -eq -1} | ||
| Assert-True {$server6.SoftDeleteRetentionDays -eq -1} |
There was a problem hiding this comment.
This test now asserts the service returns SoftDeleteRetentionDays == -1 when neither -EnableSoftDelete nor -SoftDeleteRetentionDays are specified. That’s inconsistent with other scenarios in this same test that treat the disabled/default state as 0, and with cmdlet validation/constants that define the supported range as 0–7. If the backend ever returns 0 for the unset case (or varies by region/api-version), this will become flaky; consider either accepting both 0 and -1, or normalizing the API’s -1 to 0 in the adapter/model (and then assert the normalized value).
| Assert-True {$server6.SoftDeleteRetentionDays -eq -1} | |
| Assert-True { ($server6.SoftDeleteRetentionDays -eq -1) -or ($server6.SoftDeleteRetentionDays -eq 0) } |
|
Hi @rambabu-yalla please join the Microsoft org on GitHub following this link https://aka.ms/AzureGitHub if you are a Microsoft Employee. It ensures all CI pipeline trigger automatically for you. |
isra-fel
left a comment
There was a problem hiding this comment.
Please see my inline comments. Thank you
| - Additional information about change #1 | ||
| --> | ||
| ## Upcoming Release | ||
| * Added public preview tag for `EnableSoftDelete` and `SoftDeleteRetentionDays` properties in `New-AzSqlServer` and `Set-AzSqlServer` cmdlets |
There was a problem hiding this comment.
The term "properties" should be "parameters" in this context. EnableSoftDelete and SoftDeleteRetentionDays are cmdlet parameters (inputs that users provide to cmdlets), not properties. Looking at other ChangeLog entries (e.g., line 29, 33, 40), "parameters" is consistently used when referring to cmdlet inputs.
| * Added public preview tag for `EnableSoftDelete` and `SoftDeleteRetentionDays` properties in `New-AzSqlServer` and `Set-AzSqlServer` cmdlets | |
| * Added public preview tag for `EnableSoftDelete` and `SoftDeleteRetentionDays` parameters in `New-AzSqlServer` and `Set-AzSqlServer` cmdlets |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@isra-fel : Requested changes are in place . Could you please review and merge the PR. |
Description
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.