Add new management cmdlets for Azure Location Based Services #5284

JCookit · 2018-01-16T22:02:59Z

Description

New component: management plane cmdlets for Azure Location Based Services (a new service).

Previously worked on in azure-powershell-pr https://github.com/Azure/azure-powershell-pr/pull/369 with feedback here. Have incorporated this feedback.

Based on the swagger here and the .net SDK here.

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.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

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.

JCookit · 2018-01-16T22:22:20Z

I see the break above --- something to do with 'mappings'? what's that?

markcowl

Initial review comments

markcowl · 2018-01-17T02:53:16Z

...edServices/Commands.LocationBasedServices/help/Get-AzureRmLocationBasedServicesAccountKey.md

+```
+
+### -InputObject
+{{Fill InputObject Description}}


Please fill in the descriptions for all parameters in the help

markcowl · 2018-01-17T02:55:01Z

...BasedServices/LocationBasedServices.Test/ScenarioTests/LocationBasedServicesAccountTests.ps1

+        # Test
+        $accountname = 'csa' + $rgname;
+        $skuname = 'S0';
+        $loc = 'West US';


regions should not be hardcoded - this will keep maintainers from being able to run and record tests in some environments

markcowl · 2018-01-17T02:55:20Z

...BasedServices/LocationBasedServices.Test/ScenarioTests/LocationBasedServicesAccountTests.ps1

+        New-AzureRmResourceGroup -Name $rgname -Location $loc;
+
+        $createdAccount = New-AzureRmLocationBasedServicesAccount -ResourceGroupName $rgname -Name $accountname -SkuName $skuname -Force;
+        Assert-NotNull $createdAccount;


Please add some additional validation of the account properties

markcowl · 2018-01-17T02:59:40Z

@JCookit A couple of things from a cursory review, will add some detail tomorrow

markcowl · 2018-01-17T03:03:54Z

@JCookit Also, the test is failing due to an app.config file in your test directory - you want to avoid using these for assembly binding redirects, as this can mask issues.

- updated help files with missing descriptions - removed app.config and obsolete newtonsoft dependency

JCookit · 2018-01-18T20:50:00Z

@markcowl
added missing help, removed app.config and changed hard-coded location.

- dynamically determine RG region in tests - also added PSSku object (was returning sdk model object before) - reran all unittests

markcowl · 2018-01-19T19:11:15Z

src/ResourceManager/LocationBasedServices/LocationBasedServices.Test/packages.config.bak

@@ -0,0 +1,28 @@
+<packages>


markcowl · 2018-01-20T04:08:51Z

src/ResourceManager/LocationBasedServices/AzureRM.LocationBasedServices.psd1

+# ProcessorArchitecture = ''
+
+# Modules that must be imported into the global environment prior to importing this module
+RequiredModules = @(@{ModuleName = 'AzureRM.Profile'; ModuleVersion = '3.3.2'; })


Needs to sue the current version (4.1.1)

markcowl · 2018-01-20T04:09:48Z

src/ResourceManager/LocationBasedServices/AzureRM.LocationBasedServices.psd1

+RequiredModules = @(@{ModuleName = 'AzureRM.Profile'; ModuleVersion = '3.3.2'; })
+
+# Assemblies that must be loaded prior to importing this module
+# RequiredAssemblies = @()


Need to add your management assembly, and any other assemblies not used by other modules here (see the other psd1 files for samples)

ok. should just be our management assembly.

markcowl · 2018-01-20T04:13:04Z

src/ResourceManager/LocationBasedServices/AzureRM.LocationBasedServices.psd1

+
+        # A URL to an icon representing this module.
+        # IconUri = ''
+


Need to add

Prerelease='preview'

here

inside the PSData? ok.

markcowl · 2018-01-20T04:14:31Z

...s.LocationBasedServices/LocationBasedServicesAccount/GetAzureLocationBasedServicesAccount.cs

+    /// <summary>
+    /// Get Location Based Services Account by name, all accounts under resource group or all accounts under the subscription
+    /// </summary>
+    [Cmdlet(VerbsCommon.Get, LocationBasedServicesAccountNounStr), OutputType(typeof(PSLocationBasedServicesAccount))]


Need a DefaultParameterSetName, since there is more than one parameter set

ok. does it matter which one? i picked.

markcowl · 2018-01-20T04:27:20Z

.../Commands.LocationBasedServices/Microsoft.Azure.Commands.LocationBasedServices.format.ps1xml

+                <PropertyName>AccountName</PropertyName>
+              </TableColumnItem>
+              <TableColumnItem>
+                <PropertyName>Id</PropertyName>


No other data that the user would liek to know about the account?

At this time, our accounts themselves are dead simple. Nothing else interesting (at least that's exposed in the SDK... yet)

markcowl · 2018-01-20T04:28:54Z

...edServices/Commands.LocationBasedServices/help/Get-AzureRmLocationBasedServicesAccountKey.md

+
+PrimaryKey                                  SecondaryKey
+----------                                  ------------
+yKp9encJJQyJMlxgj0gc4n5Iz9DheQyxt020Rwnd3a8 AeE4SoxPk6BrYZgRlqA8uuchLXzkTEHTpm3_iMaml2g


You might want to ensure that these are obviously redeacted (xxxxxxxx, yyyyyyyyyy, ***********, etc.)

ya wasn't sure what best practice was. These keys are just examples; they won't work.
but i can redact.

markcowl · 2018-01-20T04:29:59Z

src/ResourceManager/LocationBasedServices/Commands.LocationBasedServices/packages.config

+<?xml version="1.0" encoding="utf-8"?>
+<packages>
+  <package id="Microsoft.Azure.Management.LocationBasedServices" version="1.0.0" targetFramework="net452" />
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.10" targetFramework="net452" />


don't need any entries other than your management client

markcowl · 2018-01-20T04:32:17Z

src/ResourceManager/LocationBasedServices/LocationBasedServices.sln

+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.Common.Authorization", "..\..\Common\Commands.Common.Authorization\Commands.Common.Authorization.csproj", "{24508E26-154D-47F1-80EE-439BF0710996}"
+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.Common.Graph.RBAC", "..\..\Common\Commands.Common.Graph.RBAC\Commands.Common.Graph.RBAC.csproj", "{269ACF73-0A34-42DC-AB9C-4B15931A489D}"


Please include Commands.Common.Network, and Commands.Common.Storage projects, they are necessary for building Profile

added Network (Storage was already there). fwiw, this isn't documented that i can find.

markcowl · 2018-01-20T04:33:22Z

src/ResourceManager/LocationBasedServices/ChangeLog.md

+        - Additional information about change #1
+-->
+## Current Release
+* Integrate with Cognitive Services Management SDK version 2.0.0.


?? I don;t think this impacts this library

you're right. bad copy/paste from the project i was modeling after.

- fixed various metadata - added ResourceId parameter sets & DefaultParameterSets - updated help - improved and reran unittests

markcowl · 2018-01-26T17:08:23Z

on demand run here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/386/

JCookit added 3 commits January 16, 2018 13:27

initial checkin to public repo for new cmdlets

74ae8b7

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

623f7f0

merge from upstream and rerun unittests

fd6415f

azuresdkci added the needs-review label Jan 16, 2018

azuresdkci assigned shahabhijeet Jan 16, 2018

maddieclayton assigned markcowl and unassigned shahabhijeet Jan 17, 2018

added mapping rule

1927989

markcowl requested changes Jan 17, 2018

View reviewed changes

maddieclayton assigned JCookit and unassigned markcowl Jan 17, 2018

maddieclayton added needs-revision and removed needs-review labels Jan 17, 2018

per feedback:

ea2c7cb

- updated help files with missing descriptions - removed app.config and obsolete newtonsoft dependency

per feedback:

94f7f98

- dynamically determine RG region in tests - also added PSSku object (was returning sdk model object before) - reran all unittests

markcowl added needs-review and removed needs-revision labels Jan 19, 2018

markcowl assigned markcowl and unassigned JCookit Jan 19, 2018

markcowl requested changes Jan 20, 2018

View reviewed changes

markcowl added needs-revision and removed needs-review labels Jan 20, 2018

markcowl assigned JCookit and unassigned markcowl Jan 20, 2018

per feedback:

b775d54

- fixed various metadata - added ResourceId parameter sets & DefaultParameterSets - updated help - improved and reran unittests

JCookit added the needs-review label Jan 21, 2018

JCookit removed the needs-revision label Jan 21, 2018

JCookit assigned markcowl Jan 21, 2018

markcowl approved these changes Jan 26, 2018

View reviewed changes

markcowl removed the needs-review label Jan 26, 2018

markcowl unassigned markcowl and JCookit Jan 26, 2018

markcowl added the Waiting for On Demand label Jan 26, 2018

markcowl merged commit 4501b79 into Azure:preview Jan 29, 2018

Add new management cmdlets for Azure Location Based Services #5284

Add new management cmdlets for Azure Location Based Services #5284

Uh oh!

Conversation

JCookit commented Jan 16, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

JCookit commented Jan 16, 2018

Uh oh!

markcowl 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!

markcowl commented Jan 17, 2018

Uh oh!

markcowl commented Jan 17, 2018

Uh oh!

JCookit commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

markcowl commented Jan 26, 2018

Uh oh!

Uh oh!

JCookit commented Jan 16, 2018 •

edited by azuresdkci

Loading

JCookit commented Jan 18, 2018 •

edited

Loading