Introduce Azure Maps cmdlets #6259

jp94 · 2018-05-21T03:39:34Z

Description

Introduce Azure maps cmdlets.

Most are renaming LocationBasedServices cmdlets to Maps.
However, I had to attach a Microsoft.Azure.Management.ResourceManager.ResourceManagementClient to pass test cases.

Cmdlet review:
https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/21

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

maddieclayton · 2018-05-21T16:25:03Z

@jp94 Looks like the reason the build is failing is because the online help link is not added to the help files: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/4635/ (ValidateHelpIssues.csv).

maddieclayton · 2018-05-21T18:24:59Z

@jp94 Doing a comparison of the file structures: it looks like you have forgotten to remove a folder in the Test/SessionsRecords folder:

Previous (correct): https://github.com/Azure/azure-powershell/tree/preview/src/ResourceManager/LocationBasedServices/LocationBasedServices.Test/SessionRecords
Now (incorrect): https://github.com/jp94/azure-powershell/tree/preview/src/ResourceManager/Maps/Maps.Test/SessionRecords

Looks like you need to remove the LocationBasedServices.Test folder.

jp94 · 2018-05-21T20:42:13Z

@maddieclayton
Do you mean to add https://docs.microsoft.com/en-us/powershell/module/azurerm.maps
to AzureRM.Maps.md and https://docs.microsoft.com/en-us/powershell/module/azurerm.maps/get-azurermmapsaccount to Get-AzureRmMapsAccount.md etc.? Since the links do not exist yet, do I also submit a PR to azure-docs-powershell?

For second comment, I think you were looking at the wrong branch jp94:preview rather than jp94:maps.

Also, I realized that I've submitted a PR with renaming LocationBasedServices to Maps.
Would it be better to cancel this and submit two PRs where one introduces Maps and another deprecates LocationBasedServices? We did send out emails to our customers that LocationBasedServices PS cmdlets (total 5 downloads) will no longer work prior to going forward with the breaking changes.

Thanks.

maddieclayton · 2018-05-21T18:28:50Z

src/ResourceManager/Maps/Maps.Test/ScenarioTests/MapsAccountTests.ps1

+        # Test
+        $accountname = 'ps-' + $rgname;
+        $skuname = 'S0';
+        $location = 'West US';


This should be Get-Location 'Microsoft.Maps' 'accounts' 'West US';

Apply everywhere.

Changing to $location = Get-Location -providerNamespace 'Microsoft.Maps' -resourceType 'accounts' -preferredLocation 'West US'; or Get-Location 'Microsoft.Maps' 'accounts' 'West US'; yields this error:

Test Name: Microsoft.Azure.Commands.Maps.Test.ScenarioTests.MapsAccountTests.TestGetAccounts Test Outcome: Failed Result Message: System.Management.Automation.ActionPreferenceStopException : The running command stopped because the preference variable "ErrorActionPreference" or common parameter is set to Stop: Exception of type 'System.Management.Automation.PSInvalidOperationException' was thrown.

Perhaps I've mis-configured something? In Maps.Test/bin/Debug/Common.ps1, Get-Location function is there, but looks like it's failing to recognize it.

Hmm, I can't find anything in your code that would cause this error. My recommendation would be to do a git clean -xdf (after committing all local changes, as this will erase all local changes), rebuilding and rerunning the test. I would think that something happened during all the renaming that is causing the function to not be found. Ping me if you are still having problems after this.

maddieclayton · 2018-05-21T18:29:24Z

src/ResourceManager/Maps/Commands.Maps/help/AzureRM.Maps.md

+---
+Module Name: AzureRM.Maps
+Module Guid: bf60f35d-6c0b-42f2-be30-eb333a31279d
+Download Help Link: None


Should have the download link: https://docs.microsoft.com/en-us/powershell/module/azurerm.locationbasedservices

Is it okay to reference the old locationbasedservices link over maps?

Oops no, please use https://docs.microsoft.com/en-us/powershell/module/azurerm.maps

maddieclayton · 2018-05-21T18:35:25Z

src/ResourceManager/Maps/AzureRM.Maps.psd1

@@ -12,7 +12,7 @@
 # RootModule = ''

 # Version number of this module.
-ModuleVersion = '1.0.1'
+ModuleVersion = '2.0.0'


I think you'd want to start as 1.0.0 for this module (since it will be new to the gallery) - let me know if you disagree.

maddieclayton · 2018-05-21T18:36:05Z

src/ResourceManager/Maps/AzureRM.Maps.psd1

+CmdletsToExport = 'Get-AzureRmMapsAccount', 
+               'New-AzureRmMapsAccount', 
+               'Remove-AzureRmMapsAccount', 
+               'Set-AzureRmMapsAccount', 


Was this just missing before?

That's interesting.
I see Set-AzureRmLocationBasedServicesAccount in netcore, but not on the original psd1. Should I remove that on both files?

If there is no Set, it should be removed everywhere.

maddieclayton · 2018-05-21T18:37:07Z

src/ResourceManager/Maps/AzureRM.Maps.psd1


        # Prerelease string of this module
-        Prerelease = 'preview'
+        Prerelease = ''


Please comment this out (add a # to the beginning).

maddieclayton · 2018-05-21T19:04:01Z

src/ResourceManager/Maps/Maps.Test/Commands.Maps.Test.csproj

@@ -71,11 +75,15 @@
    <Reference Include="Microsoft.IdentityModel.Clients.ActiveDirectory.WindowsForms, Version=2.28.3.860, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
      <HintPath>..\..\..\packages\Microsoft.IdentityModel.Clients.ActiveDirectory.2.28.3\lib\net45\Microsoft.IdentityModel.Clients.ActiveDirectory.WindowsForms.dll</HintPath>
    </Reference>
+    <Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


Remove this new reference - in common targets.

maddieclayton · 2018-05-21T21:01:47Z

src/ResourceManager/Maps/Maps.Test/ScenarioTests/MapsAccountTests.ps1

+        Assert-AreEqual $createdAccount.Location $createdAccountAgain.Location;
+        Assert-AreEqual $createdAccount.Sku.Name $createdAccountAgain.Sku.Name;
+
+        Retry-IfException { Remove-AzureRmMapsAccount -ResourceGroupName $rgname -Name $accountname -Confirm:$false; }


Please add a check here to ensure the account was deleted (make sure get returns nothing)

Looks like the check is in under function Test-RemoveAzureRmMapsAccount at line 73 & 83. I think this check is good, but I can also add to all other test cases if needed.

As long as remove has an assert somewhere in the tests, no need to add the check here. 👍

maddieclayton · 2018-05-21T21:05:55Z

src/ResourceManager/Maps/Maps.Test/TestController.cs

+            return context.GetServiceClient<MapsManagementClient>(RestTestFramework.TestEnvironmentFactory.GetTestEnvironment());
+        }
+
+        private LegacyResourceManagementClient GetLegacyResourceManagementClient(RestTestFramework.MockContext context)


What changed to mean that you need this? Did you change the tests?

No changes were introduced.
If I recall, it worked ~2 months ago, but broke recently after pulling the latest code from the preview branch. This includes unmodified LocationBasedServices cmdlet tests, that yields the same error like shown below:

Test Name: Microsoft.Azure.Commands.Maps.Test.ScenarioTests.MapsAccountTests.TestRemoveAccount Test Outcome: Failed Result Message: System.Management.Automation.ActionPreferenceStopException : The running command stopped because the preference variable "ErrorActionPreference" or common parameter is set to Stop: TestManagementClientHelper class wasn't initialized with the ResourceManagementClient client.

Interesting. This is fine then.

maddieclayton · 2018-05-21T21:06:08Z

src/ResourceManager/Maps/Maps.Test/packages.config

  <package id="Microsoft.Bcl" version="1.1.9" targetFramework="net45" />
  <package id="Microsoft.Bcl.Async" version="1.0.168" targetFramework="net45" />
  <package id="Microsoft.Bcl.Build" version="1.0.14" targetFramework="net45" />
  <package id="Microsoft.IdentityModel.Clients.ActiveDirectory" version="2.28.3" targetFramework="net45" />
  <package id="Microsoft.Net.Http" version="2.2.28" targetFramework="net45" />
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.10" targetFramework="net452" />


Remove this.

maddieclayton · 2018-05-21T21:06:24Z

src/ResourceManager/Maps/Maps.sln

@@ -6,6 +6,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.ResourceManager.Co
 EndProject
 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.Resources", "..\Resources\Commands.Resources\Commands.Resources.csproj", "{E1F5201D-6067-430E-B303-4E367652991B}"
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.Tags", "..\Tags\Commands.Tags\Commands.Tags.csproj", "{2493A8F7-1949-4F29-8D53-9D459046C3B8}"


Why was this added?

maddieclayton · 2018-05-21T21:12:05Z

@jp94 To answer questions from above:

No need to open a new PR in docs. Once the release is live, the docs team will pull the docs from our repo. You just need to add the links (what you said about which links to add is correct).

The current PR removing the old module and adding the new simulataneous is much easier to review because it only shows the changes. Please keep like this.

maddieclayton · 2018-05-22T21:10:07Z

src/ResourceManager/Maps/Maps.Test/Properties/AssemblyInfo.cs

@@ -47,6 +47,6 @@
 // You can specify all the values or you can default the Build and Revision Numbers 
 // by using the '*' as shown below:

-[assembly: AssemblyVersion("1.0.0")]
-[assembly: AssemblyFileVersion("1.0.0")]
+[assembly: AssemblyVersion("2.0.0")]


This should be 1.0.0 as well.

maddieclayton · 2018-05-22T21:11:13Z

src/ResourceManager/Maps/Maps.sln

@@ -56,6 +56,10 @@ Global
 		{E1F5201D-6067-430E-B303-4E367652991B}.Debug|Any CPU.Build.0 = Debug|Any CPU
 		{E1F5201D-6067-430E-B303-4E367652991B}.Release|Any CPU.ActiveCfg = Release|Any CPU
 		{E1F5201D-6067-430E-B303-4E367652991B}.Release|Any CPU.Build.0 = Release|Any CPU
+		{2493A8F7-1949-4F29-8D53-9D459046C3B8}.Debug|Any CPU.ActiveCfg = Debug|Any CPU


Remove these four lines as well - this is from the tags.common project.

maddieclayton · 2018-05-22T23:48:15Z

@jp94 Looks like the build is failing because your "Remove" cmdlet does not implement passthru. To add this, simply add a switchparameter "PassThru", and when the parameter is specified, print a boolean indicating the success or failure of the removal. Let me know if you have any questions about this.

jp94 · 2018-05-23T03:45:22Z

@maddieclayton
Sorry about that (forgot to run msbuild build.proj after merging with Azure/preview).
I want to confirm the changes below before pushing.

    [...]
+   [Cmdlet(VerbsCommon.Remove, MapsAccountNounStr, DefaultParameterSetName = NameParameterSet, SupportsShouldProcess = true), OutputType(typeof(bool))]
    public class RemoveAzureMapsAccount : MapsAccountBaseCmdlet
    {
        [...]

+       [Parameter(Mandatory = false)]
+       public SwitchParameter PassThru { get; set; }

        public override void ExecuteCmdlet()
        {
            base.ExecuteCmdlet();

            RunCmdLet(() =>
            {
                [...]

                if (!string.IsNullOrEmpty(rgName)
                    && !string.IsNullOrEmpty(name)
                    && ShouldProcess(name, string.Format(CultureInfo.CurrentCulture, Resources.RemoveAccount_ProcessMessage, name)))
                {
                    this.MapsClient.Accounts.Delete(rgName, name);
+                   if (PassThru.IsPresent)
+                   {
+                       WriteObject(true);
+                   }
                }
            });
        }
    }
}

I was able to complete the build without errors, and you can see the changes noted by + on the left side.
Original
Thanks.

maddieclayton · 2018-05-23T16:10:09Z

@jp94 That looks great! My only comment would be to add a "HelpMessage" to the PassThru parameter.

maddieclayton · 2018-05-23T18:28:30Z

@jp94 You will also need to regenerate the help file, so the new parameter gets picked up. Thanks!

maddieclayton · 2018-05-23T19:25:24Z

src/ResourceManager/Maps/Commands.Maps/help/Remove-AzureRmMapsAccount.md

@@ -106,6 +106,19 @@ Accept pipeline input: True (ByPropertyName)
 Accept wildcard characters: False
 ```

+### -PassThru
+Return whether the specified account was successfully deleted or not.```yaml


Please move this yaml tag down to the next line (this is a known issue in platyps)

Yep.. I fixed the issue, and am running the whole msbuild build.proj again right now.

maddieclayton · 2018-05-23T19:42:48Z

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

jp94 added 8 commits May 7, 2018 01:39

[Maps] Rename folders and files from LocationBasedServices

a8a4744

[Maps] Rename help file names

1fda5d6

[Maps] Renaming variables, strings, descriptions, etc.

1074ff8

[Maps] Use Maps SDK over LocationBasedServices SDK

b56b091

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

555e81e

[Maps] Add legacy ResourceManager & update test recordings

29d1123

[Maps] Add legacy ResourceManager & update test recordings

49e6765

Merge remote-tracking branch 'Azure/preview' into maps

c348a3e

azuresdkci added the needs-review label May 21, 2018

maddieclayton self-assigned this May 21, 2018

maddieclayton suggested changes May 21, 2018

View reviewed changes

maddieclayton added Merge Conflicts needs-revision and removed needs-review labels May 21, 2018

maddieclayton assigned jp94 and unassigned maddieclayton May 21, 2018

jp94 added 2 commits May 22, 2018 13:42

[Maps] Resolving merge revision requests

d2f2f81

Merge remote-tracking branch 'Azure/preview' into maps

2cf517c

maddieclayton removed the Merge Conflicts label May 22, 2018

maddieclayton suggested changes May 22, 2018

View reviewed changes

[Maps] Resolving merge revision requests (2)

1c74602

maddieclayton added needs-review and removed needs-revision labels May 22, 2018

maddieclayton assigned maddieclayton and unassigned jp94 May 22, 2018

maddieclayton added needs-revision and removed needs-review labels May 22, 2018

maddieclayton assigned jp94 and unassigned maddieclayton May 22, 2018

[Maps] Add PassThru to Remove method, resolve build failure

26b440e

jp94 added 2 commits May 23, 2018 11:51

[Maps] Update help files to reflect -PassThru param addition

1324e23

[Maps] Fix trailing yaml string for PassThru

b56ec7a

maddieclayton suggested changes May 23, 2018

View reviewed changes

maddieclayton approved these changes May 23, 2018

View reviewed changes

maddieclayton added Waiting for On Demand and removed needs-revision labels May 23, 2018

maddieclayton unassigned jp94 May 23, 2018

maddieclayton merged commit 9a10811 into Azure:preview May 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Azure Maps cmdlets #6259

Introduce Azure Maps cmdlets #6259

jp94 commented May 21, 2018 •

edited

Loading

maddieclayton commented May 21, 2018

maddieclayton commented May 21, 2018

jp94 commented May 21, 2018

maddieclayton May 21, 2018

jp94 May 22, 2018

maddieclayton May 22, 2018

maddieclayton May 21, 2018

jp94 May 21, 2018

maddieclayton May 21, 2018

maddieclayton May 21, 2018

maddieclayton May 21, 2018

jp94 May 21, 2018

maddieclayton May 22, 2018

maddieclayton May 21, 2018

maddieclayton May 21, 2018

maddieclayton May 21, 2018

jp94 May 21, 2018

maddieclayton May 22, 2018

maddieclayton May 21, 2018

jp94 May 21, 2018

maddieclayton May 22, 2018

maddieclayton May 21, 2018

maddieclayton May 21, 2018

jp94 May 21, 2018

maddieclayton commented May 21, 2018

maddieclayton May 22, 2018

maddieclayton May 22, 2018

maddieclayton commented May 22, 2018

jp94 commented May 23, 2018

maddieclayton commented May 23, 2018

maddieclayton commented May 23, 2018

maddieclayton May 23, 2018

jp94 May 23, 2018

maddieclayton commented May 23, 2018

Introduce Azure Maps cmdlets #6259

Introduce Azure Maps cmdlets #6259

Conversation

jp94 commented May 21, 2018 • edited Loading

Description

Checklist

maddieclayton commented May 21, 2018

maddieclayton commented May 21, 2018

jp94 commented May 21, 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

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

maddieclayton commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maddieclayton commented May 22, 2018

jp94 commented May 23, 2018

maddieclayton commented May 23, 2018

maddieclayton commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maddieclayton commented May 23, 2018

jp94 commented May 21, 2018 •

edited

Loading