Tab completer for existing resources (Iss #5415) #6163

vladimir-shcherbakov · 2018-05-08T22:39:09Z

Description

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

vladimir-shcherbakov · 2018-05-10T20:20:16Z

iss #5415

maddieclayton · 2018-05-21T15:13:06Z

@vladimir-shcherbakov is this ready for review? If not, please add the "Do not merge" tag.

vladimir-shcherbakov · 2018-05-22T23:47:10Z

@maddieclayton
Yes, it's ready.

maddieclayton · 2018-06-06T00:45:45Z

...urceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceIdCompleter.cs

+                }
+
+                var instance = AzureSession.Instance;
+                var client = instance.ClientFactory.CreateCustomArmClient<ResourceManagementClient>(


I fixed the CreateArmClient so we can just use this call now: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceGroupCompleter.cs#L51

maddieclayton · 2018-06-06T00:46:08Z

...urceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceIdCompleter.cs

+                    instance.AuthenticationFactory.GetServiceClientCredentials(context, AzureEnvironment.Endpoint.ResourceManager),
+                    instance.ClientFactory.GetCustomHandlers());
+
+                client.SubscriptionId = context.Subscription.Id;


This can be removed once the call above is changed.

maddieclayton · 2018-06-06T00:47:57Z

...urceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceIdCompleter.cs

+                IEnumerable<string> resourceInfoList;
+                try
+                {
+                    resourceInfoList = client.Resources.List(odata)


This should be an async call so that we can have an automatic timeout (we don't want to have this pending for more than a few seconds, as it completely disables the powershell window) - see here: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceGroupCompleter.cs#L53

maddieclayton · 2018-06-06T01:13:51Z

...ceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceNameCompleter.cs

+                $"$resourceGroupParameterName = \"{resourceGroupParameterName ?? "$Null"}\"\n" +
+                $"$nodeServiceParameterName = \"{nodeServiceParameterName ?? "$Null"}\"\n" +
+                "$ids = [Microsoft.Azure.Commands.ResourceManager.Common.ArgumentCompleters.ResourceNameCompleterAttribute]::GetResources($resourceType)\n" +
+                "if ($resourceGroupParameterName -ne $Null -and $fakeBoundParameters.Contains($resourceGroupParameterName)) { $ids = $ids | Where-Object { $_.Group -eq $resourceGroupParameterName }}\n" +


If the resourcegroupname is not null, you should make the SDK call with the resourcegroupname set - this will probably be a lot faster than getting by type and then filtering by resourcegroup.

In this context, if a user has provided a resource group name:

Get-AzrureRmSomething -ResouceGroupName "grName" -SomethingName ...

It gets used as a filter for the next SomethingName argument.

maddieclayton · 2018-06-06T01:15:15Z

...ceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceNameCompleter.cs

+            base(CreateScriptBlock(
+                resourceType,
+                resourceGroupParameterName,
+                nodeServiceParameterName))


What happens if the resource is further than two levels deep (as in the case of SQL resources)?

Depth more than two levels is not supported now.
for example, for this resource id:

/subscriptions/{sub-id}/resourceGroups/{rg-name}/providers/Microsoft.Sql/servers/{server-name}/databases/{db-name}/key1/val1

it returns

{db-name}

I think if a request follows to support depth more than two levels - it can be quickly satisfied without breaking changes.

maddieclayton · 2018-06-06T03:34:03Z

...ceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceNameCompleter.cs

+            private static class Patterns
+            {
+                // example: /subscriptions/1c638cf4-608f-4ee6-b680-c329e824c3a8/resourceGroups/rgrssdfw69c0219330/providers/Microsoft.Sql/servers/sqlserver2c5254480/databases/master
+                private const string SubscripitonId = @"([0-9a-f]{8}[-][0-9a-f]{4}[-][0-9a-f]{4}[-][0-9a-f]{4}[-][0-9a-f]{12})";


subscription spelled wrong

maddieclayton · 2018-06-06T03:36:20Z

...ceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceNameCompleter.cs

+
+            public static ResourceInfo FromResourceId(string resourceId)
+            {
+                var match = Regex.Match(resourceId, Patterns.ResourceId, RegexOptions.IgnoreCase);


The numbers here seem a little magically to me, but this is fine as long as you've tested it on resources at multiple levels.

2 levels only

maddieclayton · 2018-06-06T03:37:01Z

src/ResourceManager/Profile/Commands.Profile.Test/ArgumentCompleterTests.cs

@@ -48,5 +48,19 @@ public void TestResourceGroupCompleter()
        {
            ProfileController.NewInstance.RunPsTest(xunitLogger, "72f988bf-86f1-41af-91ab-2d7cd011db47", "Test-ResourceGroupCompleter");
        }
+
+        [Fact]
+        [Trait(Category.AcceptanceType, Category.Flaky)]


Why did you add flaky?

copied from other completes tests.
fixed

maddieclayton · 2018-06-06T03:38:38Z

src/ResourceManager/Profile/Commands.Profile.Test/ArgumentCompleterTests.ps1

+    Assert-AreEqualArray $resourceNames $expectResourceNames
+    # change time to update the cache
+    [Microsoft.Azure.Commands.ResourceManager.Common.ArgumentCompleters.ResourceNameCompleterAttribute]::TimeToUpdate = [System.TimeSpan]::FromSeconds(1)
+    Start-Sleep -Seconds 1


Use wait-seconds: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Common/Commands.ScenarioTests.ResourceManager.Common/Common.ps1#L307. We don't allow sleeps in tests.

maddieclayton · 2018-06-06T03:39:46Z

src/ResourceManager/Profile/Commands.Profile.Test/ArgumentCompleterTests.ps1

+    Assert-AreEqualArray $resourceIds $expectResourceIds
+    # change time to update the cache
+    [Microsoft.Azure.Commands.ResourceManager.Common.ArgumentCompleters.ResourceIdCompleterAttribute]::TimeToUpdate = [System.TimeSpan]::FromSeconds(1)
+    Start-Sleep -Seconds 1


same comment here.

maddieclayton · 2018-06-06T03:41:53Z

@vladimir-shcherbakov Looks like your test is failing on travis - please check

maddieclayton · 2018-06-11T20:38:16Z

@vladimir-shcherbakov Can you remove ResourceNameCompleter?

vladimir-shcherbakov · 2018-06-11T22:15:12Z

ResourceNameCompleter removed

maddieclayton · 2018-06-12T20:05:19Z

...urceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceIdCompleter.cs

+
+                var client = AzureSession.Instance.ClientFactory.CreateArmClient<ResourceManagementClient>(context, AzureEnvironment.Endpoint.ResourceManager);
+
+                client.SubscriptionId = context.Subscription.Id;


Remove this, the "createarmclient" call does this automatically

maddieclayton · 2018-06-12T20:14:58Z

@vladimir-shcherbakov One small comment then LGTM. Updated your PR to include the changelog fix, so should pass this time.

…ure-powershell into iss#5415

praries880 · 2018-06-13T17:46:25Z

on demand : https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/939/

azuresdkci added the needs-review label May 8, 2018

vladimir-shcherbakov force-pushed the iss#5415 branch 3 times, most recently from cdbda1a to 770fbc0 Compare May 16, 2018 01:16

vladimir-shcherbakov changed the title ~~[Design] Tab completer for existing resources (Iss #5415)~~ Tab completer for existing resources (Iss #5415) May 23, 2018

maddieclayton assigned cormacpayne and maddieclayton Jun 2, 2018

maddieclayton suggested changes Jun 6, 2018

View reviewed changes

maddieclayton assigned vladimir-shcherbakov and unassigned cormacpayne and maddieclayton Jun 6, 2018

maddieclayton added needs-revision and removed needs-review labels Jun 6, 2018

vladimir-shcherbakov assigned maddieclayton and unassigned vladimir-shcherbakov Jun 8, 2018

vladimir-shcherbakov added needs-review and removed needs-revision labels Jun 8, 2018

maddieclayton added needs-revision and removed needs-review labels Jun 11, 2018

maddieclayton assigned vladimir-shcherbakov and unassigned maddieclayton Jun 11, 2018

vladimir-shcherbakov added 2 commits June 11, 2018 15:03

Resource Id completer added

916dd9f

Resource Id completer applied to Compute/VirtualMachine cmdlets.

f5f60eb

vladimir-shcherbakov force-pushed the iss#5415 branch from 9e171ed to f5f60eb Compare June 11, 2018 22:07

vladimir-shcherbakov and others added 3 commits June 11, 2018 16:00

change log updated.

2ae326c

Profile change log updated.

d8611ed

Merge branch 'preview' into iss#5415

fd1272f

maddieclayton suggested changes Jun 12, 2018

View reviewed changes

vladimir-shcherbakov added 2 commits June 12, 2018 14:16

PR comments fix.

433e78f

Merge branch 'iss#5415' of https://github.com/vladimir-shcherbakov/az…

9afa174

…ure-powershell into iss#5415

vladimir-shcherbakov removed their assignment Jun 12, 2018

vladimir-shcherbakov added needs-review and removed needs-revision labels Jun 12, 2018

vladimir-shcherbakov assigned maddieclayton Jun 12, 2018

maddieclayton previously approved these changes Jun 12, 2018

View reviewed changes

praries880 changed the base branch from preview to release-06-15-18 June 13, 2018 17:46

Merge branch 'release-06-15-18' into iss#5415

12df6d2

praries880 dismissed maddieclayton’s stale review via 12df6d2 June 13, 2018 17:48

praries880 added Waiting for On Demand Waiting for CI and removed needs-review labels Jun 13, 2018

praries880 approved these changes Jun 13, 2018

View reviewed changes

praries880 removed the Waiting for On Demand label Jun 13, 2018

praries880 merged commit 036d75f into Azure:release-06-15-18 Jun 13, 2018


		var client = AzureSession.Instance.ClientFactory.CreateArmClient<ResourceManagementClient>(context, AzureEnvironment.Endpoint.ResourceManager);

		client.SubscriptionId = context.Subscription.Id;

Tab completer for existing resources (Iss #5415) #6163

Tab completer for existing resources (Iss #5415) #6163

Uh oh!

Conversation

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

Uh oh!

Description

Checklist

Uh oh!

vladimir-shcherbakov commented May 10, 2018

Uh oh!

maddieclayton commented May 21, 2018

Uh oh!

vladimir-shcherbakov commented May 22, 2018

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!

vladimir-shcherbakov Jun 8, 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!

vladimir-shcherbakov Jun 8, 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!

Choose a reason for hiding this comment

Uh oh!

maddieclayton commented Jun 6, 2018

Uh oh!

maddieclayton commented Jun 11, 2018

Uh oh!

vladimir-shcherbakov commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maddieclayton commented Jun 12, 2018

Uh oh!

praries880 commented Jun 13, 2018

Uh oh!

Uh oh!

vladimir-shcherbakov commented May 8, 2018 •

edited

Loading

vladimir-shcherbakov Jun 8, 2018 •

edited

Loading

vladimir-shcherbakov Jun 8, 2018 •

edited

Loading