Skip to content

Changing CLI for azure-consumption reflecting the new api-version#4900

Merged
derekbekoe merged 3 commits intoAzure:devfrom
sandeepnl:dev
Nov 16, 2017
Merged

Changing CLI for azure-consumption reflecting the new api-version#4900
derekbekoe merged 3 commits intoAzure:devfrom
sandeepnl:dev

Conversation

@sandeepnl
Copy link
Copy Markdown
Contributor

@sandeepnl sandeepnl commented Nov 16, 2017


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

Command Guidelines

  • Each command and parameter has a meaningful description.
  • Each new command has a test.

(see Authoring Command Modules)

@azuresdkci
Copy link
Copy Markdown
Contributor

View a preview at https://prompt.ws/r/Azure/azure-cli/4900
This is an experimental preview for @microsoft.com users.
(It may take a minute or two for your instance to be ready)
Email feedback to 'azfeedback' with subject 'Prompt Feedback'.

@sandeepnl
Copy link
Copy Markdown
Contributor Author

@derekbekoe & @lmazuel Please review

@lmazuel lmazuel requested a review from derekbekoe November 16, 2017 21:30
Copy link
Copy Markdown
Member

@derekbekoe derekbekoe left a comment

Choose a reason for hiding this comment

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

Looks good overall.
Left a few comments.

===============

0.2.0
++++++++++
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Heading length is too long. It should match the no. of characters in the heading.
CI may fail due to this.


0.2.0
++++++++++
Release of new GA api version 2017-11-30.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should start with * so it's an actual bullet point list.

register_cli_argument('consumption usage list', 'start_date', options_list=('--start-date', '-s'), type=get_datetime_type(), help='start date (in UTC Y-m-d) of the usages')
register_cli_argument('consumption usage list', 'end_date', options_list=('--end-date', '-e'), type=get_datetime_type(), help='end date (in UTC Y-m-d) of the usages')
register_cli_argument('consumption usage list', 'invoice_name', options_list=('--invoice-name', '-i'), help='name of a specific invoice to get the usage details that associate with')
register_cli_argument('consumption usage list', 'start_date', options_list=('--start-date', '-s'), type=get_datetime_type(), help='start date (in UTC Y-m-d) of the usages. Both start date and end date need to be supplied or neither ')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Remove the extra space at the end of the help text.


return list(client.list(scope, expand=expand, filter=filter_expression, top=top))
if (start_date and not end_date) or (end_date and not start_date):
raise ValueError("Usage : [--start-date <startDate> & --end-date <endDate>].")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be done at the validator stage like this:
"usage error: Both --start-date and --end-date need to be supplied or neither."
https://github.com/Azure/azure-cli/blob/dev/src/command_modules/azure-cli-extension/azure/cli/command_modules/extension/_params.py#L23

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, but that example didnt help me on how I can put a validator spanning two arguments. I will sync with you offline for this

@derekbekoe derekbekoe merged commit dc96afc into Azure:dev Nov 16, 2017
@sandeepnl
Copy link
Copy Markdown
Contributor Author

Thanks @derekbekoe for merging. How can we get this tagged to be released for the Dec 4th release of CLI?

@derekbekoe
Copy link
Copy Markdown
Member

Everything in dev branch is automatically included in each release so you should be good!

tjprescott added a commit that referenced this pull request Nov 27, 2017
* sp reset-credentials from dev

* rebase polish rbac error message

* rebase role assignment list: show default assignments for classic administrators

* Modified AzureUSGov Endpoint to .US (#4877)

* Modified AzureUSGov Endpoint to .US

* Updated History.RST to include USGovAADEndPoint Change

* Updated Setup.py to match HISTORY.RST

* Reduced previous change in HISTORY.RST to a single line

* HISTORY.rst merge with latest commit

* HISTORY.RST Syntax Correction

* webapp/functionapp: ensure list/show display correct set of apps (#4891)

* Fix authoring doc typo (#4903)

* Changing CLI for azure-consumption reflecting the new api-version (#4900)

* Changing CLI for azure-consumption reflecting the new api-version

* Modifying Setup.py and history.rst

* Implementing review comments

* Rebase remove component module

* [acr] typo fix in help (#4904)

* [acr] typo fix in help

* [acr] bump version and add history line item

* Rebase of support raw format on resource show

* Web: add node appsetting when runtime is not specified (#4907)

* webapp: fix a bug in the cert name generation (#4909)

* Fix container module default ports (#4950)

* fix default ports

* bump up version

* update history

* Advisor command module (#4898)

* Initial change to add Advisor module.

* Add list recommendations command.

* More commands.

* More commands.

* Pylint fixes and adding a test.

* More updates.

* More updates.

* Fixing precheck errors.

* Fixing more precheck errors.

* Initial change to add Advisor module.

* Add list recommendations command.

* More commands.

* More commands.

* Pylint fixes and adding a test.

* More updates.

* More updates.

* Fixing precheck errors.

* Fixing more precheck errors.

* Adding missing manifest.

* Fixes for test failures.

* Addressing review comments and add unit tests.

* More fixes.

* Address CI failures.

* Extra space.

* One more fix.

* rebase sql usage commands.

* Add advisor to default install (#4956)

* Add consumption module to code owners (#4906)

* Added sql server conn-policy show/update commands (#4888)

* Update proj file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants