Add ResourceGroup to DefaultParameterValues #4911

maddieclayton · 2017-11-01T23:59:06Z

Description

Add default resource group to every cmdlet that has ResourceGroupName mandatory for every parameter #3808. Based off of changes in #4734.

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-11-02T06:43:56Z

@azuresdkci test this please

markcowl · 2017-11-03T00:06:02Z

tools/UpdateModules.ps1

+     if ($AddDefaultParameters) 
+     {
+        $nestedModules = $ModuleMetadata.NestedModules
+        $AllCmdlets = @()


It feels like we should factor this out into its own function

markcowl · 2017-11-03T00:24:08Z

tools/UpdateModules.ps1

+        }
+
+        $FilteredCommands = @()
+        $AllCmdlets | ForEach-Object {


This also feels like it could be in it's own function that returned a predicate, so that this function would basically be

$nestedModules | Get-CmdletType | Where-Object {Test-RequiredParameter -Cmdlet $_ -ParameterName "ResourceGroup"}

markcowl · 2017-11-06T16:35:19Z

tools/UpdateModules.ps1

+    {
+        $rgParameter = $Cmdlet.GetProperties() | Where-Object {$_.Name -eq "ResourceGroupName"}
+        if ($rgParameter -ne $null) {
+            $parameterSets = $rgParameter.CustomAttributes | Where-Object {$_.AttributeType.Name -eq "ParameterAttribute"}


I think this variable name is a bit confusing - I thought this was an enumeration of parameter sets, but it is really just anenumeration of parameter attributes. Renaming this to '$parameters' or '$parameterAttributes' would make the code a bit easier to read, I think.

markcowl · 2017-11-06T16:36:05Z

tools/UpdateModules.ps1

+        return "`$module = Get-Module $ModuleName `
+if (`$module -ne `$null -and `$module.Version.ToString().CompareTo(`"$MinimumVersion`") -lt 0) `
+{ `
+    Write-Warning `"A later version of $ModuleName was found to be imported already. Please see <link> for more details.`" `


I think thsi is a bit out oif date with the latest in Cormac's PR. Please pull the latest

markcowl · 2017-11-06T16:37:37Z

tools/UpdateModules.ps1

+        }
+
+        else {
+        return "@()"


Are we return a string representing the code for an array?

Yes, when put into the template it will be interpreted as an array.

markcowl · 2017-11-06T16:39:21Z

tools/UpdateModules.ps1

+    }
+}
+
+function Test-RequiredParameter


Perhaps this should be Test-CmdletRequiredParameter and the parameter name should likely be a parameter.

markcowl · 2017-11-06T16:40:58Z

tools/AzureRM.Example.psm1

+$FilteredCommands | ForEach-Object {
+	$global:PSDefaultParameterValues.Add($_,
+		{
+			$context = Get-AzureRmContext


What happens if there is no context (the user hasn't logged in yet)? Wnat to make sure that we are not pushing errors into the Error stream.

Added a null check

markcowl · 2017-11-15T00:29:11Z

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

maddieclayton · 2017-11-15T01:01:14Z

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

azdevxps added 7 commits October 24, 2017 17:27

initial changes

31d526b

put script in startup file

07c1e06

remove old changes

ccb1a30

Move script to updatemodule

29371d5

move script to build folder

d41cfb1

revert changes

5a1bdf5

base changes on Cormac's PR

647691d

maddieclayton assigned markcowl Nov 1, 2017

maddieclayton added the needs-review label Nov 1, 2017

markcowl requested changes Nov 3, 2017

View reviewed changes

markcowl added needs-revision Merge Conflicts and removed needs-review labels Nov 3, 2017

Merge upstream

c7a69ec

maddieclayton removed the Merge Conflicts label Nov 3, 2017

Address review comments

c3eb2d0

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

markcowl requested changes Nov 6, 2017

View reviewed changes

markcowl requested a review from cormacpayne November 6, 2017 16:41

markcowl assigned maddieclayton and cormacpayne and unassigned markcowl Nov 6, 2017

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

maddieclayton changed the base branch from preview to release-5.0.0 November 6, 2017 23:38

azdevxps added 2 commits November 6, 2017 15:40

Resolve conflicts

d6daaa6

address PR comments

0fa53f9

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

maddieclayton assigned markcowl and unassigned maddieclayton Nov 6, 2017

maddieclayton changed the base branch from release-5.0.0 to preview November 8, 2017 22:59

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

markcowl approved these changes Nov 15, 2017

View reviewed changes

markcowl removed the needs-review label Nov 15, 2017

markcowl unassigned markcowl and cormacpayne Nov 15, 2017

markcowl added the Waiting for On Demand label Nov 15, 2017

maddieclayton merged commit 9b78152 into Azure:preview Nov 15, 2017

maddieclayton deleted the DefaultScript branch November 18, 2017 00:49

Add ResourceGroup to DefaultParameterValues #4911

Add ResourceGroup to DefaultParameterValues #4911

Uh oh!

Conversation

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

Uh oh!

Description

Uh oh!

maddieclayton commented Nov 2, 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!

markcowl commented Nov 15, 2017

Uh oh!

maddieclayton commented Nov 15, 2017

Uh oh!

Uh oh!

maddieclayton commented Nov 1, 2017 •

edited

Loading