Fix Argument Completers #4945

maddieclayton · 2017-11-08T22:32:05Z

Description

#4910
Add script to psm1 file that will register Location/ResourceGroup argument completers.

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.

markcowl · 2017-11-09T19:31:16Z

tools/AzureRM.Example.psm1

+    $ResourceGroupCommands = %RGCCOMMANDS%
+
+    $CommonAssembly = [Reflection.Assembly]::LoadFrom('.\Microsoft.Azure.Commands.ResourceManager.Common.dll')


Why do we need to define $CommonAssembly? This assembly should be loaded in the ImportedDependencies above.

markcowl · 2017-11-09T19:34:53Z

tools/UpdateModules.ps1

+     $resourceGroupCompleterCommands = Find-CompleterAttribute -CompleterName "ResourceGroupCompleterAttribute" -ModuleMetadata $ModuleMetadata -ModulePath $ModulePath -isRMModule $isRMModule
+     $template = $template -replace "%RGCCOMMANDS%", $resourceGroupCompleterCommands
+
+     $locationCompleterCommands = Find-CompleterAttribute -CompleterName "LocationCompleterAttribute" -ModuleMetadata $ModuleMetadata -ModulePath $ModulePath -isRMModule $isRMModule


what is the result if the list is empty?

if the list is empty, an empty list will be inserted into the template and no completers will be registered in the foreach loop,

markcowl · 2017-11-09T19:39:09Z

tools/UpdateModules.ps1

@@ -122,22 +191,22 @@ $templateLocation = "$PSScriptRoot\AzureRM.Example.psm1"
 if (($scope -eq 'All') -or $publishToLocal ) {
    # If we publish 'All' or to local folder, publish AzureRM.Profile first, becasue it is the common dependency
    Write-Host "Updating profile module"
-    Create-ModulePsm1 -ModulePath "$resourceManagerRootFolder\AzureRM.Profile" -TemplatePath $templateLocation
+    Create-ModulePsm1 -ModulePath "$resourceManagerRootFolder\AzureRM.Profile" -TemplatePath $templateLocation $true


this last parameter should be a named parameter, so there is some indication of what this is doing.

markcowl · 2017-11-09T20:12:38Z

tools/UpdateModules.ps1

+                    if ($completerAttribute -ne $null) {
+                        $constructedCommands += "@('" + $currentCmdlet.GetCustomAttributes("System.Management.Automation.CmdletAttribute").VerbName + "-" + $currentCmdlet.GetCustomAttributes("System.Management.Automation.CmdletAttribute").NounName + "', "
+                        $constructedCommands += "'" + $_.Name + "', "
+                        if ($completerAttribute.ConstructorArguments.Count -eq 0) 


this would be simplified if you store the cmdletattribute in a varaible:

$AtrributeTypeName="System.Management.Automation.CmdletAttribute" $cmdletAttribute = $currentCmdlet.GetCustomAttributes($attributeTypeName)

It also seems like the result should be a list of hashtables @{CmdletName="<name>"; CmdletType="<typeName>"} this will make any code that uses this list a bit easier to read.

Finally, I think we should add some methods to the attribute itself to generate the list of constructor argument values, i.e. $completerAttribute.GetConstructorArgumentList() .

The problem with the hashtable is that we actual need three values: the cmdlet name, the parameter name, and the attribute arguments (for the location completer). I could make a hashtable of hashtables, but I think that the list of lists might be easier to read in this case. If you would like me to change it let me know.

I wrote the code adding a method for generating the list of constructor arguments, but it actually ended up being more convoluted because I had to create a new LocationCompleterAttribute object and convert the list of strings from c# to powershell. I'll leave this how it is for now, but let me know if you would rather creating a new method.

markcowl · 2017-11-09T20:13:16Z

tools/UpdateModules.ps1

@@ -26,7 +26,8 @@ function Create-ModulePsm1
  [CmdletBinding()]
  param(
    [string]$ModulePath,
-    [string]$TemplatePath
+    [string]$TemplatePath,
+    [bool]$isRMModule


Follow the conventiosn with capital i

Looks like this is still lower case 'i'

Yikes. Fixed.

markcowl · 2017-11-15T00:35:14Z

...ResourceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/PSCompleterBase.cs

+        {
+        }
+
+        public virtual string[] GetCompleterValues()


You can make this abstract, so that no impementation is necessary (If the class is abstract, it also cannot be created).

markcowl · 2017-11-15T00:36:13Z

tools/UpdateModules.ps1

@@ -26,7 +26,8 @@ function Create-ModulePsm1
  [CmdletBinding()]
  param(
    [string]$ModulePath,
-    [string]$TemplatePath
+    [string]$TemplatePath,
+    [bool]$isRMModule


Looks like this is still lower case 'i'

maddieclayton · 2017-11-15T18:30:21Z

@azuresdkci test this please

maddieclayton · 2017-11-16T21:29:08Z

sign job: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-sign/9/

maddieclayton · 2017-11-21T02:28:19Z

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

Successful sign job here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-sign/16/

markcowl · 2017-11-21T19:08:23Z

@maddieclayton I've added the posh-pack and release builds below as well - as long as the packaging job produces usable debug build, and the release job passes (does not need to produce a working build), then we are good to merge this. Excellent work!

packaging run: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/posh-pack/23/

markcowl · 2017-11-21T19:10:42Z

posh-release run here: https://azuresdkci.westus2.cloudapp.azure.com/job/posh-release/2/

Fix Completers

643986f

maddieclayton assigned markcowl Nov 8, 2017

maddieclayton added the needs-review label Nov 8, 2017

maddieclayton added 2 commits November 8, 2017 14:52

Merge branch 'preview' into FixCompleters

2699d96

Merge branch 'preview' into FixCompleters

6290f46

markcowl requested changes Nov 9, 2017

View reviewed changes

markcowl added needs-revision and removed needs-review labels Nov 9, 2017

markcowl assigned maddieclayton and unassigned markcowl Nov 9, 2017

azdevxps added 2 commits November 10, 2017 15:51

answer comments and fix caching bug

99fed5e

change debug to release

1f06f71

maddieclayton assigned markcowl and unassigned maddieclayton Nov 10, 2017

maddieclayton added needs-review needs-revision and removed needs-revision needs-review labels Nov 10, 2017

Create wrapper class

aa84709

maddieclayton added needs-review and removed needs-revision labels Nov 14, 2017

markcowl requested changes Nov 15, 2017

View reviewed changes

markcowl added needs-revision and removed needs-review labels Nov 15, 2017

fix comments

f23e556

maddieclayton added Waiting for On Demand and removed needs-revision labels Nov 15, 2017

azdevxps added 2 commits November 15, 2017 09:42

merge upstream

0479c89

fix merge issues

17c3cf7

Maddie Clayton added 7 commits November 17, 2017 13:03

disable and enable strong name verification

1bade39

suppress output and run as admin

387d066

Elevate powershell prompt

d771cab

Fix build issue

74d6180

fix build

8115054

fix build parameters

1f9f61e

fix build

553b715

maddieclayton unassigned markcowl Nov 18, 2017

Maddie Clayton and others added 7 commits November 18, 2017 10:38

replace double quotes with single quotes

35ff51d

wait for process to end

7f40f6e

restart msiserver

a88534a

write output to file

54de3ee

try different disable method

65b9b7d

end exec

d5b1a10

write psm1 files after signing completed

875a08e

maddieclayton assigned markcowl and unassigned maddieclayton Nov 21, 2017

maddieclayton added needs-review and removed Do Not Merge 🚫 labels Nov 21, 2017

markcowl approved these changes Nov 21, 2017

View reviewed changes

markcowl removed the needs-review label Nov 21, 2017

markcowl removed their assignment Nov 21, 2017

markcowl added the Do Not Merge 🚫 label Nov 21, 2017

maddieclayton merged commit 082c27e into Azure:preview Nov 21, 2017

maddieclayton deleted the FixCompleters branch November 21, 2017 21:07

maddieclayton removed the Do Not Merge 🚫 label Nov 21, 2017

		$ResourceGroupCommands = %RGCCOMMANDS%

		$CommonAssembly = [Reflection.Assembly]::LoadFrom('.\Microsoft.Azure.Commands.ResourceManager.Common.dll')

Fix Argument Completers #4945

Fix Argument Completers #4945

Uh oh!

Conversation

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

Uh oh!

Description

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!

maddieclayton commented Nov 15, 2017

Uh oh!

maddieclayton commented Nov 16, 2017

Uh oh!

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

Uh oh!

Uh oh!

markcowl commented Nov 21, 2017

Uh oh!

markcowl commented Nov 21, 2017

Uh oh!

Uh oh!

maddieclayton commented Nov 8, 2017 •

edited

Loading

maddieclayton commented Nov 21, 2017 •

edited

Loading