Update Set-AzureRmSqlServerAuditing and Set-AzureRmSqlDatabaseAuditing Cmdlets #6457
Conversation
Code without tests.
Adding tests and test records
Update test records
Update help files.
Update .gitignore
|
| /// <summary> | ||
| /// Gets or sets storage account subscription id. | ||
| /// </summary> | ||
| [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = AuditingHelpMessages.AuditStorageAccountSubscriptionIdHelpMessage)] |
There was a problem hiding this comment.
As per the discussion in the design review
The behavior as the changes stand currently is that the user can specify StorageAccountSubscriptionId without specifying StorageAccountName.
Given that StorageAccountName is required when StorageAccountSubscriptionIdis specified, I would recomment adding a parameter set whereStorageAccountNameis required andStorageAccountSubscriptionId` is optional.
| /// <summary> | ||
| /// Gets or sets storage account subscription id. | ||
| /// </summary> | ||
| [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = AuditingHelpMessages.AuditStorageAccountSubscriptionIdHelpMessage)] |
There was a problem hiding this comment.
As per the discussion in the design review
The behavior as the changes stand currently is that the user can specify StorageAccountSubscriptionId without specifying StorageAccountName.
Given that StorageAccountName is required when StorageAccountSubscriptionIdis specified, I would recomment adding a parameter set whereStorageAccountNameis required andStorageAccountSubscriptionId` is optional.
…me parameter set Combined StorageAccountSubscriptionId and StorageAccountName under same parameter set
Update old auditing cmdlets
Fix errors regarding md files
Fix error in md file
Fix examples in md file
|
@praries880 Your requests were addressed. |
| StringBuilder uriBuilder = new StringBuilder(Context.Environment.GetEndpointAsUri(AzureEnvironment.Endpoint.ResourceManager).ToString()); | ||
| uriBuilder.AppendFormat("{0}/listKeys?api-version={1}", | ||
| storageAccountId, | ||
| isClassicStorage ? "2016-11-01" : "2017-06-01"); |
There was a problem hiding this comment.
Nitpick : These string literals should be constants instead.
There was a problem hiding this comment.
@praries880 Versions were extracted to constants, while Uri template was not due to readability.
| if (isClassicStorage) | ||
| { | ||
| primaryKey = (string)storageAccountKeysResponse["primaryKey"]; | ||
| secondaryKey = (string)storageAccountKeysResponse["secondaryKey"]; |
There was a problem hiding this comment.
Nitpick : These string literals should be constants instead.
Extract some hardcoded strings to constants
|
@praries880 Since all requests were addressed and relevant open issue was accepted, can "cmdlet review required" and "needs revision" Labels be removed from PR? |
praries880
added
Waiting for On Demand
and removed
Cmdlet Review Required ⚠️
needs-revision
labels
Description
Adding a new parameter (-StorageAccountSubscriptionId) existing Set-AzureRmSqlServerAuditing and Set-AzureRmSqlDatabaseAuditing Cmdlets.
Checklist
CONTRIBUTING.mdplatyPSmodule