Added Deleted server command and modified new and set logical server#29017
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
This pull request adds a new Get-AzSqlDeletedServer cmdlet and modifies the soft-delete retention functionality in Azure SQL Server cmdlets. The changes improve the soft-delete feature's usability by simplifying the parameter model and providing better tooling for managing deleted servers.
Key Changes:
- Added new
Get-AzSqlDeletedServercmdlet to retrieve information about soft-deleted Azure SQL servers by location - Simplified soft-delete retention configuration by making
SoftDeleteRetentionDaysthe primary parameter (deprecatedEnableSoftDelete) - Reduced maximum retention period from 35 days to 7 days (breaking change)
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Sql/Sql/help/Get-AzSqlDeletedServer.md | New help documentation for Get-AzSqlDeletedServer cmdlet |
| src/Sql/Sql/help/Set-AzSqlServer.md | Updated examples to use SoftDeleteRetentionDays parameter instead of EnableSoftDelete |
| src/Sql/Sql/help/Restore-AzSqlServer.md | Clarified parameter descriptions for resource group and location requirements |
| src/Sql/Sql/help/New-AzSqlServer.md | Updated examples and parameter help to reflect new retention range (0-7 days) |
| src/Sql/Sql/Server/Services/AzureSqlDeletedServerCommunicator.cs | New service communicator for deleted server REST endpoints |
| src/Sql/Sql/Server/Services/AzureSqlDeletedServerAdapter.cs | New adapter layer for deleted server operations with model conversion |
| src/Sql/Sql/Server/Model/AzureSqlDeletedServerModel.cs | New model class representing deleted server properties |
| src/Sql/Sql/Server/Cmdlet/SetAzureSqlServer.cs | Refactored to use centralized soft-delete validation and computation methods |
| src/Sql/Sql/Server/Cmdlet/RestoreAzureSqlServer.cs | Enhanced validation logic for deleted server restoration with improved error messages |
| src/Sql/Sql/Server/Cmdlet/NewAzureSqlServer.cs | Refactored to use centralized soft-delete validation and added deprecation warning |
| src/Sql/Sql/Server/Cmdlet/GetAzSqlDeletedServer.cs | New cmdlet implementation for retrieving deleted servers |
| src/Sql/Sql/Server/Cmdlet/AzureSqlServerCmdletBase.cs | Added shared validation and computation methods for soft-delete parameters |
| src/Sql/Sql/Server/Cmdlet/AzureSqlDeletedServerCmdletBase.cs | New base class for deleted server cmdlets |
| src/Sql/Sql/Properties/Resources.resx | Added new error message strings for deleted server operations |
| src/Sql/Sql/Properties/Resources.Designer.cs | Generated code for new resource strings |
| src/Sql/Sql/ChangeLog.md | Added release notes documenting new cmdlet and breaking changes |
| src/Sql/Sql/Az.Sql.psd1 | Added Get-AzSqlDeletedServer to exported cmdlets list |
| src/Sql/Sql.Test/UnitTests/AzureSqlDeletedServerAttributeTests.cs | New unit tests for cmdlet attributes |
| src/Sql/Sql.Test/SessionRecords/Microsoft.Azure.Commands.Sql.Test.ScenarioTests.DeletedServerTests/TestGetDeletedServerByLocation.json | Test recording for deleted server retrieval |
| src/Sql/Sql.Test/ScenarioTests/ServerCrudTests.ps1 | Updated test cases to use SoftDeleteRetentionDays parameter |
| src/Sql/Sql.Test/ScenarioTests/DeletedServerTests.ps1 | New scenario tests for Get-AzSqlDeletedServer cmdlet |
| src/Sql/Sql.Test/ScenarioTests/DeletedServerTests.cs | New test runner class for deleted server tests |
Files not reviewed (1)
- src/Sql/Sql/Properties/Resources.Designer.cs: Language not supported
| string errorMessage = errEx.Body?.Error?.Message ?? errEx.Message; | ||
|
|
||
| if (errorCode == "ResourceGroupNotFound" || | ||
| (errorMessage.Contains("Resource group") && errorMessage.Contains("could not be found"))) |
There was a problem hiding this comment.
The error handling checks both errorCode and errorMessage to determine ResourceGroupNotFound condition. However, the string matching in errorMessage ("Resource group" and "could not be found") is fragile and could break if Microsoft changes the error message text. Consider relying primarily on the errorCode when available, or use a more robust error detection mechanism.
| string errorMessage = errEx.Body?.Error?.Message ?? errEx.Message; | |
| if (errorCode == "ResourceGroupNotFound" || | |
| (errorMessage.Contains("Resource group") && errorMessage.Contains("could not be found"))) | |
| if (string.Equals(errorCode, "ResourceGroupNotFound", StringComparison.OrdinalIgnoreCase)) |
| /// Gets or sets the name of the deleted server to retrieve. If not specified, lists all deleted servers in the location. | ||
| /// </summary> | ||
| [Parameter(Mandatory = false, | ||
| ValueFromPipelineByPropertyName = true, | ||
| Position = 0, | ||
| HelpMessage = "The name of the deleted server to retrieve. If not specified, lists all deleted servers in the location.")] | ||
| [ValidateNotNullOrEmpty] | ||
| public string ServerName { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the location where the deleted server was located | ||
| /// </summary> | ||
| [Parameter(Mandatory = true, | ||
| ValueFromPipelineByPropertyName = true, | ||
| Position = 1, | ||
| HelpMessage = "The Azure region where the deleted server was located.")] | ||
| [LocationCompleter("Microsoft.Sql/deletedServers")] | ||
| [ValidateNotNullOrEmpty] | ||
| public string Location { get; set; } |
There was a problem hiding this comment.
The Position attribute for Location parameter is set to 0, but for ServerName it's set to 1. However, in the parameter definition, ServerName comes before Location. The positions should match the natural order or the intended usage pattern. Consider making Location Position 1 and ServerName Position 0, or adjust them to match the expected command usage pattern.
| /// Gets or sets the name of the deleted server to retrieve. If not specified, lists all deleted servers in the location. | |
| /// </summary> | |
| [Parameter(Mandatory = false, | |
| ValueFromPipelineByPropertyName = true, | |
| Position = 0, | |
| HelpMessage = "The name of the deleted server to retrieve. If not specified, lists all deleted servers in the location.")] | |
| [ValidateNotNullOrEmpty] | |
| public string ServerName { get; set; } | |
| /// <summary> | |
| /// Gets or sets the location where the deleted server was located | |
| /// </summary> | |
| [Parameter(Mandatory = true, | |
| ValueFromPipelineByPropertyName = true, | |
| Position = 1, | |
| HelpMessage = "The Azure region where the deleted server was located.")] | |
| [LocationCompleter("Microsoft.Sql/deletedServers")] | |
| [ValidateNotNullOrEmpty] | |
| public string Location { get; set; } | |
| /// Gets or sets the location where the deleted server was located | |
| /// </summary> | |
| [Parameter(Mandatory = true, | |
| ValueFromPipelineByPropertyName = true, | |
| Position = 0, | |
| HelpMessage = "The Azure region where the deleted server was located.")] | |
| [LocationCompleter("Microsoft.Sql/deletedServers")] | |
| [ValidateNotNullOrEmpty] | |
| public string Location { get; set; } | |
| /// <summary> | |
| /// Gets or sets the name of the deleted server to retrieve. If not specified, lists all deleted servers in the location. | |
| /// </summary> | |
| [Parameter(Mandatory = false, | |
| ValueFromPipelineByPropertyName = true, | |
| Position = 1, | |
| HelpMessage = "The name of the deleted server to retrieve. If not specified, lists all deleted servers in the location.")] | |
| [ValidateNotNullOrEmpty] | |
| public string ServerName { get; set; } |
| string[] segments = deletedServer.OriginalId.Split('/'); | ||
|
|
||
| // Parse servername and subscription from originalId if available | ||
| string parsedServerName = segments[8]; | ||
| string parsedSubscriptionId = segments[2]; | ||
| string parsedResourceGroupName = segments[4]; | ||
|
|
There was a problem hiding this comment.
Hardcoded array indices (2, 4, 8) are used to parse the resource ID without validation. If the OriginalId format changes or is malformed, this will throw an IndexOutOfRangeException. Consider using a more robust parsing method or at least validating the array length before accessing indices.
| string[] segments = deletedServer.OriginalId.Split('/'); | |
| // Parse servername and subscription from originalId if available | |
| string parsedServerName = segments[8]; | |
| string parsedSubscriptionId = segments[2]; | |
| string parsedResourceGroupName = segments[4]; | |
| string parsedServerName = null; | |
| string parsedSubscriptionId = null; | |
| string parsedResourceGroupName = null; | |
| if (!string.IsNullOrEmpty(deletedServer.OriginalId)) | |
| { | |
| string[] segments = deletedServer.OriginalId.Split('/'); | |
| // Parse server name, subscription, and resource group from OriginalId if available | |
| if (segments.Length > 8) | |
| { | |
| parsedSubscriptionId = segments[2]; | |
| parsedResourceGroupName = segments[4]; | |
| parsedServerName = segments[8]; | |
| } | |
| } |
|
|
||
| ### -SoftDeleteRetentionDays | ||
| Specifies the soft-delete retention days for the server. The acceptable values for this parameter are 0-35. Specify 0 to disable the SoftDelete | ||
| Specifies the soft-delete retention days for the server. The acceptable values for this parameter are 0-7. Specify 0 to disable the SoftDelete |
There was a problem hiding this comment.
The acceptable values range has been changed from 0-35 to 0-7, which is a breaking change for existing users who may have configured retention between 8-35 days. This breaking change should be clearly documented in the ChangeLog with proper migration guidance.
| Specifies the soft-delete retention days for the server. The acceptable values for this parameter are 0-7. Specify 0 to disable the SoftDelete | |
| Specifies the number of days that deleted databases are retained for recovery on this server. The acceptable values for this parameter are 0 through 7. Specify 0 to disable soft-delete retention. Earlier versions of this cmdlet allowed values from 0 through 35, so existing scripts or configurations that use values between 8 and 35 must be updated to use a value between 0 and 7 to succeed. |
| - Additional information about change #1 | ||
| --> | ||
| ## Upcoming Release | ||
| * Added 'Get-AzSqlDeletedServer' cmdlet to retrieve soft deleted Azure SQL servers |
There was a problem hiding this comment.
According to ChangeLog guidelines, avoid using special characters like backticks in ChangeLog entries as they can cause issues in PowerShell module manifests. Replace 'Get-AzSqlDeletedServer' with Get-AzSqlDeletedServer (without quotes) or use simple quotes.
| * Added 'Get-AzSqlDeletedServer' cmdlet to retrieve soft deleted Azure SQL servers | ||
| - Supports retrieving deleted servers by location or specific deleted server by name | ||
| * Preannounced breaking changes. Please refer to https://go.microsoft.com/fwlink/?linkid=2333229 | ||
| * The EnableSoftDelete parameter is deprecated from `New-AzSqlServer` and `Set-AzSqlServer` cmdlets and will be removed by May 2026. |
There was a problem hiding this comment.
The ChangeLog entry mentions "The EnableSoftDelete parameter is deprecated" but does not specify which version it was deprecated from. According to ChangeLog guidelines for Azure PowerShell, entries should clearly communicate the change from the user's perspective. Consider rewording to: "Deprecated EnableSoftDelete parameter in New-AzSqlServer and Set-AzSqlServer cmdlets. Use SoftDeleteRetentionDays parameter instead. EnableSoftDelete will be removed in May 2026."
| ## Upcoming Release | ||
| * Added 'Get-AzSqlDeletedServer' cmdlet to retrieve soft deleted Azure SQL servers | ||
| - Supports retrieving deleted servers by location or specific deleted server by name | ||
| * Preannounced breaking changes. Please refer to https://go.microsoft.com/fwlink/?linkid=2333229 |
There was a problem hiding this comment.
The ChangeLog entry about breaking changes lacks detail for users. Consider expanding it to explain what the breaking changes are (e.g., SoftDeleteRetentionDays range reduced from 0-35 to 0-7, EnableSoftDelete parameter deprecated). Users should understand the impact without needing to click the link.
| [ValidateRange(0, 35)] | ||
| public int? SoftDeleteRetentionDays { get; set; } | ||
| HelpMessage = "Specifies the number of days to retain a deleted server for possible restoration. Valid values are 0-7. A value of 0 disables soft-delete retention.")] | ||
| public int? SoftDeleteRetentionDays { get; set; } |
There was a problem hiding this comment.
The ValidateRange attribute has been removed from the SoftDeleteRetentionDays parameter. While validation logic exists in ValidateSoftDeleteParameters method, removing the ValidateRange attribute means users won't get immediate parameter validation feedback before the cmdlet executes. Consider keeping the ValidateRange attribute for better user experience.
| public int? SoftDeleteRetentionDays { get; set; } | |
| [ValidateRange(0, 7)] | |
| public int? SoftDeleteRetentionDays { get; set; } |
| 'Get-AzSqlDeletedServer', | ||
| 'Get-AzSqlElasticJob', |
There was a problem hiding this comment.
There is inconsistent indentation with tabs instead of spaces on lines 138-139. The rest of the file uses spaces. Maintain consistent indentation throughout the file.
| 'Get-AzSqlDeletedServer', | |
| 'Get-AzSqlElasticJob', | |
| 'Get-AzSqlDeletedServer', | |
| 'Get-AzSqlElasticJob', |
| * Added 'Get-AzSqlDeletedServer' cmdlet to retrieve soft deleted Azure SQL servers | ||
| - Supports retrieving deleted servers by location or specific deleted server by name | ||
| * Preannounced breaking changes. Please refer to https://go.microsoft.com/fwlink/?linkid=2333229 | ||
| * The EnableSoftDelete parameter is deprecated from `New-AzSqlServer` and `Set-AzSqlServer` cmdlets and will be removed by May 2026. |
There was a problem hiding this comment.
According to ChangeLog guidelines, avoid using backticks around cmdlet names in ChangeLog entries. Replace New-AzSqlServer and Set-AzSqlServer with New-AzSqlServer and Set-AzSqlServer (without backticks) or use simple quotes.
rambabu-yalla
force-pushed
the
AddDeletedSqlServerCommand
branch
from
e2d36c5 to
0f45c7d
Compare
| protected void ValidateSoftDeleteParameters(int? softDeleteRetentionDays, bool? enableSoftDelete) | ||
| { | ||
| // Validate SoftDeleteRetentionDays range | ||
| if (softDeleteRetentionDays.HasValue) | ||
| { | ||
| if (softDeleteRetentionDays.Value < SoftDeleteRetentionDaysDisabled || softDeleteRetentionDays.Value > SoftDeleteRetentionDaysMaximum) | ||
| { | ||
| throw new PSArgumentException($"SoftDeleteRetentionDays must be between {SoftDeleteRetentionDaysDisabled} and {SoftDeleteRetentionDaysMaximum}.", "SoftDeleteRetentionDays"); | ||
| } | ||
| } | ||
|
|
||
| // If both EnableSoftDelete and SoftDeleteRetentionDays are specified, validate their combination | ||
| if (enableSoftDelete.HasValue && softDeleteRetentionDays.HasValue) | ||
| { | ||
| if (enableSoftDelete == true && (softDeleteRetentionDays.Value < SoftDeleteRetentionDaysMinimum || softDeleteRetentionDays.Value > SoftDeleteRetentionDaysMaximum)) | ||
| { | ||
| throw new PSArgumentException($"When EnableSoftDelete is true, SoftDeleteRetentionDays must be between {SoftDeleteRetentionDaysMinimum} and {SoftDeleteRetentionDaysMaximum}.", "SoftDeleteRetentionDays"); | ||
| } | ||
| else if (enableSoftDelete == false && softDeleteRetentionDays.Value != SoftDeleteRetentionDaysDisabled) | ||
| { | ||
| throw new PSArgumentException($"When EnableSoftDelete is false, SoftDeleteRetentionDays must be {SoftDeleteRetentionDaysDisabled}.", "SoftDeleteRetentionDays"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Computes the SoftDeleteRetentionDays value based on provided parameters | ||
| /// </summary> | ||
| /// <param name="softDeleteRetentionDays">The explicitly provided retention days</param> | ||
| /// <param name="enableSoftDelete">The enable soft delete flag</param> | ||
| /// <param name="existingValue">The existing retention days value (for Set operations)</param> | ||
| /// <returns>The computed retention days value</returns> | ||
| protected int? ComputeSoftDeleteRetentionDays(int? softDeleteRetentionDays, bool? enableSoftDelete, int? existingValue = null) | ||
| { | ||
| if (softDeleteRetentionDays.HasValue) | ||
| { | ||
| // SoftDeleteRetentionDays was explicitly provided, use it | ||
| return softDeleteRetentionDays.Value; | ||
| } | ||
| else if (enableSoftDelete.HasValue) | ||
| { | ||
| // Only EnableSoftDelete was provided | ||
| if (enableSoftDelete == true) | ||
| { | ||
| // Enabling soft-delete without specifying days, default to 7 | ||
| return SoftDeleteRetentionDaysMaximum; | ||
| } | ||
| else | ||
| { | ||
| // Disabling soft-delete, set to 0 | ||
| return SoftDeleteRetentionDaysDisabled; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Neither parameter specified, return existing value or null | ||
| return existingValue; | ||
| } | ||
| } |
There was a problem hiding this comment.
There is no test coverage for the new validation logic that enforces SoftDeleteRetentionDays must be between 0 and 7. Consider adding negative test cases that verify:
- Values less than 0 are rejected with the appropriate error message
- Values greater than 7 are rejected with the appropriate error message
- The validation properly handles the interaction between EnableSoftDelete and SoftDeleteRetentionDays parameters
| AzureSqlDeletedServerModel model = new AzureSqlDeletedServerModel() | ||
| { | ||
| ServerName = parsedServerName, | ||
| DeletionTime = deletedServer.DeletionTime, | ||
| FullyQualifiedDomainName = deletedServer.FullyQualifiedDomainName, | ||
| Version = deletedServer.Version, | ||
| Id = deletedServer.Id, | ||
| Type = deletedServer.Type, | ||
| OriginalId = deletedServer.OriginalId, | ||
| SubscriptionId = parsedSubscriptionId, | ||
| ResourceGroupName = parsedResourceGroupName | ||
| }; |
There was a problem hiding this comment.
The Location property is missing from the AzureSqlDeletedServerModel but is shown in the help documentation examples (lines 33, 53). The cmdlet receives Location as a parameter but doesn't populate it in the returned model. Either add a Location property to the model and populate it in CreateDeletedServerModelFromResponse, or update the help documentation to remove Location from the example outputs.
| private AzureSqlDeletedServerModel GetDeletedServerInResourceGroup() | ||
| { | ||
| DeletedServer deletedServer; | ||
| try | ||
| { | ||
| deletedServer = DeletedServerAdapter.GetDeletedServer(this.Location, this.ServerName); | ||
| } | ||
| catch (ErrorResponseException errEx) when (errEx.Response?.StatusCode == System.Net.HttpStatusCode.NotFound) | ||
| { | ||
| throw new PSArgumentException( | ||
| string.Format(Properties.Resources.DeletedServerNotFound, this.ServerName, this.Location), | ||
| "ServerName"); | ||
| } | ||
|
|
||
| AzureSqlDeletedServerModel deletedServerModel = DeletedServerAdapter.CreateDeletedServerModelFromResponse(deletedServer); | ||
|
|
||
| if (deletedServerModel == null) | ||
| { | ||
| throw new PSArgumentException( | ||
| string.Format(Properties.Resources.DeletedServerNotFound, this.ServerName, this.Location), | ||
| "ServerName"); | ||
| } | ||
|
|
||
| // Validate that the user-provided ResourceGroupName matches the deleted server's ResourceGroupName | ||
| if (!string.Equals(this.ResourceGroupName, deletedServerModel.ResourceGroupName, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| throw new PSArgumentException( | ||
| string.Format(Properties.Resources.ResourceGroupMismatchForRestore, | ||
| this.ResourceGroupName, deletedServerModel.ResourceGroupName, this.ServerName), | ||
| "ResourceGroupName"); | ||
| } | ||
|
|
||
| return deletedServerModel; | ||
| } |
There was a problem hiding this comment.
There is no test coverage for the new error handling scenarios in RestoreAzureSqlServer:
- ResourceGroupMismatchForRestore error when user-provided resource group doesn't match the deleted server's original resource group
- ResourceGroupNotFoundForRestore error when the resource group doesn't exist
These are important error paths that should be tested to ensure users get clear, actionable error messages.
| /// </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 = "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.
The help message still references the EnableSoftDelete parameter in its description: "When enabled, a dropped server can be restored within the retention window (defaults to 7 days if not specified)." However, the parameter is deprecated and users are encouraged to use SoftDeleteRetentionDays instead. The help message should be updated to clarify that this parameter is deprecated and direct users to use SoftDeleteRetentionDays parameter instead.
| 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 = "Deprecated. Use the SoftDeleteRetentionDays parameter instead to configure soft-delete retention. Setting SoftDeleteRetentionDays to 1-7 enables soft-delete, and setting it to 0 disables soft-delete.")] |
| @@ -164,14 +165,14 @@ public class NewAzureSqlServer : AzureSqlServerCmdletBase | |||
| [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.")] | |||
There was a problem hiding this comment.
The help message still references the EnableSoftDelete parameter in its description: "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." However, the parameter is deprecated and users are encouraged to use SoftDeleteRetentionDays instead. The help message should be updated to clarify that this parameter is deprecated and direct users to use SoftDeleteRetentionDays parameter instead.
| 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 = "DEPRECATED. This parameter will be removed in a future release. Use SoftDeleteRetentionDays instead. Setting SoftDeleteRetentionDays to 1-7 enables soft-delete retention (a dropped server can be restored within the retention window), and setting it to 0 disables soft-delete retention.")] |
| <data name="ResourceGroupMismatchForRestore" xml:space="preserve"> | ||
| <value>The specified resource group '{0}' does not match the deleted server's original resource group '{1}'. Server '{2}' must be restored to its original resource group.</value> | ||
| </data> | ||
| <data name="ResourceGroupNotFoundForRestore" xml:space="preserve"> | ||
| <value>Cannot restore deleted server '{0}' because resource group '{1}' does not exist. Please create the resource group before restoring the server.</value> | ||
| </data> |
There was a problem hiding this comment.
The error message ResourceGroupMismatchForRestore ends with a period, while ResourceGroupNotFoundForRestore also ends with a period. However, many other error messages in the file don't end with periods (e.g., DeletedServerNotFound, DeletedServerNotFoundInLocation). For consistency with the broader codebase, consider whether error messages should consistently end with periods or not.
| Assert-StartsWith ($server5.ServerName + ".") $server5.FullyQualifiedDomainName | ||
| Assert-AreEqual $server5.SoftDeleteRetentionDays 0 | ||
|
|
||
| # Scenario 6: Create server without either parameter (should default to 0 - disabled ot -1 until backend fix is deployed) |
There was a problem hiding this comment.
There's a typo in the comment. "ot" should be "or". The comment reads "should default to 0 - disabled ot -1" but should be "should default to 0 - disabled or -1".
| # Scenario 6: Create server without either parameter (should default to 0 - disabled ot -1 until backend fix is deployed) | |
| # Scenario 6: Create server without either parameter (should default to 0 - disabled or -1 until backend fix is deployed) |
|
/azp run azure-powershell - security-tools |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| This command creates a version 12 Azure SQL Database server with TDE CMK enabled. | ||
|
|
||
| ### Example 4: Create a new Azure SQL Database server with soft delete retention enabled with default retention days |
There was a problem hiding this comment.
File: New-AzSqlServer.md
-
SoftDeleteRetentionDaysdescription inaccuracy:
Help file says: "The acceptable values for this parameter are 0-7"
Code implementation: The code uses ValidateSoftDeleteParameters() method which validates against
SoftDeleteRetentionDaysMinimum (1) and SoftDeleteRetentionDaysMaximum (7), with 0 being disabled. -
EnableSoftDeletemissing deprecation notice in description:
Code has: [GenericBreakingChangeWithVersion("The EnableSoftDelete parameter will be removed. Please use SoftDeleteRetentionDays parameter instead...", "16.0.0", "7.0.0")]
Help file description: Simply states "Specifies whether or not soft-delete retention is enabled for the server."
⚠️ Should mention that this parameter is deprecated and will be removed.
| /// <returns>A collection of deleted servers</returns> | ||
| protected override IEnumerable<AzureSqlDeletedServerModel> GetEntity() | ||
| { | ||
| ICollection<AzureSqlDeletedServerModel> results = new List<AzureSqlDeletedServerModel>(); |
| else | ||
| { | ||
| // List all deleted servers in the location | ||
| var deletedServers = ModelAdapter.ListDeletedServers(this.Location); |
There was a problem hiding this comment.
ListDeletedServers returns IEnumerable.
Can this be rewritten using Linq?
results = ModelAdapter.ListDeletedServers(this.Location)
.Select(deletedServer => ModelAdapter.CreateDeletedServerModelFromResponse(deletedServer))
There was a problem hiding this comment.
IEnumerable<AzureSqlDeletedServerModel> results = null;
...
// for one result
results = new List<AzureSqlDeletedServerModel> { ModelAdapter.CreateDeletedServerModelFromResponse(deletedServer)}
|
Server has a validation for this but the client should also contain a test?
|
|
Missing tests?
|
pranavathalye
left a comment
pranavathalye
left a comment
There was a problem hiding this comment.
Please address all outstanding comments before merging.
Head branch was pushed to by a user without write access
|
|
||
| ### -EnableSoftDelete | ||
| Specifies whether or not soft-delete retention is enabled for the server. | ||
| **This parameter has been deprecated and will be removed in May 2026 (Az version 16.0.0 / Az.Sql version 7.0.0). Please use the SoftDeleteRetentionDays parameter instead.** |
There was a problem hiding this comment.
The deprecation notice is missing the module name (Az.Sql) along with the version information. For consistency with Azure PowerShell deprecation practices, it should mention both the Az module version and the Az.Sql module version.
The notice should be: "This parameter has been deprecated and will be removed in May 2026 (Az version 16.0.0 / Az.Sql version 7.0.0)."
| **This parameter has been deprecated and will be removed in May 2026 (Az version 16.0.0 / Az.Sql version 7.0.0). Please use the SoftDeleteRetentionDays parameter instead.** | |
| This parameter has been deprecated and will be removed in May 2026 (Az version 16.0.0 / Az.Sql version 7.0.0). |
| <#[SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="Test passwords only valid for the duration of the test")]#> | ||
| $serverPassword = "t357ingP@s5w0rd!" |
There was a problem hiding this comment.
The comment on line 122 includes a SuppressMessage attribute for the password on line 123, but other test functions in this file (lines 22, 66) also have hardcoded test passwords without the SuppressMessage attribute. For consistency and to avoid security scanning false positives, all test passwords should have the SuppressMessage attribute.
| <#[SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="Test passwords only valid for the duration of the test")]#> | |
| $serverPassword = "t357ingP@s5w0rd!" | |
| $serverPassword = [guid]::NewGuid().ToString() |
|
|
||
| ### -EnableSoftDelete | ||
| Boolean value for whether or not to enable soft-delete for the server such that the server can be restored for a default of 7 days after dropping. If you want to specify a different retention period, use the SoftDeleteRetentionDays parameter. | ||
| **This parameter has been deprecated and will be removed in May 2026 (Az version 16.0.0 / Az.Sql version 7.0.0). Please use the SoftDeleteRetentionDays parameter instead.** |
There was a problem hiding this comment.
The deprecation notice is missing the module name (Az.Sql) along with the version information. For consistency with Azure PowerShell deprecation practices, it should mention both the Az module version and the Az.Sql module version.
The notice should be: "This parameter has been deprecated and will be removed in May 2026 (Az version 16.0.0 / Az.Sql version 7.0.0)."
| string[] segments = deletedServer.OriginalId.Split('/'); | ||
|
|
||
| // Parse servername and subscription from originalId if available |
There was a problem hiding this comment.
The method is parsing the resource ID without validating that the split array has enough elements. If the OriginalId format is unexpected or malformed, this will throw an IndexOutOfRangeException. Add validation to ensure segments.Length is at least 9 before accessing segments[8], or wrap in a try-catch block to provide a more meaningful error message.
| string[] segments = deletedServer.OriginalId.Split('/'); | |
| // Parse servername and subscription from originalId if available | |
| if (string.IsNullOrEmpty(deletedServer.OriginalId)) | |
| { | |
| throw new InvalidOperationException("Deleted server OriginalId is null or empty. Unable to parse subscription, resource group, and server name."); | |
| } | |
| string[] segments = deletedServer.OriginalId.Split('/'); | |
| // Expect an ARM resource ID with at least 9 segments so that indices 2, 4, and 8 are valid. | |
| if (segments.Length <= 8) | |
| { | |
| throw new InvalidOperationException( | |
| string.Format( | |
| "Deleted server OriginalId '{0}' is not in the expected format. Unable to parse subscription, resource group, and server name.", | |
| deletedServer.OriginalId)); | |
| } | |
| // Parse server name, subscription, and resource group from OriginalId |
| [ValidateRange(0, 35)] | ||
| public int? SoftDeleteRetentionDays { get; set; } | ||
| HelpMessage = "Specifies the number of days to retain a deleted server for possible restoration. Valid values are 0-7. A value of 0 disables soft-delete retention.")] | ||
| public int? SoftDeleteRetentionDays { get; set; } |
There was a problem hiding this comment.
Inconsistent indentation. The line "public int? SoftDeleteRetentionDays { get; set; }" should be aligned with the "[Parameter(...)]" attribute above it. All other parameter declarations in this file follow the standard indentation pattern.
| public int? SoftDeleteRetentionDays { get; set; } | |
| public int? SoftDeleteRetentionDays { get; set; } |
| { | ||
| // Get a specific deleted server | ||
| var deletedServer = ModelAdapter.GetDeletedServer(this.Location, this.ServerName); | ||
| results = new List<AzureSqlDeletedServerModel> { ModelAdapter.CreateDeletedServerModelFromResponse(deletedServer)}; |
There was a problem hiding this comment.
Missing space after closing brace in list initialization. The code "new List { ModelAdapter.CreateDeletedServerModelFromResponse(deletedServer)};" should have a space before the closing brace for consistency with C# formatting conventions: "new List { ModelAdapter.CreateDeletedServerModelFromResponse(deletedServer) };"
| results = new List<AzureSqlDeletedServerModel> { ModelAdapter.CreateDeletedServerModelFromResponse(deletedServer)}; | |
| results = new List<AzureSqlDeletedServerModel> { ModelAdapter.CreateDeletedServerModelFromResponse(deletedServer) }; |
|
/azp run azure-powershell - security-tools |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Head branch was pushed to by a user without write access
| [Parameter(Mandatory = true, | ||
| ValueFromPipelineByPropertyName = true, | ||
| Position = 0, | ||
| HelpMessage = "The Azure region where the deleted server was located.")] | ||
| [LocationCompleter("Microsoft.Sql/deletedServers")] | ||
| [ValidateNotNullOrEmpty] | ||
| public string Location { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the name of the deleted server to retrieve. If not specified, lists all deleted servers in the location. | ||
| /// </summary> | ||
| [Parameter(Mandatory = false, | ||
| ValueFromPipelineByPropertyName = true, | ||
| Position = 1, | ||
| HelpMessage = "The name of the deleted server to retrieve. If not specified, lists all deleted servers in the location.")] | ||
| [ValidateNotNullOrEmpty] | ||
| public string ServerName { get; set; } |
There was a problem hiding this comment.
The Position attribute is being specified in the cmdlet parameter definition, but it's documented in the help file. Consider whether positional parameters are appropriate for this cmdlet, as Azure PowerShell cmdlets typically prefer named parameters for clarity. If positions are needed, ensure they are consistent with similar cmdlets in the module.
| Position: 0 | ||
| Default value: None | ||
| Accept pipeline input: True (ByPropertyName) | ||
| Accept wildcard characters: False | ||
| ``` | ||
|
|
||
| ### -ServerName | ||
| The name of the deleted server to retrieve. If not specified, lists all deleted servers in the location. | ||
|
|
||
| ```yaml | ||
| Type: System.String | ||
| Parameter Sets: (All) | ||
| Aliases: | ||
|
|
||
| Required: False | ||
| Position: 1 |
There was a problem hiding this comment.
The Position attribute is set to 0 and 1 for the Location and ServerName parameters respectively. However, PowerShell cmdlets in Azure PowerShell typically don't use positional parameters for better clarity and to avoid ambiguity. Consider removing the Position attributes to enforce named parameters only.
|
/azp run azure-powershell - security-tools |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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.