Skip to content

Conversation

@rajshah11
Copy link
Contributor

@rajshah11 rajshah11 commented Feb 1, 2018

Description

This PR includes cmdlets and tests for Management Groups.

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.

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.

Copy link
Member

@cormacpayne cormacpayne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 a few initial comments to take a look at

@@ -2,4224 +2,256 @@
<Include xmlns="http://schemas.microsoft.com/wix/2006/wi">
<Fragment>
<DirectoryRef Id="PowerShellFolder">
<Component Id="cmp146D4BC76FE0B2425722E32B160714B3" Guid="*">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 it looks like the wix file was incorrectly generated -- you will need to do a full build and then generate this file. Information about generating the wxi file can be found here.

# ProcessorArchitecture = ''

# Modules that must be imported into the global environment prior to importing this module
RequiredModules = @(@{ModuleName = 'AzureRM.Profile'; ModuleVersion = '4.0.0'; })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 please make sure to pull the latest changes from the preview branch. The latest version of AzureRM.Profile that your module should be referencing is 4.2.0

# IconUri = ''

# ReleaseNotes of this module
ReleaseNotes = '* Add support for online help
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 you can leave the ReleaseNotes field blank for now -- we will update it before this is released

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 ping on this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!

// You can specify all the values or you can default the Build and Revision Numbers
// by using the '*' as shown below:
// [assembly: AssemblyVersion("1.0.*")]
[assembly: AssemblyVersion("1.0.0.0")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 for consistency, AssemblyVersion and AssemblyFileVersion should match the version of your module (0.0.1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!

/// <summary>
/// Get-AzureRmManagementGroup Cmdlet
/// </summary>
[Cmdlet(VerbsCommon.Get,"AzureRmManagementGroup", DefaultParameterSetName = Constants.ParameterSetNames.GroupOperationsParameterSet, SupportsShouldProcess = true, ConfirmImpact = ConfirmImpact.Medium), OutputType(typeof(string))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 please remove the ConfirmImpact property

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!

<WarningLevel>4</WarningLevel>
</PropertyGroup>
<ItemGroup>
<Reference Include="Hyak.Common, Version=1.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 please import the Common.Dependencies.targets file, like so. This will allow you to remove almost all of the packages you are referencing in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!!

<Link>AzureRM.ManagementGroups.psd1</Link>
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
<None Include="app.config" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 please remove this reference to app.config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
<Import Project="..\packages\Microsoft.Bcl.Build.1.0.14\tools\Microsoft.Bcl.Build.targets" Condition="Exists('..\packages\Microsoft.Bcl.Build.1.0.14\tools\Microsoft.Bcl.Build.targets')" />
<Target Name="EnsureBclBuildImported" BeforeTargets="BeforeBuild" Condition="'$(BclBuildImported)' == ''">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 you can remove this Bcl target

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

// You can specify all the values or you can default the Build and Revision Numbers
// by using the '*' as shown below:
// [assembly: AssemblyVersion("1.0.*")]
[assembly: AssemblyVersion("1.0.0.0")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 same comment about keeping this values in line with the module version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -0,0 +1,18 @@
<?xml version="1.0" encoding="utf-8"?>
<packages>
<package id="Hyak.Common" version="1.0.2" targetFramework="net452" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 similarly to your .csproj file, you can remove all of the common packages (it should be everything but the Microsoft.Azure.Management.ResourceManager package)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@cormacpayne
Copy link
Member

@rajshah11 Hey Raj, the build is currently failing because it cannot find version 1.7.0-preview of Microsoft.Azure.Management.ResourceManager. You can do one of two things:

(1) If your changes are merged into the .NET SDK repository and the package is ready to be published, you can publish it to NuGet

(2) Create a signed NuGet package that you can place in tools/LocalFeed and use for testing until the package is pushed to NuGet

@cormacpayne cormacpayne changed the base branch from preview to mgmt-group-preview February 9, 2018 23:28
Copy link
Member

@cormacpayne cormacpayne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 a few more comments to take a look at. Also,

(1) Make sure to update the TestMappings.json file to include an entry for your tests

(2) Once the Microsoft.Azure.Management.ResourceManager SDK is published with your changes, we should look at reverting the changes you made to external projects that update the version of this SDK used

# IconUri = ''

# ReleaseNotes of this module
ReleaseNotes = '* Add support for online help
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 ping on this comment

- Run Get-Help with the -Online parameter to open the online help in your default Internet browser'

# External dependent modules of this module
# ExternalModuleDependencies = ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 if you updated your version of the PowerShellGet module to 1.6.0, you'll be able to update this module to include the Prerelease property, which you will want to set to preview. Here is an example of a module doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks.


function Test-ListManagementGroups
{
$response = Get-AzureRmManagementGroup
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 for all of the below tests, please be sure the necessary setup is included in the tests (i.e., if anything needs to be created before Get-AzureRmManagementGroup is called, that should be included). This makes it easier for others to record these tests if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!


function Test-RemoveManagementGroup
{
$response = Remove-AzureRmManagementGroup -GroupName TestPSNewGroup3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 same comment about necessary test setup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!


function Test-UpdateManagementGroup
{
$response = Update-AzureRmManagementGroup -GroupName TestPSNewGroup3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 same comment about including the necessary test setup for the below tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!

[Cmdlet(VerbsCommon.Remove, "AzureRmManagementGroup",
DefaultParameterSetName = Constants.ParameterSetNames.GroupOperationsParameterSet,
SupportsShouldProcess = true), OutputType(typeof(bool))]
public class RemoveAzureRmManagementGroup : AzureManagementGroupsCmdletBase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 this cmdlet should support the InputObject parameter set, so that a user could pipe the output of Get-AzureRmManagementGroup to this cmdlet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks

{
if (context.Subscription.Id != SubscriptionId.ToString())
{
Utility.AzureManagementGroupAutoRegisterSubscription(SubscriptionId.ToString(), context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 same comment about if this is necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cormacpayne Replied above

[Cmdlet(VerbsCommon.Remove, "AzureRmManagementGroupSubscription",
DefaultParameterSetName = Constants.ParameterSetNames.GroupOperationsParameterSet,
SupportsShouldProcess = true), OutputType(typeof(bool))]
public class RemoveAzureRmManagementGroupSubscription : AzureManagementGroupAutoRegisterCmdletBase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 same comment about supporting the InputObject parameter set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a Get for subscription in a group. I don't think piping should be supported here. Correct me if I am wrong. Thanks

namespace Microsoft.Azure.Commands.ManagementGroups.Cmdlets
{
[Cmdlet("Update", "AzureRmManagementGroup", DefaultParameterSetName = Constants.ParameterSetNames.GroupOperationsParameterSet, SupportsShouldProcess = true), OutputType(typeof(PSManagementGroupNoChildren))]
public class UpdateAzureRmManagementGroup : AzureManagementGroupsCmdletBase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 same comment about supporting the InputObject parameter set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks

</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
<Import Project="..\..\..\..\tools\Common.Dependencies.targets" />
<Import Project="..\packages\Microsoft.Bcl.Build.1.0.14\tools\Microsoft.Bcl.Build.targets" Condition="Exists('..\packages\Microsoft.Bcl.Build.1.0.14\tools\Microsoft.Bcl.Build.targets')" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajshah11 you should be able to remove this import

Copy link
Contributor Author

@rajshah11 rajshah11 Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. Thanks

@rajshah11
Copy link
Contributor Author

@cormacpayne made all the changes and the build is passing too. Just FYI we have moved out of the ResourceManager SDK for a SDK of our own. I have reverted the package references changes and made the new changes accordingly

@rajshah11 rajshah11 mentioned this pull request Feb 14, 2018
13 tasks
@maddieclayton
Copy link
Contributor

Closing in favor of the new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants