-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Introduced changes to cmdlets to support self-server restore of sql logical server #28589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduced changes to cmdlets to support self-server restore of sql logical server #28589
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for self-service restore functionality for SQL logical servers by adding soft-delete retention capabilities and a new restore cmdlet. The changes allow users to configure retention periods for deleted servers and restore them within that timeframe.
Key changes:
- Added soft-delete retention parameters to New-AzSqlServer and Set-AzSqlServer cmdlets
- Introduced new Restore-AzSqlServer cmdlet for restoring deleted servers
- Updated the underlying data model and service layer to support retention functionality
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Sql/Sql/help/Set-AzSqlServer.md | Added documentation for new retention parameters |
| src/Sql/Sql/help/Restore-AzSqlServer.md | Added help documentation for new restore cmdlet |
| src/Sql/Sql/help/New-AzSqlServer.md | Added documentation for soft-delete retention parameters |
| src/Sql/Sql/Server/Services/AzureSqlServerCommunicator.cs | Added method to retrieve deleted servers |
| src/Sql/Sql/Server/Services/AzureSqlServerAdapter.cs | Added support for retention days and create mode in server operations |
| src/Sql/Sql/Server/Model/AzureSqlServerModel.cs | Extended model with retention and create mode properties |
| src/Sql/Sql/Server/Cmdlet/SetAzureSqlServer.cs | Added retention parameters and logic |
| src/Sql/Sql/Server/Cmdlet/RestoreAzureSqlServer.cs | New cmdlet implementation for server restoration |
| src/Sql/Sql/Server/Cmdlet/NewAzureSqlServer.cs | Added soft-delete retention parameters |
| src/Sql/Sql/ChangeLog.md | Updated changelog with feature description |
| src/Sql/Sql/Az.Sql.psd1 | Added new Restore-AzSqlServer cmdlet to exports |
| src/Sql/Sql.Management.Sdk/README.md | Updated SDK configuration with new API specifications |
| Sid = this.ExternalAdminSID | ||
| } | ||
| }, | ||
| RetentionDays = (this.EnableSoftDeleteRetention && !this.SoftDeleteRetentionDays.HasValue) ? 7 : this.SoftDeleteRetentionDays |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still covered for the scenario of SoftDeleteRetentionDays being null and EnableSoftDeleteRetention not being passed (i.e. using default value of false)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- src/Sql/Sql/Properties/Resources.Designer.cs: Language not supported
7c84cba to
8863670
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- src/Sql/Sql/Properties/Resources.Designer.cs: Language not supported
8863670 to
afdcbf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- src/Sql/Sql/Properties/Resources.Designer.cs: Language not supported
afdcbf7 to
bdc66f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- src/Sql/Sql/Properties/Resources.Designer.cs: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please check in upgraded SDK, under src/Sql/Sql/Management.Sdk
- Please add proper test cases
| } | ||
|
|
||
| // SoftDeleteRetentionDays depends on EnableSoftDeleteRetention; if days are provided but soft-delete is not enabled, fail early. | ||
| if (this.SoftDeleteRetentionDays.HasValue && this.SoftDeleteRetentionDays > 0 && !this.EnableSoftDeleteRetention) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should SoftDeleteRetentionDays <= 0 be accepted if EnableSoftDeletionRetention is not supplied or set as false? I ask this purely from a command line user experience behaviour. Should the dependent parameter in absence of its parent always be rejected?
| /// <returns>The created server</returns> | ||
| protected override IEnumerable<Model.AzureSqlServerModel> PersistChanges(IEnumerable<Model.AzureSqlServerModel> entity) | ||
| { | ||
| return new List<Model.AzureSqlServerModel>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Assert that entity.Count == 1?
| { | ||
| // If not enabling, only explicitly set retention to 0 when the caller provided 0. | ||
| // Otherwise, leave as null so the service preserves the existing retention setting. | ||
| if (this.SoftDeleteRetentionDays.HasValue && this.SoftDeleteRetentionDays.Value == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a special case to allow user to reset SoftDeleteRetentionDays to 0, which effectively disables soft delete?
- Is changing of retention days via this cmdlet not allowed?
- If setting retention to 0 days is used as means to disallow soft deletion -
2a. How does user enable it back?
2b. Why not use a parameterDisableSoftDeleteorDisableSoftDeleteRetention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed first review pass.
Checked in Generated files under src/Sql/Sql/Management.Sdk and also added test cases with recordings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 16 out of 33 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- src/Sql/Sql/Properties/Resources.Designer.cs: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 16 out of 40 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- src/Sql/Sql/Properties/Resources.Designer.cs: Language not supported
| if (ModelAdapter.GetDeletedServer(this.Location, this.ServerName) == null) | ||
| { | ||
| throw new PSArgumentException( | ||
| string.Format(Properties.Resources.DeletedServerNotFound, | ||
| this.ServerName, this.ResourceGroupName), | ||
| "ServerName"); | ||
| } |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message format uses this.ResourceGroupName but the GetDeletedServer method only uses this.Location and this.ServerName. This could be misleading since the deleted server lookup is by location and server name, not resource group.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
|
||
| ### Example 1 | ||
| ```powershell | ||
| PS C:\> Restore-AzSqlServer -ResourceGroupName "resourcegroup01" -ServerName "server01" -Location "CentralUS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please delete "PS C:> "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| ### | ||
| ``` yaml | ||
| commit: 37d29762e68000fc58e03dcefada999f3876f0af | ||
| commit: 51eb4a8df996ebb94c835bf93e08a7001e5d5459 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pipeline validation tool detected the SDK does not match the commit in this README.md, please either regenerate or make sure what generated in your local was uploaded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| .SYNOPSIS | ||
| Tests disabling soft delete | ||
| #> | ||
| function Test-ServerDisableSoftDelete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no record for this test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same functionality is tested in existing test cases. removed the test
affb1e7 to
b363bf6
Compare
b363bf6 to
7c09029
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
…ver restore
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.