-
Notifications
You must be signed in to change notification settings - Fork 4k
AddAzureRMLogProfile - Angshuman patch 2 #4429
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
Conversation
1) Added proper Synopsis with detailed verbatim about the storage account. 2) The current cmd help has an empty example so added a working example. 3) Updated the -Location parameter verbatim 4) Updated the -RetentionInDays verbatim 5) Updated the -StorageAccountId with an example and updated verbatim.
Fixed with comments from @vcanaa a) Removed the limit of one profile and one storage account from verbatim. This allows the verbatim to be applicable even when the limits are increased. b) Retaining "Global","West US" format to align with the format returned by Get-AzureLocation | Select DisplayName c) Modified -Location "Global,West US" to -Location "Global","West US" d) Fixed typo for ever to forever
Added and modified comments as per feedback.
@angshumannayak, |
Can one of the admins verify this patch? |
@azuresdkci add to whitelist |
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.
@angshumannayak a couple comments, otherwise LGTM
Specifies the list of locations. | ||
Specifies the location of the log profile. | ||
Valid values: Run below cmdlet to get the latest list of locations. | ||
```yaml |
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.
@angshumannayak please remove this extra ```yaml
block. It will affect how platyPS
generates the MAML from this markdown. You can keep the command you have in the block, but the ```yaml
text string (and closing ```
) will need to be removed.
@@ -106,7 +117,11 @@ Accept wildcard characters: False | |||
``` | |||
|
|||
### -StorageAccountId | |||
Specifies the ID of the Storage account. | |||
Specifies the ID of the Storage account. ID is the fully qualified Resource ID of the storage account for example | |||
```yaml |
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.
@angshumannayak same comment
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.
@cormacpayne if I have removed the yaml from around the block, but if I remove the ``` then command doesn't get into a block. Also I haven't added these blocks, these were present in the original version. I am just adding content to them as it's mostly empty.
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.
@angshumannayak if the yaml block you added is not removed, then platyPS
will throw an error when it tries to convert this markdown file in MAML, which affects the resulting help the user sees when Get-Help
is ran. The content you have added can remain, it just cannot exist within a yaml block.
@azuresdkci add to whitelist |
Removed ```yaml from 1) List of locations 2) StorageAccointId
Removed ```yaml from 1) List of locations 2) StorageAccointId
Removed ```yaml from 1) List of locations 2) StorageAccountId
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.
Removed the ```yaml as suggested by cormacpayne
Description
This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.
General Guidelines
Testing Guidelines
Cmdlet Signature Guidelines
ShouldProcess
and haveSupportShouldProcess=true
specified in the cmdlet attribute. You can find more information onShouldProcess
here.OutputType
attribute if any output is produced - if the cmdlet produces no output, it should implement aPassThru
parameter.Cmdlet Parameter Guidelines