Add new command budget to consumption #6024

bgsky · 2018-04-05T21:52:27Z

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

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).
I adhere to the Command Guidelines.

promptws · 2018-04-05T21:52:55Z

View a preview at https://prompt.ws/r/Azure/azure-cli/6024
This is an experimental preview for @microsoft users.

derekbekoe · 2018-04-09T17:37:12Z

src/command_modules/azure-cli-consumption/HISTORY.rst

@@ -2,6 +2,10 @@

 Release History
 ===============
+0.3.1
+++++
+* Added commands `budget`.


Change to clearer description. Currently, this gives very little information.
Here are some examples from previous releases of the module.
https://docs.microsoft.com/en-us/cli/azure/release-notes-azure-cli?view=azure-cli-latest#consumption
https://docs.microsoft.com/en-us/cli/azure/release-notes-azure-cli?view=azure-cli-latest#consumption-1

derekbekoe · 2018-04-09T17:39:49Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_validators.py

+            return Decimal(string)
+        except ValueError:
+            raise ValueError("the value passed cannot be converted to decimal")
+    return decimal_type


You shouldn't need the wrapper function get_decimal_type.
Just define decimal_type as is then in your param definition, pass the function decimal_type instead of doing get_decimal_type ()

Same with get_datetime_type above.

derekbekoe · 2018-04-09T17:41:07Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_validators.py

+    """lowercase the time grain for comparison"""
+    budgetcategory = namespace.category.lower()
+    if budgetcategory != 'cost' and budgetcategory != 'usage':
+        raise CLIError("usage error: --parameters.category must be set to either cost or usage")


The param name is --category. User won't know what --parameters.category is.

Additionally, this should be covered by using get_enum_type in the param registration.
https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#registering-enums

derekbekoe · 2018-04-09T17:41:14Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_validators.py

+    time_grain = namespace.time_grain.lower().strip()
+
+    if time_grain != 'annually' and time_grain != 'quarterly' and time_grain != 'monthly':
+        raise CLIError("usage error: --parameters.time_grain can be 'Annually', 'Quarterly', or 'Monthly'.")


https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#registering-enums

derekbekoe · 2018-04-09T17:41:22Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_validators.py

+        raise CLIError("usage error: --parameters.time_grain can be 'Annually', 'Quarterly', or 'Monthly'.")
+
+    if namespace.amount < 0:
+        raise CLIError("usage error: --parameters.amount must be greater than 0")


derekbekoe · 2018-04-09T17:42:25Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/commands.py

+                         exception_handler=consumption_exception_handler, validator=validate_budget_parameters, client_factory=budget_mgmt_client_factory)
+
+        p.custom_command('delete', 'cli_consumption_delete_budget', transform=None,
+                         exception_handler=consumption_exception_handler, client_factory=budget_mgmt_client_factory)


Instead of duplicating exception_handler and client_factory everywhere, you should be able to define them in the command_group and it'll be used for all in the group.

derekbekoe · 2018-04-09T17:42:48Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/custom.py

+def cli_consumption_list_budgets(client, resource_group_name=None):
+    if resource_group_name:
+        return list(client.list_by_resource_group_name(resource_group_name))
+    return list(client.list())


No need to call list() around the SDK calls. The CLI handles this.

…tion

bgsky · 2018-04-10T08:10:32Z

@derekbekoe @tjprescott All comments above are addressed. Please proceed on your side.

tjprescott

Ditto all of @derekbekoe's comments. Added some of my own.

tjprescott · 2018-04-09T23:11:35Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_params.py

@@ -34,3 +36,15 @@ def load_arguments(self, _):
        cmp.argument('top', options_list=['--top', '-t'], type=int, help='Maximum number of items to return. Value range: 1-1000.')
        cmp.argument('start_date', options_list=['--start-date', '-s'], type=get_datetime_type(), help='Start date (YYYY-MM-DD in UTC). If specified, also requires --end-date.')
        cmp.argument('end_date', options_list=['--end-date', '-e'], type=get_datetime_type(), help='End date (YYYY-MM-DD in UTC). If specified, also requires --start-date.')
+
+    with self.argument_context('consumption budget') as cb:
+        cb.argument('resource_group_name', options_list='--resource-group-name', type=str, help='Resource group of the budget.')


This is overriding the global resource group aliasing, but shouldn't.

tjprescott · 2018-04-10T16:41:05Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_params.py

+
+    with self.argument_context('consumption budget') as cb:
+        cb.argument('resource_group_name', options_list='--resource-group-name', type=str, help='Resource group of the budget.')
+        cb.argument('budget_name', options_list='--budget-name', type=str, help='Name of a budget.')


Options list is not required here. Type is not required.

tjprescott · 2018-04-10T16:41:21Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_params.py

+    with self.argument_context('consumption budget') as cb:
+        cb.argument('resource_group_name', options_list='--resource-group-name', type=str, help='Resource group of the budget.')
+        cb.argument('budget_name', options_list='--budget-name', type=str, help='Name of a budget.')
+        cb.argument('category', options_list=['--category', '-c'], type=str, help='Category of the budget can be cost or usage.')


Should be enum type. Type kwarg not required.

tjprescott · 2018-04-10T16:41:48Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_params.py

+        cb.argument('budget_name', options_list='--budget-name', type=str, help='Name of a budget.')
+        cb.argument('category', options_list=['--category', '-c'], type=str, help='Category of the budget can be cost or usage.')
+        cb.argument('amount', options_list=['--amount', '-a'], type=get_decimal_type(), help='Amount of a budget.')
+        cb.argument('time_grain', options_list='--time-grain', type=str, help='Time grain of the budget can be monthly, quarterly, or annually.')


Options list not required. Type not required. Should be enum type...

tjprescott · 2018-04-10T16:44:55Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_params.py

+        cb.argument('time_grain', options_list='--time-grain', type=str, help='Time grain of the budget can be monthly, quarterly, or annually.')
+        cb.argument('start_date', options_list=['--start-date', '-s'], type=get_datetime_type(), help='Start date (YYYY-MM-DD in UTC) of time period of a budget.')
+        cb.argument('end_date', options_list=['--end-date', '-e'], type=get_datetime_type(), help='End date (YYYY-MM-DD in UTC) of time period of a budget.')
+        cb.argument('resource_groups', options_list='--resource-groups', nargs='+', help='Space-separated list of resource groups to filter on.')


Options list not required. These filter type arguments should have arg_group='Filter' so they stand out in help. Right now, you'll have

--resource-group -g The "resource group" --resource-groups Resources groups to filter on

You might also consider making the property name something like "--filter-groups" or something. Right now, --resource-group and --resource-groups would be easy to confuse.

tjprescott · 2018-04-10T16:45:05Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_params.py

+        cb.argument('start_date', options_list=['--start-date', '-s'], type=get_datetime_type(), help='Start date (YYYY-MM-DD in UTC) of time period of a budget.')
+        cb.argument('end_date', options_list=['--end-date', '-e'], type=get_datetime_type(), help='End date (YYYY-MM-DD in UTC) of time period of a budget.')
+        cb.argument('resource_groups', options_list='--resource-groups', nargs='+', help='Space-separated list of resource groups to filter on.')
+        cb.argument('resources', options_list='--resources', nargs='+', help='Space-separated list of resource instances to filter on.')


Options list not required.

tjprescott · 2018-04-10T16:45:35Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_params.py

+        cb.argument('end_date', options_list=['--end-date', '-e'], type=get_datetime_type(), help='End date (YYYY-MM-DD in UTC) of time period of a budget.')
+        cb.argument('resource_groups', options_list='--resource-groups', nargs='+', help='Space-separated list of resource groups to filter on.')
+        cb.argument('resources', options_list='--resources', nargs='+', help='Space-separated list of resource instances to filter on.')
+        cb.argument('meters', options_list='--meters', nargs='+', help='Space-separated list of meters to filter on. Required if category is usage.')


Options list not required. What are meters?

tjprescott · 2018-04-10T16:46:53Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/commands.py

+                         exception_handler=consumption_exception_handler, validator=validate_budget_parameters, client_factory=budget_mgmt_client_factory)
+
+        p.custom_command('delete', 'cli_consumption_delete_budget', transform=None,
+                         exception_handler=consumption_exception_handler, client_factory=budget_mgmt_client_factory)


The default values of validator and transform are None, so you can (and should) omit those.

bgsky · 2018-04-10T23:42:01Z

@derekbekoe @tjprescott Resolved comments from both of you. Please refer to the latest iteration and proceed.

derekbekoe · 2018-04-16T23:30:03Z

@tjprescott Can you take a look and review also?

tjprescott

Overall looks much better. Still a few small issues.

tjprescott · 2018-04-17T00:23:36Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_params.py



 def load_arguments(self, _):
    with self.argument_context('consumption usage') as c:
        c.argument('top', options_list=['--top', '-t'], type=int, help='Maximum number of items to return. Value range: 1-1000.')
        c.argument('include_additional_properties', options_list=['--include-additional-properties', '-a'], action='store_true', help='Include additional properties in the usages.')
        c.argument('include_meter_details', options_list=['--include-meter-details', '-m'], action='store_true', help='Include meter details in the usages.')
-        c.argument('start_date', options_list=['--start-date', '-s'], type=get_datetime_type(), help='Start date (YYYY-MM-DD in UTC). If specified, also requires --end-date.')
-        c.argument('end_date', options_list=['--end-date', '-e'], type=get_datetime_type(), help='End date (YYYY-MM-DD in UTC). If specified, also requires --start-date.')
+        c.argument('start_date', options_list=['--start-date', '-s'], type=datetime_type, help='Start date (YYYY-MM-DD in UTC). If specified, also requires --end-date.')


Consider using the get_datetime_type method from azure.cli.core.commands.parameters. They are methods because you have to parameterize, but the intention will be to standardize date handling using these configurable methods. Try it on one command and let me know whether it fits your scenario.

tjprescott · 2018-04-17T00:24:03Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_params.py

+
+    with self.argument_context('consumption budget') as cb:
+        cb.argument('budget_name', help='Name of a budget.')
+        cb.argument('category', arg_type=get_enum_type(['cost', 'usage']), help='Category of the budget can be cost or usage.')


Why are these enums not in your SDK?

tjprescott · 2018-04-17T00:24:42Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_params.py

+        cb.argument('time_grain', arg_type=get_enum_type(['monthly', 'quarterly', 'annually']), help='Time grain of the budget can be monthly, quarterly, or annually.')
+        cb.argument('start_date', options_list=['--start-date', '-s'], type=datetime_type, help='Start date (YYYY-MM-DD in UTC) of time period of a budget.')
+        cb.argument('end_date', options_list=['--end-date', '-e'], type=datetime_type, help='End date (YYYY-MM-DD in UTC) of time period of a budget.')
+        cb.argument('resource_groups', options_list='--resource-group-filter', nargs='+', help='Space-separated list of resource groups to filter on.')


This is fine, but you should still add arg_group='Filter' to these so they are grouped together in help.

tjprescott · 2018-04-17T00:25:38Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_validators.py

-        raise ValueError("Input '{}' not valid. Valid example: 2017-02-11T23:59:59Z".format(string))
-    return datetime_type
+    accepted_date_formats = ['%Y-%m-%dT%H:%M:%SZ', '%Y-%m-%dT%H:%MZ', '%Y-%m-%dT%HZ', '%Y-%m-%d']
+    for form in accepted_date_formats:


Try the version and core. If you feel it doesn't support you scenarios, let me know so I can improve it.

tjprescott · 2018-04-17T00:26:25Z

src/command_modules/azure-cli-consumption/azure/cli/command_modules/consumption/_validators.py

+        raise CLIError("usage error: --category must be set to either cost or usage")
+    time_grain = namespace.time_grain.lower().strip()
+    if time_grain != 'annually' and time_grain != 'quarterly' and time_grain != 'monthly':
+        raise CLIError("usage error: --time_grain can be 'Annually', 'Quarterly', or 'Monthly'.")


These enums should not be validated in code at all. That's why you register them as enums. The CLI handles all of that. Validating the amount is fine though.

Removed validation on category and time grain

bgsky · 2018-04-24T20:52:33Z

@tjprescott get_datetime_type method got errors when running test cases. Will prefer to keep original datetime_type.

Add new command budget to consumption

32dada6

derekbekoe added this to the Sprint 36 (Build) milestone Apr 5, 2018

REDMOND\chanwan added 2 commits April 5, 2018 16:29

Fix build errors on consumption

7ec8e0c

Fix test bugs on consumption

d473a14

derekbekoe suggested changes Apr 9, 2018

View reviewed changes

REDMOND\chanwan added 4 commits April 9, 2018 23:33

Addressed comments of argument type, wrapper, and comments on consump…

8f476d6

…tion

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

3237ae9

Fixed build errors on indent

ee09a4d

Fixed import sequence error

ee51703

tjprescott suggested changes Apr 10, 2018

View reviewed changes

REDMOND\chanwan added 3 commits April 10, 2018 12:31

Fixed bugs on argument option list

89c6bb8

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

f90fc2d

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

2d892eb

derekbekoe approved these changes Apr 16, 2018

View reviewed changes

tjprescott suggested changes Apr 17, 2018

View reviewed changes

REDMOND\chanwan added 3 commits April 23, 2018 16:16

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

85d4dc4

Removed enum type validation

5869ff5

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

3b282ec

tjprescott approved these changes Apr 24, 2018

View reviewed changes

tjprescott merged commit ecb1c53 into Azure:dev Apr 24, 2018

derekbekoe mentioned this pull request Apr 24, 2018

Misspelling in BASH shell, az consumption usage list #5645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new command budget to consumption #6024

Add new command budget to consumption #6024

bgsky commented Apr 5, 2018 •

edited

Loading

promptws commented Apr 5, 2018

derekbekoe Apr 9, 2018

derekbekoe Apr 9, 2018

derekbekoe Apr 9, 2018

derekbekoe Apr 9, 2018

tjprescott Apr 9, 2018

derekbekoe Apr 9, 2018

tjprescott Apr 9, 2018

derekbekoe Apr 9, 2018

derekbekoe Apr 9, 2018

derekbekoe Apr 9, 2018

bgsky commented Apr 10, 2018

tjprescott left a comment

tjprescott Apr 9, 2018

tjprescott Apr 10, 2018

tjprescott Apr 10, 2018

tjprescott Apr 10, 2018

tjprescott Apr 10, 2018

tjprescott Apr 10, 2018

tjprescott Apr 10, 2018

tjprescott Apr 10, 2018

bgsky commented Apr 10, 2018

derekbekoe commented Apr 16, 2018

tjprescott left a comment

tjprescott Apr 17, 2018

tjprescott Apr 17, 2018

tjprescott Apr 17, 2018

bgsky Apr 23, 2018

tjprescott Apr 17, 2018

tjprescott Apr 17, 2018

bgsky Apr 23, 2018

bgsky commented Apr 24, 2018

Add new command budget to consumption #6024

Add new command budget to consumption #6024

Conversation

bgsky commented Apr 5, 2018 • edited Loading

promptws commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgsky commented Apr 10, 2018

tjprescott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgsky commented Apr 10, 2018

derekbekoe commented Apr 16, 2018

tjprescott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgsky commented Apr 24, 2018

bgsky commented Apr 5, 2018 •

edited

Loading