On-board Maps #6303

jp94 · 2018-05-07T22:43:28Z

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.

Refer to #5716
We were having a service rename from Location Based Services to Maps.
Since the timeline was short and that there was an embargo on "Azure Maps" name, the PR was canceled.
We are ready to on-board Azure Maps command module now.

@lmazuel @tjprescott

- Add the Preview Terms agreement requirement - Remove location, custom_header, and raw parameters for account operations.

[Fix] --key-type, -t are now --key

[Refactor] get_account command now specifically throws error only if it's a 404 response.

[Output] Added recordings

promptws · 2018-05-07T22:43:49Z

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

tjprescott · 2018-05-07T22:53:04Z

@jp94 which dev are you working with on the CLI side?

lmazuel · 2018-05-07T23:03:47Z

@tjprescott package is released:
https://pypi.org/project/azure-mgmt-maps/

jp94 · 2018-05-08T01:21:37Z

The build error states,

AssertionError: Make sure /home/travis/build/Azure/azure-cli/src/command_modules/azure-cli-maps/azure_bdist_wheel.py is correct. It should look like /home/travis/build/Azure/azure-cli/src/azure-cli-core/azure_bdist_wheel.py

I diff checked, but it says they are identical.

troydai · 2018-05-08T15:55:13Z

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

+logger = get_logger(__name__)
+
+
+# pylint: disable=line-too-long


Break line instead of disabling pylint.

tjprescott · 2018-05-08T16:58:38Z

See #5716 for intial review.

tjprescott · 2018-05-08T16:59:52Z

@derekbekoe what is the fix for the bdist wheel issue?

tjprescott

Overall LGTM. A couple feedback points for your consideration. Would like @sptramer to review the help entries and then I'll sign-off.

tjprescott · 2018-05-08T17:01:50Z

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

+                                     completer=get_resource_name_completion_list(
+                                         'Microsoft.Maps/accounts'),
+                                     help='The name of the Maps Account',
+                                     validator=validate_account_name)


As your module grows, this will get very cumbersome. Most modules simply put one entry on one line and disable pylint's "line-too-long" warning. Up to you. Opinions vary (mine is pretty clearly in favor of long lines)

tjprescott · 2018-05-08T17:02:34Z

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

+                   options_list=['--sku', '-s'],
+                   help='The name of the SKU, in standard format (such as S0).',
+                   arg_type=get_enum_type(['S0']),
+                   required=False)


You shouldn't need required=False. If your method signature has sku_name=None the CLI will already deduce this.

tjprescott · 2018-05-08T17:04:34Z

src/command_modules/azure-cli-maps/azure/cli/command_modules/maps/validators.py

+    if not ACCOUNT_NAME_MIN_LENGTH <= char_len <= ACCOUNT_NAME_MAX_LENGTH:
+        raise CLIError("Input account-name is invalid. Account name character length must be between 2 and 64.")
+    if not re.match(ACCOUNT_NAME_REGEX, account_name):
+        raise CLIError("Input account-name is invalid. Allowed regex: " + ACCOUNT_NAME_REGEX.pattern +


If your service error is clear enough, I'd recommend you avoid putting this client-side validation in. Otherwise, if the service later becomes more forgiving, the CLI will artificially restrict valid names. If the service error is not good enough, then this is an okay workaround.

You make a good point here.

derekbekoe · 2018-05-08T17:11:24Z

@jp94 This is the azure_bdist_wheel.py file in your PR - https://github.com/Azure/azure-cli/pull/6303/files#diff-3423f41d513e4e7b0cb1d04b1d23b5c7.

This is the one that it compares against - https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure_bdist_wheel.py.

They are not the same.

codecov-io · 2018-05-08T21:25:37Z

Codecov Report

❗ No coverage uploaded for pull request base (dev@8b1c318). Click here to learn what that means.
The diff coverage is 0%.

@@          Coverage Diff          @@
##             dev   #6303   +/-   ##
=====================================
  Coverage       ?      0%           
=====================================
  Files          ?      11           
  Lines          ?     133           
  Branches       ?       9           
=====================================
  Hits           ?       0           
  Misses         ?     133           
  Partials       ?       0

Impacted Files	Coverage Δ
...ure-cli-maps/azure/cli/command_modules/__init__.py	`0% <0%> (ø)`
...e-cli-maps/azure/cli/command_modules/maps/_help.py	`0% <0%> (ø)`
.../azure/cli/command_modules/maps/_client_factory.py	`0% <0%> (ø)`
...ommand_modules/azure-cli-maps/azure_bdist_wheel.py	`0% <0%> (ø)`
...mmand_modules/azure-cli-maps/azure/cli/__init__.py	`0% <0%> (ø)`
src/command_modules/azure-cli-maps/setup.py	`0% <0%> (ø)`
...-cli-maps/azure/cli/command_modules/maps/custom.py	`0% <0%> (ø)`
...c/command_modules/azure-cli-maps/azure/__init__.py	`0% <0%> (ø)`
...li-maps/azure/cli/command_modules/maps/__init__.py	`0% <0%> (ø)`
...li-maps/azure/cli/command_modules/maps/commands.py	`0% <0%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b1c318...19b2fe9. Read the comment docs.

Removed validators.py. Will be adding checks in REST API specs and our backend routes to avoid redundancy. Updated azure_bdist_wheel.py Introduced minor fixes.

jp94 · 2018-05-09T07:28:23Z

Looks like I didn't pull the latest code from the dev branch, and I was still seeing the old azure_bdist_wheel.py

Removed validators.py. This results in error stacktrace with a generic response "Operation returned an invalid status code 'Bad Request'". We will be adding checks in REST API specs and our backend routes to avoid redundancy. I'll talk to my manager about getting this in. Let me know if this change should be reverted for the duration, or if this is okay.

sptramer

Some help copyedit changes.

sptramer · 2018-05-09T15:54:51Z

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

+
+helps['maps'] = """
+    type: group
+    short-summary: Manage Azure Maps accounts.


Identical to the maps account description, which indicates the subgroup is unnecessary. If the toplevel namespace is used for future expansion, change this description.

So should I remove the helps['maps'] or helps['maps account']?
I removed helps['maps'], because removing helps['maps account'] makes "az maps account -h" to not give a short summary.

sptramer · 2018-05-09T15:57:18Z

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

+
+helps['maps account list'] = """
+    type: command
+    short-summary: Show all Maps accounts in a Subscription or in a Resource Group.


Lowercase subscription and resource group. Only capitalize Maps if used as Azure Maps.

I've lowercase'd all "Maps" to "maps", except for "Azure Maps".
This includes "the Maps APIs" to "the maps APIs" as well correct?

Maps APIs refers directly to a product name, so Maps should be capitalized there.

sptramer · 2018-05-09T15:59:00Z

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

+helps['maps account create'] = """
+    type: command
+    short-summary: Create a Maps account.
+    long-summary: |


Not sure this long summary is needed; does not add information.

sptramer · 2018-05-09T15:59:31Z

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

+helps['maps account keys list'] = """
+    type: command
+    short-summary: List the keys to use with the Maps APIs.
+    long-summary: |


Remove the first sentence. long-description is always a supplement to short-description.

sptramer · 2018-05-09T16:02:24Z

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

+helps['maps account keys renew'] = """
+    type: command
+    short-summary: Renew either the primary or secondary key for use with the Maps APIs.
+    long-summary: |


Rewrite as This command immediately invalidates old API keys. Only the renewed keys can be used to connect to maps.

sptramer · 2018-05-09T16:02:41Z

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

+    # Argument Definition
+    maps_name_type = CLIArgumentType(options_list=['--name', '-n'],
+                                     completer=get_resource_name_completion_list('Microsoft.Maps/accounts'),
+                                     help='The name of the Maps Account')


Lowercase maps and account

sptramer · 2018-05-09T16:03:14Z

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

+    with self.argument_context('maps account') as c:
+        c.argument('sku_name',
+                   options_list=['--sku', '-s'],
+                   help='The name of the SKU, in standard format (such as S0).',


Remove everything after the , - the valid values are enumerated in help.

sptramer · 2018-05-09T16:05:26Z

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

+
+    with self.argument_context('maps account keys renew') as c:
+        c.argument('key_type',
+                   options_list=['--key'],


Is the help for this argument pulled from elsewhere?

This help is being pulled from the Azure Python SDK (Maps).

Command az maps account keys renew: Renew either the primary or secondary key for use with the maps APIs. This command immediately invalidates old API keys. Only the renewed keys can be used to connect to maps. Arguments --key [Required]: Whether the operation refers to the primary or secondary key. Allowed values: primary, secondary. --name -n [Required]: The name of the maps account. --resource-group -g : Resource group name.

Looks good to me.

Misinterpreted PR comment.

jp94 · 2018-05-12T02:35:29Z

I've been trying to resolve azdev verify module-load-perf (PURPOSE='Module Load Performance').
I was able to fix the servicefabric module failing by reverting merge remote-tracking branch 'upstream/dev' into rename. Do you know what may have caused it to fail?
Obviously, I don't want to push the merge reverts.

For example, my previous commit passed for module-load-perf: https://travis-ci.org/Azure/azure-cli/jobs/376134725 But after merging with dev branch, it failed: https://travis-ci.org/Azure/azure-cli/jobs/376565890

troydai · 2018-05-14T16:10:13Z

@tjprescott the module local perf test keep failing in service fabric. Any idea?

tjprescott · 2018-05-14T17:58:53Z

@troydai I sent an email about it. It is essentially the same problem, yet identified, that caused us to raise the SQL threshold. Now you'll note SQL is at 3 ms and the problem manifests in servicefabric. I sent @jp94 the workaround.

troydai · 2018-05-15T16:12:41Z

thanks, guys.

jp94 and others added 25 commits February 23, 2018 15:12

Introduce LocationBasedServices command_module

418b2a1

Introduce test cases for LocationBasedServices command_module

8bbcd51

[Refactor] Renamed lbs to locationbasedservices

18d9384

[Refactor] Renamed lbs to locationbasedservices

cb738db

[Legal] Add the Preview Terms agreement requirement

b74cb79

- Add the Preview Terms agreement requirement - Remove location, custom_header, and raw parameters for account operations.

[Test] Add more strict assertion rule for key validation.

409b03b

[Test] Add test for tags parameter

ed3ca01

[Test] Added an additional resource group.

81e4fb5

[Test] Init recordings

0d11a6d

Merge remote-tracking branch 'upstream/dev' into dev

87accc7

[Pylint] Specify string format arguments as logging function parameters

9e5051f

[Refactor] Update description in _help to match the general template

77a9f22

[Refactor] General template mismatch fix for 'key'

98d973e

[Feature] Introduce search by ids

5853331

[Fix] --key-type, -t are now --key

[Temporary] Throw an exception on 'account show' command, when nonexist

4d930f8

[Test] 'account show' on non-existent account should not return empty.

d36abf9

[Feature] Introduce update command

b79cb10

[Refactor] get_account command now specifically throws error only if it's a 404 response.

[Test] Add test for update and search by id

75795e6

[Output] Added recordings

Merge remote-tracking branch 'upstream/dev' into dev

100df29

[Refactor] Rename generic client (sync with updated Swagger)

1fd7bef

[Refactor] Use default SDK's function

1aee6b2

[Refactor] Remove Preview Terms & docs in custom.py

35976e7

[Maps] Rename LocationBasedServices to Maps

e7784a1

[Maps] Use Maps SDK from LocationBasedServices SDK

f9ca47d

[Maps] Update dependencies

52b9dde

tjprescott added the New Service label May 7, 2018

[Maps] Resolving build error (doc_source_map)

c0c8bb8

troydai reviewed May 8, 2018

View reviewed changes

tjprescott self-requested a review May 8, 2018 16:58

tjprescott added this to the Sprint 37 milestone May 8, 2018

tjprescott reviewed May 8, 2018

View reviewed changes

tjprescott requested a review from sptramer May 8, 2018 17:05

Merge remote-tracking branch 'upstream/dev' into rename

498ca9c

jp94 added 2 commits May 8, 2018 23:53

Merge remote-tracking branch 'upstream/dev' into rename

4176c07

[Maps] Resolving PR issues.

c6c5c02

Removed validators.py. Will be adding checks in REST API specs and our backend routes to avoid redundancy. Updated azure_bdist_wheel.py Introduced minor fixes.

sptramer suggested changes May 9, 2018

View reviewed changes

[Maps] Revise help text

31f7a3a

sptramer approved these changes May 9, 2018

View reviewed changes

jp94 added 2 commits May 9, 2018 13:56

[Maps] Adding back help docs for maps account

d49f8ad

Misinterpreted PR comment.

Merge remote-tracking branch 'upstream/dev' into rename

1f3d2c1

tjprescott approved these changes May 14, 2018

View reviewed changes

troydai approved these changes May 14, 2018

View reviewed changes

[Load Test] replace sql with servicefabric

19b2fe9

tjprescott merged commit 36b4b3d into Azure:dev May 14, 2018

haroldrandom added the New Service label Oct 25, 2019

		logger = get_logger(__name__)


		# pylint: disable=line-too-long

On-board Maps #6303

On-board Maps #6303

Uh oh!

Conversation

jp94 commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promptws commented May 7, 2018

Uh oh!

tjprescott commented May 7, 2018

Uh oh!

lmazuel commented May 7, 2018

Uh oh!

jp94 commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjprescott commented May 8, 2018

Uh oh!

tjprescott commented May 8, 2018

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekbekoe commented May 8, 2018

Uh oh!

codecov-io commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jp94 commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sptramer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jp94 May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jp94 May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jp94 commented May 12, 2018

Uh oh!

troydai commented May 14, 2018

Uh oh!

tjprescott commented May 14, 2018

Uh oh!

jp94 commented May 7, 2018 •

edited

Loading

codecov-io commented May 8, 2018 •

edited

Loading

jp94 commented May 9, 2018 •

edited

Loading

jp94 May 9, 2018 •

edited

Loading

jp94 May 9, 2018 •

edited

Loading