-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
SQL Ledger Cmdlets #14974
SQL Ledger Cmdlets #14974
Conversation
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.
Looks good overall.
let's add WhatIf and tests
/// </summary> | ||
[Parameter(Mandatory = false, | ||
HelpMessage = "The enable ledger option for the Azure Sql Database")] | ||
public SwitchParameter EnableLedger { get; set; } |
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.
Since we don't allow alterting this, should this be part of the Set cmdlet?
src/Sql/Sql/LedgerDigestUploads/Cmdlet/AzureSqlDatabaseLedgerDigestUploadBase.cs
Outdated
Show resolved
Hide resolved
src/Sql/Sql/LedgerDigestUploads/Cmdlet/AzureSqlDatabaseLedgerDigestUploadBase.cs
Outdated
Show resolved
Hide resolved
src/Sql/Sql/LedgerDigestUploads/Cmdlet/EnableAzSqlDatabaseLedgerDigestUpload.cs
Show resolved
Hide resolved
src/Sql/Sql/LedgerDigestUploads/Model/AzureSqlDatabaseLedgerDigestLocationModel.cs
Show resolved
Hide resolved
15a1724
to
e79386f
Compare
.SYNOPSIS | ||
Tests getting default ledger digest upload configuration | ||
#> | ||
function Test-GetDefaultLedgerDigestUpload |
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.
For the Get case, we don't have various parameter options:name, resourceID, etc.? Only for Set?
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.
Oh it seems like we do but you test everything in one test?
$ledgerDigestUpload = Get-AzSqlDatabaseLedgerDigestUpload -ResourceGroupName $params.rgname -ServerName $params.serverName -DatabaseName $params.databaseName | ||
|
||
# Assert | ||
Assert-AreEqual $ledgerDigestUpload.State "Disabled" |
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.
So we only test Get for Disabled? Are the other scenarios tested as part of the Set tests?
closed, use #14997 |
Description
In progress PR for SQL Ledger cmdlets, see issue: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/929
See SDK change here: Azure/azure-sdk-for-net#21021
Swagger change here: https://github.com/Azure/azure-rest-api-specs/pull/13916/files, https://github.com/Azure/azure-rest-api-specs/pull/13871/files
Adds ledger cmdlets, and pipes IsLedgerOn property for database creation. Also updating naming in other resources to match changes in SDK. Currently testing with local SDK version, will update once new version is published.
Checklist
CONTRIBUTING.md
ChangeLog.md
file(s) has been updated:ChangeLog.md
file can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
## Upcoming Release
header -- no new version header should be added