Skip to content

New version of cmdlets for AzureRM.Billing and AzureRM.Consumption #3852

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

New version of cmdlets for AzureRM.Billing and AzureRM.Consumption #3852

wants to merge 8 commits into from

Conversation

Panda-Wang
Copy link

@Panda-Wang Panda-Wang commented Apr 27, 2017 •
edited by azuresdkci
Loading

Description

New version of cmdlets for AzureRM.Billing and AzureRM.Consumption

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.

cormacpayne
cormacpayne suggested changes Apr 27, 2017
View reviewed changes
src/ResourceManager/Billing/AzureRM.Billing.psd1 Outdated
@@ -12,7 +12,7 @@
# RootModule = ''

# Version number of this module.
ModuleVersion = '0.11.0'
ModuleVersion = '0.12.0'
Copy link
Member

Choose a reason for hiding this comment

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

@Panda-Wang please revert this change; we will update the module versions during the release

Copy link
Author

Choose a reason for hiding this comment

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

reverted

src/ResourceManager/Billing/ChangeLog.md Outdated
@@ -19,6 +19,13 @@
-->
## Current Release

## Version 0.12.0
Copy link
Member

Choose a reason for hiding this comment

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

@Panda-Wang please remove the ## Version 0.12.0 header you have added and move the content you have added to under the ## Current Release header

Copy link
Author

Choose a reason for hiding this comment

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

removed

src/ResourceManager/Billing/Commands.Billing.Test/Properties/AssemblyInfo.cs Outdated
[assembly: AssemblyVersion("0.11.0")]
[assembly: AssemblyFileVersion("0.11.0")]
[assembly: AssemblyVersion("0.12.0")]
[assembly: AssemblyFileVersion("0.12.0")]
Copy link
Member

Choose a reason for hiding this comment

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

@Panda-Wang please revert the changes made to this file

Copy link
Author

Choose a reason for hiding this comment

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

reverted

src/ResourceManager/Billing/Commands.Billing/Cmdlets/BillingPeriods/GetAzureRmBillingPeriod.cs Outdated
{
if (ParameterSetName.Equals(Constants.ParameterSetNames.ListParameterSet))
{
if (MaxCount.HasValue && (MaxCount.Value > 100 || MaxCount.Value < 1))
Copy link
Member

Choose a reason for hiding this comment

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

@Panda-Wang you can use the ValidateRange attribute to limit the values that users pass through to the MaxCount parameter. Here is an example of a parameter using the ValidateRange attribute

Copy link
Author

Choose a reason for hiding this comment

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

fixed

src/ResourceManager/Billing/Commands.Billing/Cmdlets/Invoices/GetAzureRmBillingInvoice.cs Outdated
}
if (MaxCount.HasValue && (MaxCount.Value > 100 || MaxCount.Value < 1))
{
throw new PSArgumentException(Resources.MaxCountExceedRangeError);
Copy link
Member

Choose a reason for hiding this comment

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

@Panda-Wang same comment about ValidateRange

Copy link
Author

Choose a reason for hiding this comment

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

fixed

src/ResourceManager/Billing/Commands.Billing/Properties/AssemblyInfo.cs Outdated
[assembly: AssemblyVersion("0.11.0")]
[assembly: AssemblyFileVersion("0.11.0")]
[assembly: AssemblyVersion("0.12.0")]
[assembly: AssemblyFileVersion("0.12.0")]
Copy link
Member

Choose a reason for hiding this comment

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

@Panda-Wang please revert the changes made in this file

Copy link
Author

Choose a reason for hiding this comment

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

reverted

src/ResourceManager/Consumption/ChangeLog.md Outdated
-->
## Current Release

## Version 0.1.0
Copy link
Member

Choose a reason for hiding this comment

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

@Panda-Wang you can remove the ## Version 0.1.0 header and place the content you have added under the ## Current Release header

Copy link
Author

Choose a reason for hiding this comment

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

removed

...er/Consumption/Commands.Consumption/Cmdlets/UsageDetails/GetAzureRmConsumptionUsageDetail.cs
[Cmdlet(VerbsCommon.Get, "AzureRmConsumptionUsageDetail", DefaultParameterSetName = Constants.ParameterSetNames.SubscriptionItemParameterSet), OutputType(typeof(List<PSUsageDetail>))]
public class GetAzureRmConsumptionUsageDetail : AzureConsumptionCmdletBase
{
const int MaxNumberToFetch = 1000;
Copy link
Member

Choose a reason for hiding this comment

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

@Panda-Wang I believe you can get rid of this if you use the ValidateRange attribute for MaxCount

Copy link
Author

Choose a reason for hiding this comment

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

this is still needed. This is the number of items to fetch in each request. as suggested by Mark, we keep pulling the next page until we have everything. but API has output size limit, thus we pull max 1000 result in each request to API call.

...er/Consumption/Commands.Consumption/Cmdlets/UsageDetails/GetAzureRmConsumptionUsageDetail.cs Outdated
[Parameter(Mandatory = false, HelpMessage = "Determine the maximum number of records to return.", ParameterSetName = Constants.ParameterSetNames.InvoiceItemParameterSet)]
[Parameter(Mandatory = false, HelpMessage = "Determine the maximum number of records to return.", ParameterSetName = Constants.ParameterSetNames.BillingPeriodItemParameterSet)]
[ValidateNotNull]
public int? MaxCount { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

@Panda-Wang it looks like for MaxCount, IncludeMeterDetails, IncludeAdditionalProperties, StartDate and EndDate you don't need to have a Parameter attribute for each parameter set. Since these parameters appear in all of the parameter sets and have the same attributes across them all, you can make them global, meaning you only need one Parameter attribute per parameter, and you don't need to include a parameter set name for them.

Copy link
Author

Choose a reason for hiding this comment

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

updated

@markcowl
Copy link
Member

@Panda-Wang PR is failing because of the names of parameters IncludeMeterDetails and IncludeAdditonalProperties please add suppressions for these in the exceptions folder here:https://github.com/Azure/azure-powershell/blob/preview/tools/StaticAnalysis/Exceptions/SignatureIssues.csv (you can just copy them from the static analysis reports here: http://azuresdkci.cloudapp.net/job/powershell/8291/artifact/src/Package/SignatureIssues.csv)

@markcowl markcowl assigned Panda-Wang and unassigned cormacpayne Apr 28, 2017
@markcowl
Copy link
Member

@azuresdkci retest this please

@markcowl
Copy link
Member

markcowl commented May 1, 2017

@Panda-Wang You will need to pull the latest and rebuild your wxi file. BTW - are you adding new dependency files? If not, you don't need to update the wxi.

@markcowl
Copy link
Member

markcowl commented May 1, 2017

@Panda-Wang It looks like you have added a bunch of changed files outside your own changes. Can you please fetch the latest locally before updating your PR

Panda-Wang pushed a commit to Panda-Wang/azure-powershell that referenced this pull request May 1, 2017
@pandawang1123
Resubmit for New version of cmdlets for AzureRM.Billing and AzureRM.C…
dbbac5b 
…onsumption Azure#3852
@Panda-Wang Panda-Wang mentioned this pull request May 1, 2017
Merged
13 tasks
@Panda-Wang
Copy link
Author

Re-submitted from a clean fork: #3880

@cormacpayne cormacpayne closed this May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cormacpayne cormacpayne cormacpayne requested changes

Assignees

@Panda-Wang Panda-Wang

Labels
needs-revision
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Panda-Wang @markcowl @cormacpayne @msftclas @pandawang1123