Skip to content

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

Merged
merged 8 commits into from
Aug 24, 2017
Merged

Conversation

angshumannayak
Copy link
Contributor

@angshumannayak angshumannayak commented Aug 8, 2017

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

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
  • The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

  • Pull request includes test coverage for the included changes.
  • PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

  • New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
  • Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

  • Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
  • Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
  • Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

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.
@msftclas
Copy link

msftclas commented Aug 8, 2017

@angshumannayak,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@azuresdkci
Copy link

Can one of the admins verify this patch?

@cormacpayne
Copy link
Member

@azuresdkci add to whitelist

Copy link
Member

@cormacpayne cormacpayne left a 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
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

@angshumannayak same comment

Copy link
Contributor Author

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.

Copy link
Member

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.

@markcowl
Copy link
Member

@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
Copy link
Contributor Author

@angshumannayak angshumannayak left a 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

@cormacpayne cormacpayne merged commit e880c23 into preview Aug 24, 2017
@cormacpayne cormacpayne deleted the angshumannayak-patch-1 branch October 14, 2017 18:17
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.

6 participants