Skip to content

Conversation

@brandong-ms
Copy link
Member

@brandong-ms brandong-ms commented Oct 23, 2025

Description

Mandatory Checklist

  • SHOULD update ChangeLog.md file(s) appropriately
    • Update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
      • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
    • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
  • SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
  • SHOULD have proper test coverage for changes in pull request.
  • SHOULD NOT adjust version of module manually in pull request

brandong-ms and others added 30 commits September 11, 2020 11:51
Merge main repo into fork
…k the new cmdlets to add PassThru parameter which returns LTR backup model object.
@azure-client-tools-bot-prd
Copy link

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

@isra-fel
Copy link
Member

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@isra-fel
Copy link
Member

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@brandong-ms brandong-ms marked this pull request as ready for review October 23, 2025 22:17
@Copilot Copilot AI review requested due to automatic review settings October 23, 2025 22:17
Copy link
Contributor

Copilot AI left a 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 adds support for immutable Long-Term Retention (LTR) backups in Azure SQL, introducing time-based and legal hold immutability features. The implementation includes new cmdlets for managing immutability policies, updated existing cmdlets to expose immutability parameters, and comprehensive documentation.

Key Changes:

  • Added four new cmdlets for managing LTR backup immutability (Lock, Remove, Set Legal Hold, Remove Legal Hold)
  • Extended existing models to include immutability properties (TimeBasedImmutability, TimeBasedImmutabilityMode, LegalHoldImmutability)
  • Updated Set-AzSqlDatabaseBackupLongTermRetentionPolicy to support immutability parameters

Reviewed Changes

Copilot reviewed 25 out of 64 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/Sql/Sql/help/*.md Added/updated help documentation for immutability cmdlets and properties
src/Sql/Sql/Properties/Resources.resx Added user-facing warning and description strings for immutability operations
src/Sql/Sql/Database Backup/Services/*.cs Implemented communicator and adapter methods for immutability API calls
src/Sql/Sql/Database Backup/Model/*.cs Extended models with immutability properties
src/Sql/Sql/Database Backup/Cmdlet/*.cs Implemented new cmdlets and updated existing policy cmdlet
src/Sql/Sql/Common/DatabaseBackupConstants.cs Added constants for immutability values
src/Sql/Sql/ChangeLog.md Documented changes for upcoming release
src/Sql/Sql/Az.Sql.psd1 Exported new cmdlets
src/Sql/Sql.Test/SessionRecords/*.json Added test recordings
src/Sql/Sql.Test/ScenarioTests/DatabaseBackupTests.cs Added test methods for immutability features
src/Sql/Sql.Management.Sdk/README.md Updated API spec references to 2024-11-01-preview
Files not reviewed (1)
  • src/Sql/Sql/Properties/Resources.Designer.cs: Language not supported

- Updated `AzureSqlDatabaseBackupLongTermRetentionPolicyModel` to add `TimeBasedImmutability` and `TimeBasedImmutabilityMode` properties.
- Updated `AzureSqlDatabaseLongTermRetentionBackupModel` to add `TimeBasedImmutability`, `TimeBasedImmutabilityMode`, and `LegalHoldImmutability` properties.
- Added new cmdlets `Lock-AzSqlDatabaseLongTermRetentionBackupImmutability` and `Remove-AzSqlDatabaseLongTermRetentionBackupImmutability` for managing time-based immutability on LTR backups.
- Added new cmdlets `Set-Az SqlDatabaseLongTermRetentionBackupLegalHold` and `Remove-AzSqlDatabaseLongTermRetentionBackupLegalHold` for managing legal hold immutability on LTR backups.
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra space in the cmdlet name. It should be "Set-AzSqlDatabaseLongTermRetentionBackupLegalHold" not "Set-Az SqlDatabaseLongTermRetentionBackupLegalHold".

Suggested change
- Added new cmdlets `Set-Az SqlDatabaseLongTermRetentionBackupLegalHold` and `Remove-AzSqlDatabaseLongTermRetentionBackupLegalHold` for managing legal hold immutability on LTR backups.
- Added new cmdlets `Set-AzSqlDatabaseLongTermRetentionBackupLegalHold` and `Remove-AzSqlDatabaseLongTermRetentionBackupLegalHold` for managing legal hold immutability on LTR backups.

Copilot uses AI. Check for mistakes.
WeekOfYear : 26
Location :
TimeBasedImmutability : Enabled
TimeBasedImmutabilityMode : Unlocked
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example output shows TimeBasedImmutabilityMode as "Unlocked" when the command explicitly sets -TimeBasedImmutabilityMode Locked. This is inconsistent with the command being executed.

Suggested change
TimeBasedImmutabilityMode : Unlocked
TimeBasedImmutabilityMode : Locked

Copilot uses AI. Check for mistakes.
### -DatabaseName
The name of the Azure SQL Database the backup is from.
The name of the Azure SQL Server the backups are under.
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description for -DatabaseName parameter is incorrect. It states "The name of the Azure SQL Server the backups are under" but should describe the database name, not the server name. The ServerName parameter already describes the server.

Suggested change
The name of the Azure SQL Server the backups are under.
The name of the Azure SQL database the backups are under.

Copilot uses AI. Check for mistakes.
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. When you refresh the help documents make sure to add -RefreshModulePage to update the index doc Az.Sql.md as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the new entries below "#Upcoming Release“

@github-actions
Copy link

This PR was labeled "needs-revision" because it has unresolved review comments or CI failures.
Please resolve all open review comments and make sure all CI checks are green. Refer to our guide to troubleshoot common CI failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants