Powershell for ManagementGroups #5536

rajshah11 · 2018-02-14T20:18:38Z

Description

This PR adds Powershell preview for Management Groups
This PR is based off a new branch, since some erroneous commits were added to the previous PR #5421

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.

Bring preview branch up to date

Update preview branch after 5.0.0 release

…handling.

…ures.

…Subscription where SubscriptionId was accidentally SubscriptonId.

… for a RP.

… to auto register.

… into powershell-for-groupsrp

…nto powershell-for-groupsrp

cormacpayne

@rajshah11 some additional comments to take a look at. I need to sync up with @markcowl tomorrow about the auto-registration changes being made in here

cormacpayne · 2018-02-15T19:14:27Z

src/ResourceManager/ManagementGroups/AzureRM.ManagementGroups.psd1

+        # External dependent modules of this module
+        # ExternalModuleDependencies = ''
+
+		Prerelease = 'preview'


@rajshah11 nit: would you mind fixing the indentation for this line?

Done. Thanks

cormacpayne · 2018-02-15T22:51:00Z

...urceManager/ManagementGroups/Commands.ManagementGroups/Models/PSManagementGroupNoChildren.cs

+
+namespace Microsoft.Azure.Commands.ManagementGroups.Models
+{
+    public class PSManagementGroupNoChildren


@rajshah11 from the previous PR:

The issue is with output formatting. Children are shown only when -Expand is supplied. When it is not, the output has Children with null (even though the group may have children). This might give an impression to the user that the group has no children and to avoid this behavior I added a new object which does not have children at all.

I think a possible solution would be to get rid of this class and have all of the functionality in the PSManagementGroup class, which has a Children property that is null by default. We can make it so the formatting file checks if the value of Children is null or not, and if it isn't, we will output the contents of the property, and hide it otherwise. Then, we can make it so the Children property gets its value from a lazy call to the SDK, meaning that whenever a user wants to access the Children for a given management group, accessing that property will make the underlying SDK call to get the children, which means users would never see a null or empty value returned unless the management group did in fact have zero children.

Let me know what you think of this idea. It will make it so we have a consistent type that is being returned and passed around, as well as mitigate and confusion the user may have about the children of a management group.

@cormacpayne We can have a single class by changing the formatting file. However, rather than making the Lazy calls, we would prefer adding a note when Get is called (without -Expand or -Recurse) saying that the user needs to pass -Expand or -Recurse to access the children. This is because of three reasons -

We need to release preview as soon as possible to our public preview customers.

By API Design, Get is not meant to show children without -Expand and -Recurse

I think we have already made all the changes needed for the preview as suggested in the review meeting. This change is something that can be aded in the future. Also, our new Powershell design is already in progress, which will provide the users which such flexibilty eventually.

I will be making the other changes. Please confirm if I can proceed with having one class by changing the formatting file and adding a note.

cormacpayne · 2018-02-15T23:14:03Z

...ourceManager/ManagementGroups/Commands.ManagementGroups/Cmdlets/NewAzureRmManagementGroup.cs

+        [Parameter(ParameterSetName = Constants.ParameterSetNames.GroupOperationsParameterSet, Mandatory = false,
+            HelpMessage = Constants.HelpMessages.ParentId)]
+        [ValidateNotNullOrEmpty]
+        public string ParentId { get; set; } = null;


@rajshah11 do you think it would make sense to have another parameter set that allows the user to pass in the PSManagementGroup object of the parent that they want for this new management group?

Yes. I think it would make sense. Will make the change.

cormacpayne · 2018-02-15T23:19:16Z

...ceManager/ManagementGroups/Commands.ManagementGroups/Cmdlets/RemoveAzureRmManagementGroup.cs

+        [Parameter(ParameterSetName = Constants.ParameterSetNames.ManagementGroupParameterSet, Mandatory = false, 
+            HelpMessage = Constants.HelpMessages.InputObject, ValueFromPipeline = true)]
+        [ValidateNotNullOrEmpty]
+        public PSManagementGroup ManagementGroup { get; set; }


@rajshah11 this should be renamed to InputObject

Done. Thanks.

cormacpayne · 2018-02-15T23:19:35Z

...ceManager/ManagementGroups/Commands.ManagementGroups/Cmdlets/RemoveAzureRmManagementGroup.cs

+    public class RemoveAzureRmManagementGroup : AzureManagementGroupsCmdletBase
+    {
+
+        [Parameter(ParameterSetName = Constants.ParameterSetNames.ManagementGroupParameterSet, Mandatory = false, 


@rajshah11 nit: this should be mandatory

cormacpayne · 2018-02-15T23:23:23Z

...ceManager/ManagementGroups/Commands.ManagementGroups/Cmdlets/RemoveAzureRmManagementGroup.cs

+
+        public override void ExecuteCmdlet()
+        {
+


@rajshah11 nit: can you remove these extra new lines?

Done. Thanks

cormacpayne · 2018-02-15T23:25:03Z

...ceManager/ManagementGroups/Commands.ManagementGroups/Cmdlets/UpdateAzureRmManagementGroup.cs

+
+namespace Microsoft.Azure.Commands.ManagementGroups.Cmdlets
+{
+    [Cmdlet("Update", "AzureRmManagementGroup", DefaultParameterSetName = Constants.ParameterSetNames.GroupOperationsParameterSet, SupportsShouldProcess = true), OutputType(typeof(PSManagementGroupNoChildren))]


@rajshah11 nit: change "Update" to VerbsCommon.Update

Verbs.Common does not have "Update" for me

cormacpayne · 2018-02-15T23:25:19Z

...ceManager/ManagementGroups/Commands.ManagementGroups/Cmdlets/UpdateAzureRmManagementGroup.cs

+        [Parameter(ParameterSetName = Constants.ParameterSetNames.ManagementGroupParameterSet, Mandatory = false,
+            HelpMessage = Constants.HelpMessages.InputObject, ValueFromPipeline = true)]
+        [ValidateNotNullOrEmpty]
+        public PSManagementGroup ManagementGroup { get; set; }


@rajshah11 same comment about renaming this to InputObject

Done. Thanks.

cormacpayne · 2018-02-15T23:25:29Z

...ceManager/ManagementGroups/Commands.ManagementGroups/Cmdlets/UpdateAzureRmManagementGroup.cs

+    [Cmdlet("Update", "AzureRmManagementGroup", DefaultParameterSetName = Constants.ParameterSetNames.GroupOperationsParameterSet, SupportsShouldProcess = true), OutputType(typeof(PSManagementGroupNoChildren))]
+    public class UpdateAzureRmManagementGroup : AzureManagementGroupsCmdletBase
+    {
+        [Parameter(ParameterSetName = Constants.ParameterSetNames.ManagementGroupParameterSet, Mandatory = false,


@rajshah11 nit: this should be mandatory

cormacpayne · 2018-02-15T23:46:57Z

...ceManager/ManagementGroups/Commands.ManagementGroups/Cmdlets/UpdateAzureRmManagementGroup.cs

+        [Parameter(ParameterSetName = Constants.ParameterSetNames.GroupOperationsParameterSet, Mandatory = false,
+            HelpMessage = Constants.HelpMessages.ParentId)]
+        [ValidateNotNullOrEmpty]
+        public string ParentId { get; set; } = null;


@rajshah11 same question about whether or not it makes sense to allow the user to pass the PSManagementGroup object they want to be the parent

Will make the change.

rajshah11 · 2018-02-16T19:48:25Z

@cormacpayne All changes are done and updated the help file. This build should succeed. Please confirm if this is good to go once the build succeeds.

cormacpayne · 2018-02-16T21:34:10Z

Sign job: https://azuresdkci.westus2.cloudapp.azure.com/job/ps-sign/138/

markcowl · 2018-02-17T03:07:29Z

...roups/Commands.ManagementGroups/Common/AzurePrecheckRegistrationAndAutoRegisterCmdletBase.cs

@rajshah11 Adding a handler to the client factory impacts all clinets created from the ClientFactory. Please follow the pattern in Reservations cmdlets for registration, in which this registration is simply performed in BeginProcessing, without using a delegating handler.

markcowl · 2018-02-17T03:08:00Z

setup/azurecmdfiles.wxi

Please revert any changes to this file

markcowl · 2018-02-17T03:09:29Z

src/ResourceManager/ManagementGroups/AzureRM.ManagementGroups.psd1

This should be updated to 4.3.0

rajshah11 · 2018-02-19T02:29:07Z

@markcowl @cormacpayne Made the suggested changes. Thanks!

cormacpayne · 2018-02-20T19:39:36Z

Sign job: https://azuresdkci.westus2.cloudapp.azure.com/job/ps-sign/150/

markcowl · 2018-02-21T00:17:47Z

...Manager/ManagementGroups/Commands.ManagementGroups/Common/AzureManagementGroupsCmdletBase.cs

+            }
+        }
+
+        public void PreregisterSubscription()


Why is this code repeated?

All comments addressed

markcowl · 2018-02-21T00:23:31Z

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

markcowl and others added 30 commits November 2, 2017 17:28

Merge pull request #370 from markcowl/uptodate

3491fd3

Bring preview branch up to date

Merge pull request #372 from cormacpayne/preview-5.0.0

72c05ab

Update preview branch after 5.0.0 release

Powershell cmdlets for groupsrp

f2bb59b

Updated cmdlets

2f2cd19

Added comments

663f57f

Removing missing file from Commands.ManagementGroups.csproj.

0b41002

Updating Cmdlets to work with the latest SDK changes.

65e53b4

Management Groups:Updating cmdlets to have better formatting + error …

918ba9c

…handling.

Management Group: Forgot to separate out PSParentGroupInfo.cs

bd83303

Management Group: Changing GroupId to be GroupName.

08dfd2e

Management Group: Forgot to include this one in bd83303.

ffa8892

Management Group: Adding better null checking for various data struct…

5d9a0eb

…ures.

Adding help files.

5ac42e0

Management Groups: Fixing typo for (Add|Remove)-AzureRmMangementGroup…

3605bfd

…Subscription where SubscriptionId was accidentally SubscriptonId.

Adding support for cmdlets to precheck registration and auto register…

87933a9

… for a RP.

Modifying the (Add|Remove)-AzureRmManagementGroupSubscription cmdlets…

b4b7312

… to auto register.

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

6b4722a

… into powershell-for-groupsrp

Updated cmdlets

cfffa98

Adding Tests and minor changes to cmdlets

c831a32

Adding license headers

398ce3e

Made changes suggested in the PR

abf4409

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

5b9b60c

…nto powershell-for-groupsrp

Pull in changes, add nuget package to local feed

32cf978

Fix for the failing Jenkins build

d8269d7

Add description to EventHub md to fix buid

326050d

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

10259e1

…nto powershell-for-groupsrp

Updated installer files

814ccae

Added autoregistration of other subs to 2 cmdlets

ada44cb

Changes after moving managementgroups out of the resource sdk

b1528a2

sdk update, new test, comments on the PR

aef5cba

azuresdkci added the needs-review label Feb 14, 2018

azuresdkci assigned cormacpayne Feb 14, 2018

MiYanni unassigned cormacpayne Feb 14, 2018

cormacpayne added the Preview label Feb 15, 2018

cormacpayne previously requested changes Feb 15, 2018

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Feb 15, 2018

cormacpayne assigned rajshah11 Feb 15, 2018

Raj Shah added 2 commits February 15, 2018 20:48

Changes suggested in the PR

0561633

Update help files

48dc2ff

markcowl requested changes Feb 17, 2018

View reviewed changes

Raj Shah and others added 4 commits February 16, 2018 19:14

Minor fixes

b46ab93

Merge branch 'mgmt-group-preview' into powershell-for-managementgroups

c6514b7

Autoreg, update profile and help

748dcc1

Revert changes to installer file

2107e00

MiYanni assigned cormacpayne and unassigned rajshah11 Feb 20, 2018

MiYanni added needs-review and removed needs-revision labels Feb 20, 2018

markcowl approved these changes Feb 21, 2018

View reviewed changes

markcowl added Waiting for On Demand and removed needs-review labels Feb 21, 2018

cormacpayne merged commit e00ef61 into Azure:mgmt-group-preview Feb 21, 2018

rajshah11 mentioned this pull request May 22, 2018

Add ManagementGroups to AzureRM.Resources and Update to use latest SDK for GA #6271

Merged

8 tasks

Powershell for ManagementGroups #5536

Powershell for ManagementGroups #5536

Uh oh!

Conversation

rajshah11 commented Feb 14, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

This PR adds Powershell preview for Management Groups This PR is based off a new branch, since some erroneous commits were added to the previous PR #5421

Uh oh!

cormacpayne left a comment

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!

rajshah11 Feb 16, 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!

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!

rajshah11 commented Feb 16, 2018

Uh oh!

cormacpayne commented Feb 16, 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!

rajshah11 commented Feb 19, 2018

Uh oh!

cormacpayne commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markcowl commented Feb 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rajshah11 commented Feb 14, 2018 •

edited by azuresdkci

Loading

This PR adds Powershell preview for Management Groups
This PR is based off a new branch, since some erroneous commits were added to the previous PR #5421

rajshah11 Feb 16, 2018 •

edited

Loading