Reserved Instance cli public PR #4838

corquiri · 2017-11-06T22:46:31Z

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.

azuresdkci · 2017-11-06T22:46:58Z

View a preview at https://prompt.ws/r/Azure/azure-cli/4838
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'.

derekbekoe

There's no help for several commands. e.g. az reservations -h. Help is required for all commands.
A lot of params don't have help text either.

derekbekoe · 2017-11-06T23:18:23Z

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

+Release History
+===============
+
+0.1.0 (2017-11-06)


Remove the date. It's no longer required.

derekbekoe · 2017-11-06T23:19:30Z

src/command_modules/azure-cli-reservations/azure/__init__.py

@@ -0,0 +1 @@
+__import__('pkg_resources').declare_namespace(__name__)


Use like other modules:

import pkg_resources pkg_resources.declare_namespace(__name__)

Same with the other init files.

troydai · 2017-11-07T00:40:31Z

src/command_modules/azure-cli-reservations/azure/cli/command_modules/reservations/_help.py

+# --------------------------------------------------------------------------------------------
+
+from azure.cli.core.help_files import helps
+# pylint: disable=line-too-long


You don't need this. None of the lines in this file is too long.

troydai · 2017-11-07T00:41:36Z

src/command_modules/azure-cli-reservations/azure/cli/command_modules/reservations/_help.py

+          type: string
+        - name: --reservation-id-1 -1
+          type: string
+        - name: --reservation-id-2 -2


Is there a short summary for each of these parameters?

Summary added for all parameters

troydai · 2017-11-07T00:43:03Z

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

+
+def cli_reservation_update_reservation(client, reservation_order_id, reservation_id,
+                                       applied_scope_type, applied_scopes=None):
+    if applied_scopes is not None:


if applied_scopes:

corquiri · 2017-11-07T02:59:41Z

Got this error from CI build:
The following files should be added to doc_source_map.json:
src/command_modules/azure-cli-reservations/azure/cli/command_modules/reservations/_help.py

So I added this line to doc/sphinx/azhelpgen/doc_source_map.json
leaving comment here because this file is not under reservations and I wanted to confirm that it's okay to modify this file

troydai · 2017-11-07T17:09:24Z

@derekbekoe this PR looks good to me. Besides the CI.

derekbekoe · 2017-11-07T18:52:10Z

src/command_modules/azure-cli-reservations/README.rst

@@ -0,0 +1,7 @@
+Microsoft Azure CLI 'reservations' Command Module
+============================================


The heading should be full length of the title.
CI may fail due to this.

derekbekoe · 2017-11-07T18:54:50Z

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

+def cli_reservation_split_reservation(client, reservation_order_id, reservation_id, quantity_1, quantity_2):
+    split = SplitRequest([quantity_1, quantity_2], create_resource_id(reservation_order_id, reservation_id))
+    x = client.split(reservation_order_id, split)
+    return x


Just return client.split(reservation_order_id, split)?

derekbekoe · 2017-11-07T18:57:23Z

...-cli-reservations/azure/cli/command_modules/reservations/tests/test_reservations_commands.py

+    def test_get_applied_reservation_order_ids(self):
+        result = self.cmd('az reservations reservation-order-id list --subscription-id 00000000-0000-0000-0000-000000000000').get_output_in_json()
+        for order_id in result['reservationOrderIds']['value']:
+            self.assertTrue('/providers/Microsoft.Capacity/reservationorders/' in order_id)


Could use self.assertIn. This would give a clearer error message on failure.
Similar elsewhere.
https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertIn

derekbekoe · 2017-11-07T18:57:28Z

...-cli-reservations/azure/cli/command_modules/reservations/tests/test_reservations_commands.py

+        for order in reservation_order_list:
+            self._validate_reservation_order(order)
+            self.assertTrue('/providers/microsoft.capacity/reservationOrders/' in order['id'])
+            self.assertTrue(order['etag'] > 0)


assertGreater(a, b)

vmss: support basic tier of vms (Azure#4847) Azure IoT CA functionality (Azure#4804) * Adds support for X.509 Certificates in IoT Hub. * Modifies release notes and changes help link. * Cleans up .gitignore, and HISTORY.rst. Passes a single factory in commands.py. Moves test utilities. Adds a file completer. * Updates style after rebase based on additional linter restrictions. Reserved Instance cli public PR (Azure#4838) Handle dashes in extension names better (Azure#4839) Fix `az aks get-credentials` on Windows (Azure#4762) * Fix `az aks get-credentials` on Windows * Move k8s file merge logic to helper function appservice: support assign managed service identity to webapp/functionapp (Azure#4837) Extension examples small improvement (Azure#4852) Fixing appservice list-locations (Azure#4846) Add extension name to telemetry and UA header (Azure#4854) Fix Azure#4825. (Azure#4853) Extensions `az extension add` --yes param as flag (Azure#4858) Fix issue Azure#2752. (Azure#4859)

Reserved Instance cli public PR

0ab46ad

derekbekoe added this to the Sprint 26- Connect() milestone Nov 6, 2017

Unused import removed

606137b

derekbekoe reviewed Nov 6, 2017

View reviewed changes

help text added, style fixed

297a966

troydai reviewed Nov 7, 2017

View reviewed changes

Sean Oh added 3 commits November 6, 2017 17:02

More help added, small logic fix

3535b24

license headers added

f8ced60

reservations added to doc_source_map

fec89a9

Sean Oh added 2 commits November 7, 2017 10:04

History file edited

de8e7e6

setup and README corrected

1378f8d

derekbekoe approved these changes Nov 7, 2017

View reviewed changes

Sean Oh added 2 commits November 7, 2017 12:42

assert statement changed

a9eb580

azure-cli/setup.py edited

2364ad6

troydai merged commit f061602 into Azure:dev Nov 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reserved Instance cli public PR #4838

Reserved Instance cli public PR #4838

corquiri commented Nov 6, 2017 •

edited

Loading

azuresdkci commented Nov 6, 2017

derekbekoe left a comment

derekbekoe Nov 6, 2017

corquiri Nov 7, 2017

derekbekoe Nov 6, 2017

corquiri Nov 7, 2017

troydai Nov 7, 2017

corquiri Nov 7, 2017

troydai Nov 7, 2017

corquiri Nov 7, 2017

troydai Nov 7, 2017

corquiri Nov 7, 2017

corquiri commented Nov 7, 2017

troydai commented Nov 7, 2017 •

edited

Loading

derekbekoe Nov 7, 2017

derekbekoe Nov 7, 2017

derekbekoe Nov 7, 2017

derekbekoe Nov 7, 2017

		@@ -0,0 +1 @@
		__import__('pkg_resources').declare_namespace(__name__)

		@@ -0,0 +1,7 @@
		Microsoft Azure CLI 'reservations' Command Module
		============================================

Reserved Instance cli public PR #4838

Reserved Instance cli public PR #4838

Conversation

corquiri commented Nov 6, 2017 • edited Loading

General Guidelines

Command Guidelines

azuresdkci commented Nov 6, 2017

derekbekoe 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corquiri commented Nov 7, 2017

troydai commented Nov 7, 2017 • edited Loading

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

corquiri commented Nov 6, 2017 •

edited

Loading

troydai commented Nov 7, 2017 •

edited

Loading