Add ResourceGroup completer #4721

maddieclayton · 2017-10-05T17:09:07Z

Description

Creates a ResourceGroupCompleter. Shows the first 50 resource groups that match the string the user has types. Default resource group will be put first in the list if it has been set. #4665

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.

maddieclayton · 2017-10-05T17:47:48Z

@azuresdkci Test this please

…/azure-powershell into ResourceCompleter1

markcowl · 2017-10-06T01:05:51Z

src/ResourceManager/Common/Commands.ResourceManager.Common/ResourceGroupCompleter.cs

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


Use the simpler CreateArmClient method here: https://github.com/Azure/azure-powershell/blob/preview/src/Common/Commands.Common.Authentication.Abstractions/Interfaces/IClientFactory.cs#L39

Staying the same as discussed offline

markcowl · 2017-10-06T01:06:10Z

src/ResourceManager/Common/Commands.ResourceManager.Common/ResourceGroupCompleter.cs

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


If you use this method, you will not need to set the subscription id

markcowl · 2017-10-06T01:07:16Z

src/ResourceManager/Common/Commands.ResourceManager.Common/ResourceGroupCompleter.cs

+                        {
+                            if (resourceGroups.Result != null)
+                            {
+                                var resourceGroupList = resourceGroups.Result.ToList();


Why write it to a list instead of just using it in the ForEach?

Also, here we are making a conscious decision to only use the first page of results each time. We should probably call this out with a comment. BTW - ho9w many resource groups are in the first page of results?

I couldn't figure out how many are in the first page - it wasn't in any of the code or documentation about IPage or List and it's more than my biggest subscription (92). Not sure if there's another place I should look.

markcowl · 2017-10-06T01:07:45Z

src/ResourceManager/Common/Commands.ResourceManager.Common/ResourceGroupCompleter.cs

+
+                            else
+                            {
+#if DEBUG


Seems like this if should surround the entire else clause

markcowl · 2017-10-06T01:07:57Z

src/ResourceManager/Common/Commands.ResourceManager.Common/ResourceGroupCompleter.cs

+
+                        else
+                        {
+#if DEBUG


same comment

markcowl · 2017-10-06T01:09:15Z

src/ResourceManager/Common/Commands.ResourceManager.Common/ResourceGroupCompleter.cs

+        {
+            if (defaultResourceGroup != null)
+            {
+                if (resourceGroupNames.Contains(defaultResourceGroup))


What if the default resource group is on page 2 of the results? Seems like we should always include the defautl resource group at the beginning

If the user types out part of a name not including the default resource group, I think it makes sense not to include the default resource group. Let me know if I should include it.

Agreed, but it still could be in the second page of results

Still waiting to hear back from the Resources team about if it is possible to query Resource Groups by name

markcowl · 2017-10-06T01:14:06Z

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

+        public void ReturnsEmptyListWhenNoResourceGroupsExist()
+        {
+            IList<string> resourceGroupsReturned = new List<string>();
+


remove extra lines between statements.

markcowl · 2017-10-06T01:14:31Z

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

+        {
+            IList<string> resourceGroupsReturned = new List<string>();
+            resourceGroupsReturned.Add("test1");
+


remove extra lines

(apply everywhere in this file)

markcowl · 2017-10-06T01:18:23Z

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

+            resourceGroupsReturned.Add("test3");
+            resourceGroupsReturned.Add("test4");
+
+            Assert.Equal(ResourceGroupCompleterAttribute.GetResourceGroups(resourceGroupsReturned, null), new string[] { "test1", "test2", "test3", "test4" });


consider using Assert.Collection for succinctly testing the properties of a known collection and its elements, when in a known order. Note this this is advisroy only (and prompting you to check out the assertiosn available int eh xunit library)

markcowl · 2017-10-18T01:36:46Z

src/ResourceManager/Common/Commands.ResourceManager.Common/ResourceGroupCompleter.cs

+        {
+            if (defaultResourceGroup != null)
+            {
+                if (resourceGroupNames.Contains(defaultResourceGroup))


Agreed, but it still could be in the second page of results

markcowl · 2017-10-20T23:22:13Z

...esourceManager/Common/Commands.ResourceManager.Common/TabCompleter/ResourceGroupCompleter.cs

+        {
+            IAzureContext context = AzureRmProfileProvider.Instance.Profile.DefaultContext;
+            var resourceGroupNamesCopy = ResourceGroupNames;
+            if (context.ExtendedProperties.ContainsKey(Resources.DefaultResourceGroupKey))


Use context.IsPropertySet

markcowl · 2017-10-20T23:25:17Z

...esourceManager/Common/Commands.ResourceManager.Common/TabCompleter/ResourceGroupCompleter.cs

+                lock (_lock)
+                {
+                    _resourceGroupNames = new List<string>();
+                    IAzureContext context = AzureRmProfileProvider.Instance.Profile.DefaultContext;


How long is this taking? I wonder if we will need to do some caching here. Let's discuss and decide how to proceed.

…nto ResourceCompleter1

cormacpayne · 2017-10-27T15:59:20Z

@maddieclayton there are merge conflicts after the namespace update

maddieclayton · 2017-10-27T16:46:24Z

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

maddieclayton · 2017-10-30T19:10:43Z

@markcowl Are there more changes that this PR needs?

markcowl

A couple of comments. Overall, this looks good.

markcowl · 2017-10-30T20:30:22Z

...eManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceGroupCompleter.cs

+                        }
+                    }
+
+                    return _resourceGroupNamesDictionary[contextHash];


so, if we haven't previously cached a value and the call actually times out in production, this will fall through to here, which will null ref, I think. We probably need to store the result in a local variable and return that, with the default value beign returned if the call to list resource groups times out.

markcowl · 2017-10-30T20:31:19Z

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

+{
+    public class ResourceGroupCompleterUnitTests
+    {
+        [Fact]


To test completely, we should think about throwing an exception from the client, verifying the behavior.

maddieclayton · 2017-10-31T01:45:14Z

@azuresdkci Test this please

markcowl · 2017-10-31T23:32:48Z

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

azdevxps and others added 2 commits October 5, 2017 10:06

add ResourceGroup completer and tests

6b6c45f

Merge branch 'preview' into ResourceCompleter1

cdebd56

azuresdkci added the needs-review label Oct 5, 2017

azuresdkci assigned markcowl Oct 5, 2017

azdevxps added 2 commits October 5, 2017 10:57

pull from master

3d37dfd

Merge branch 'ResourceCompleter1' of https://github.com/maddieclayton…

43e3580

…/azure-powershell into ResourceCompleter1

markcowl requested changes Oct 6, 2017

View reviewed changes

markcowl added needs-revision Do Not Merge 🚫 and removed needs-review labels Oct 6, 2017

Make fixes based on review

b1a72c5

maddieclayton assigned maddieclayton and unassigned markcowl Oct 13, 2017

markcowl requested changes Oct 18, 2017

View reviewed changes

maddieclayton removed the Do Not Merge 🚫 label Oct 19, 2017

Make sure default first in list

37a20dd

maddieclayton added needs-review and removed needs-revision labels Oct 19, 2017

maddieclayton assigned markcowl and unassigned maddieclayton Oct 19, 2017

remove extra filter

561407b

markcowl requested changes Oct 23, 2017

View reviewed changes

markcowl added needs-revision and removed needs-review labels Oct 23, 2017

markcowl assigned maddieclayton and unassigned markcowl Oct 23, 2017

azdevxps added 2 commits October 25, 2017 17:00

Merge branch 'preview' of https://github.com/Azure/azure-powershell i…

86d26f4

…nto ResourceCompleter1

fix PR comments

06f7963

maddieclayton added the needs-review label Oct 26, 2017

maddieclayton assigned markcowl Oct 26, 2017

maddieclayton and others added 3 commits October 27, 2017 09:24

Merge branch 'preview' into ResourceCompleter1

6dd3c1a

Merge branch 'preview' into ResourceCompleter1

ab86e95

merge changes

eda2e76

markcowl added needs-revision and removed needs-review labels Oct 30, 2017

markcowl assigned maddieclayton and unassigned markcowl Oct 30, 2017

markcowl assigned maddieclayton and unassigned maddieclayton Oct 30, 2017

markcowl removed the needs-revision label Oct 30, 2017

markcowl requested changes Oct 30, 2017

View reviewed changes

markcowl added the needs-revision label Oct 30, 2017

fix comments

ad7c5c4

maddieclayton added needs-review and removed needs-revision labels Oct 30, 2017

maddieclayton assigned markcowl and unassigned maddieclayton Oct 30, 2017

remove unnecessary usings

9e95f80

markcowl approved these changes Oct 31, 2017

View reviewed changes

markcowl added Waiting for On Demand and removed needs-review labels Oct 31, 2017

markcowl removed their assignment Oct 31, 2017

maddieclayton merged commit 93dc1de into Azure:preview Nov 1, 2017

maddieclayton deleted the ResourceCompleter1 branch November 1, 2017 00:52

Add ResourceGroup completer #4721

Add ResourceGroup completer #4721

Uh oh!

Conversation

maddieclayton commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

maddieclayton commented Oct 5, 2017

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 Oct 6, 2017 • 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!

cormacpayne commented Oct 27, 2017

Uh oh!

maddieclayton commented Oct 27, 2017

Uh oh!

maddieclayton commented Oct 30, 2017

Uh oh!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maddieclayton Oct 30, 2017 • 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!

maddieclayton commented Oct 31, 2017

Uh oh!

markcowl commented Oct 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

maddieclayton commented Oct 5, 2017 •

edited

Loading

markcowl Oct 6, 2017 •

edited

Loading

maddieclayton Oct 30, 2017 •

edited

Loading