-
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 parameterDisableSoftDelete
orDisableSoftDeleteRetention
?
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"); | ||
} |
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.
Copilot uses AI. Check for mistakes.
/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.md
and reviewed the following information:ChangeLog.md
file(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
.## Upcoming Release
header in the past tense.ChangeLog.md
if no new release is required, such as fixing test case only.