Service Fabric RP PowerShell #3783

chingchenmsft · 2017-04-17T18:00:25Z

Description

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.

msftclas · 2017-04-17T18:00:30Z

@chingchenmsft,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

azuresdkci · 2017-04-17T18:05:05Z

Can one of the admins verify this patch?

cormacpayne · 2017-04-17T22:40:20Z

@azuresdki add to whitelist

cormacpayne

@chingchenmsft a few initial comments before reviewing the cmdlets.

Also, in order for your tests to run, you will need to add a reference to your test dll to the following file

cormacpayne · 2017-04-17T22:44:44Z

...ds.ScenarioTests.ResourceManager.Common/Commands.ScenarioTests.ResourceManager.Common.csproj

@@ -52,6 +52,9 @@
    <Reference Include="Microsoft.Azure.Management.ResourceManager">
      <HintPath>..\..\..\packages\Microsoft.Azure.Management.ResourceManager.1.2.0-preview\lib\net45\Microsoft.Azure.Management.ResourceManager.dll</HintPath>
    </Reference>
+    <Reference Include="Microsoft.Azure.Management.ServiceFabric">


@chingchenmsft why was this reference added? #Resolved

removed.

In reply to: 111838002 [](ancestors = 111838002)

cormacpayne · 2017-04-17T22:45:03Z

src/ResourceManager/ServiceFabric/AzureRM.ServiceFabric.psd1

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


@chingchenmsft this will need to be version 2.8.0

sorry will fix it.

cormacpayne · 2017-04-17T22:47:37Z

src/ResourceManager/ServiceFabric/Commands.ServiceFabric/Commands.ServiceFabric.csproj

+      <Project>{3819d8a7-c62c-4c47-8ddd-0332d9ce1252}</Project>
+      <Name>Commands.ResourceManager.Common</Name>
+    </ProjectReference>
+    <ProjectReference Include="..\..\Profile\Commands.Profile\Commands.Profile.csproj">


@chingchenmsft why are you adding references to the Profile, Resources, and Tags projects?

i used a method in resource, and the resource need the other two to compile

In reply to: 111838392 [](ancestors = 111838392)

Adding references to other cmdlet projects is not allowed. There is a PR in the current stack that removes all of these for existing projects (#3734). Please ensure that you are using one of the common code libraries for accessing common features - this might mean moving some additional items into common code.

i checked, Key Vault also referenced the method, can you help to abstract it to common ?

cormacpayne · 2017-04-17T23:10:09Z

...ResourceManager/ServiceFabric/Commands.ServiceFabric.Test/Commands.ServiceFabric.Test.csproj

+    <None Include="ScenarioTests\TestServiceFabric.ps1">
+      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
+    </None>
+    <None Include="SessionRecords\Microsoft.Azure.Commands.ServiceFabric.Test.ScenarioTests.TestServiceFabric\TestAddAzureRmServiceFabricClientCertificate.json" />


@chingchenmsft all of session record json files will need to have the <CopyToOutputDirectory>Always</CopyToOutputDirectory> tag so that they can be copied and found during the build

into sfrp

markcowl

Some general comments:

(1) Please regenerate the wxi file following the instructions here:

(2) Please add your test project to azure.powershell.test.targets in the repo root

(3) Please add in the documentation folder and changelog for your module directory, and please fill out the changelog entiry introducing your new cmdlets
(4) As discussed in review, generally you should allow piping between cmdlets by using a parameter set that takes ValueFromPipeline using your own types, rather than using ValueFromPipelineByPropertyName over all the properties. Normally, only ResourceGroupName and Name should be set up for ValueFromPipelineByPropertyName

(5) You need to implement ShouldProcess for all cmdlets that make changes. You can find a discussion of appropriate ShouldProcess implementation here: https://gist.github.com/markcowl/338e16fe5c8bbf195aff9f8af0db585d

markcowl · 2017-04-21T23:13:14Z

src/ResourceManager/ServiceFabric/AzureRM.ServiceFabric.psd1

+# RootModule = ''
+
+# Version number of this module.
+ModuleVersion = '1.0.0'


This should probably start with '0.x.0', as I imagine your cmdlet surface area will be changing

changed to 0.1.0 , is this fine for our first version ?

markcowl · 2017-04-21T23:35:57Z

...er/ServiceFabric/Commands.ServiceFabric/Commands/AddAzureRmServiceFabricClientCertificate.cs

+        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, ParameterSetName = SingleUpdateWithCommonNameSet,
+                   HelpMessage = "Client authentication type")]
+        [ValidateNotNullOrEmpty()]
+        public virtual bool IsAdmin { get; set; }


You should not use boolean parameters. This should be a switch parameter

markcowl · 2017-04-21T23:37:31Z

...er/ServiceFabric/Commands.ServiceFabric/Commands/AddAzureRmServiceFabricClientCertificate.cs

+                   HelpMessage = "Specify client certificate thumbprint and authentication type")]
+        [ValidateNotNullOrEmpty()]
+        [Alias("ThumbprintsAndAuthenticationTypes")]
+        public Hashtable ThumbprintsAndTypes { get; set; }


This is a hashtable of thumbbprints and booleans? How about havein a parameter for admincertificatethumbprint that takes an array of certs and a parameter that takes non-admin certificate thumbprints? This hashtable strikes me as difficult to fire out.

markcowl · 2017-04-21T23:41:24Z

...urceManager/ServiceFabric/Commands.ServiceFabric/Commands/AddAzureRmServiceFabricNodeType.cs

+
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true,
+                   HelpMessage = "The sku name")]
+        [ValidateNotNullOrEmpty()]


Is there not a validateset for this?

this can't be set, it depend on region availability

markcowl · 2017-04-21T23:41:42Z

...urceManager/ServiceFabric/Commands.ServiceFabric/Commands/AddAzureRmServiceFabricNodeType.cs

+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true,
+                   HelpMessage = "Tier")]
+        [ValidateNotNullOrEmpty()]
+        public string Tier


ValidateSet?

this can't be set, it depend on region availability

markcowl · 2017-04-22T00:14:56Z

...anager/ServiceFabric/Commands.ServiceFabric/Commands/UpdateAzureRmServiceFabricDurability.cs

+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true,
+                   HelpMessage = "Specify Sku name of the node type")]
+        [ValidateNotNullOrEmpty()]
+        public string SkuName { get; set; }


Same, why not just Sku?

markcowl · 2017-04-22T00:15:09Z

...nager/ServiceFabric/Commands.ServiceFabric/Commands/UpdateAzureRmServiceFabricReliability.cs

+
+namespace Microsoft.Azure.Commands.ServiceFabric.Commands
+{
+    [Cmdlet(VerbsData.Update, CmdletNoun.AzureRmServiceFabricReliability), OutputType(typeof(PSCluster))]


SupportsShouldProcess

markcowl · 2017-04-22T00:15:20Z

...nager/ServiceFabric/Commands.ServiceFabric/Commands/UpdateAzureRmServiceFabricReliability.cs

+        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true,
+                   HelpMessage = "VM instance number")]
+        [ValidateNotNullOrEmpty()]
+        public ReliabilityLevel ReliabilityLevel { get; set; }


ValidateSet

it is Enum, still need ValidateSet?

markcowl · 2017-04-22T00:16:11Z

...eManager/ServiceFabric/Commands.ServiceFabric/Commands/UpdateAzureRmServiceFabricVmssBase.cs

+                   HelpMessage = "The number of nodes to add")]
+        [ValidateNotNullOrEmpty()]
+        [Alias("NumberOfNodesToAdd")]
+        public virtual int Number { get; set; }


I think you may want a ValidateRange, not a ValidateNotNullOrEmpty

markcowl · 2017-04-22T00:16:21Z

...eManager/ServiceFabric/Commands.ServiceFabric/Commands/UpdateAzureRmServiceFabricVmssBase.cs

+
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true,
+                   HelpMessage = "Node type name")]
+        public string NodeTypeName { get; set; }


markcowl · 2017-04-28T19:52:45Z

@chingchenmsft There are credscan matches causing the build to fail, you can find these summarized here: http://azuresdkci.cloudapp.net/job/powershell/8321/artifact/src/Package/credscan-filtered.tsv

markcowl

You need to resolve the credscan issues identified here: http://azuresdkci.cloudapp.net/job/powershell/8321/artifact/src/Package/credscan-filtered.tsv

Also, it looks like you didn't add a changelog.md for your new RP, please add a changelog, and put details on the current release in the 'Current Release' section. The changelog format is as here: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Cdn/ChangeLog.md, you only need the Current Release section

chingchenmsft · 2017-04-28T23:30:42Z

removed the cred, and add changelog.md

markcowl · 2017-04-29T00:42:07Z

on demand run here: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1493/

signing run here: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-sign/993/

LGTM once this passes

markcowl · 2017-04-30T22:27:34Z

on demand (including new changes): http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1504/

signing (including new changes): http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-sign/996/

Talked with Cormac, we agreed the dependencies are acceptabel for now

QingChenmsft added 5 commits April 4, 2017 10:45

add sfrp powershell

08323f6

fix nugget

d116bae

fix

5fd2630

fix md file

4ce63e9

add linux node type

5ced42b

chingchenmsft requested a review from hovsepm April 17, 2017 18:00

msftclas added the cla-not-required label Apr 17, 2017

azuresdkci assigned markcowl Apr 17, 2017

fix md

4ce8946

cormacpayne previously requested changes Apr 17, 2017

View reviewed changes

Revert back ScenarioTests.ResourceManager.Common.csproj

9cf655d

cormacpayne assigned chingchenmsft and unassigned markcowl Apr 18, 2017

cormacpayne added the needs-revision label Apr 18, 2017

QingChenmsft and others added 9 commits April 18, 2017 14:03

fix bugs

af8e534

Fixed link

0691c17

Fixed link

fa96ea6

Fixed link

b574e09

Fixed link

515b12e

wait key vault

121ef23

Merge branch 'sfrp' of https://github.com/chingchenmsft/azure-powershell

9b07047

into sfrp

refactor and use key vault to generate cert

eb2dc51

sf rp

365665d

markcowl requested changes Apr 22, 2017

View reviewed changes

QingChenmsft added 3 commits April 23, 2017 19:10

fix

76c9d99

fix

e14f49c

fix comments

7bd4e9f

QingChenmsft added 4 commits April 27, 2017 21:52

fix

2e73b0e

fix

bb41fb7

fix

851f6af

fix

b1ccea6

markcowl requested changes Apr 28, 2017

View reviewed changes

markcowl added needs-revision and removed needs-review labels Apr 28, 2017

QingChenmsft added 2 commits April 28, 2017 13:49

fix cred issue and update md file

0a865b4

fix md files

5d5311b

chingchenmsft added needs-review and removed needs-revision labels Apr 28, 2017

chingchenmsft added needs-review and removed needs-review labels Apr 28, 2017

QingChenmsft and others added 2 commits April 28, 2017 17:05

Merge https://github.com/Azure/azure-powershell into preview

e03a876

Update ChangeLog.md

e588dff

markcowl previously approved these changes Apr 29, 2017

View reviewed changes

markcowl added Waiting for On Demand and removed needs-review labels Apr 29, 2017

markcowl unassigned chingchenmsft Apr 29, 2017

add snk for test project and fix format of the md file

d0a3a85

chingchenmsft dismissed markcowl’s stale review via d0a3a85 April 29, 2017 03:23

QingChenmsft added 2 commits April 28, 2017 20:30

Merge branch 'preview' into sfrp

02dc1db

fix subject

2de33af

markcowl approved these changes May 1, 2017

View reviewed changes

markcowl merged commit 5e079f8 into Azure:preview May 1, 2017

Service Fabric RP PowerShell #3783

Service Fabric RP PowerShell #3783

Uh oh!

Conversation

chingchenmsft commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

msftclas commented Apr 17, 2017

Uh oh!

azuresdkci commented Apr 17, 2017

Uh oh!

cormacpayne commented Apr 17, 2017

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

cormacpayne Apr 17, 2017 • edited by chingchenmsft 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!

markcowl 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!

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!

chingchenmsft commented Apr 17, 2017 •

edited

Loading

cormacpayne Apr 17, 2017 •

edited by chingchenmsft

Loading

markcowl commented Apr 29, 2017 •

edited

Loading

markcowl commented Apr 30, 2017 •

edited

Loading