update New Roleassignment creation calls to have delegation flag by darshanhs90 · Pull Request #5023 · Azure/azure-powershell

darshanhs90 · 2017-11-23T01:00:14Z

Linked to SDK PR
Azure/azure-sdk-for-net#3872

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.

cormacpayne · 2017-11-27T17:36:58Z

@darshanhs90 feel free to remove the Do Not Merge label and assign this PR back to me when the SDK was been published and this PR has been updated

cormacpayne · 2017-11-30T19:18:18Z

@azuresdkci test this please

darshanhs90 · 2017-11-30T20:42:25Z

@cormacpayne Have updated the PR after the publish of the SDK

shawns1 · 2017-12-01T06:22:11Z

@azuresdkci test this please

…nto preview

cormacpayne

@darshanhs90 a few comments

cormacpayne · 2017-12-01T18:11:03Z

src/ResourceManager/Resources/Commands.Resources.Test/Commands.Resources.Test.csproj

      <Name>Commands.Common.Authentication</Name>
    </ProjectReference>
-    <ProjectReference Include="..\..\..\Common\Commands.Common.Authorization\Commands.Common.Authorization.csproj">
+   <!-- <ProjectReference Include="..\..\..\Common\Commands.Common.Authorization\Commands.Common.Authorization.csproj">


@darshanhs90 please remove this section if you are going to comment it out #Resolved

cormacpayne · 2017-12-01T18:11:28Z

src/ResourceManager/Resources/Commands.Resources/Commands.Resources.csproj

      <Name>Commands.Common.Authentication.Abstractions</Name>
    </ProjectReference>
-    <ProjectReference Include="..\..\..\Common\Commands.Common.Authorization\Commands.Common.Authorization.csproj">
+<!--    <ProjectReference Include="..\..\..\Common\Commands.Common.Authorization\Commands.Common.Authorization.csproj">


@darshanhs90 same comment about removing this section #Resolved

cormacpayne · 2017-12-01T18:13:26Z

src/ResourceManager/Resources/Commands.Resources/packages.config

 <packages>
  <package id="Microsoft.Azure.Management.ResourceManager" version="1.6.0-preview" targetFramework="net452" />
  <package id="Microsoft.Azure.Management.Resources" version="2.20.0-preview" targetFramework="net45" />
+  <package id="Microsoft.Azure.Management.Authorization" version="2.6.0-preview" targetFramework="net452" />


@darshanhs90 since you are adding a new package to your module, you will need to do two additional things:

(1) Add the new package to the RequiredAssemblies field in AzureRM.Resources

(2) Update the .wxi file #Resolved

@cormacpayne Regenerated the wxi file as per the steps mentioned , Let me know if there is anything that needs to be done since this feature was planned to be included for the upcoming 12/8 powersheel release.

cormacpayne · 2017-12-01T18:16:21Z

...esourceManager/Resources/Commands.Resources/RoleAssignments/NewAzureRoleAssignmentCommand.cs

+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, ParameterSetName = ParameterSet.ResourceWithSPN,
+            HelpMessage = "Delegation flag.")]
+        [ValidateNotNullOrEmpty]
+        public bool CanDelegate { get; set; }


@darshanhs90 two comments about this parameter:

(1) Since it can be found in every parameter set, it would be best if you would put just one Parameter attribute on this, but give it no ParameterSetName property - this will put the parameter into every single parameter set and you won't have to copy-paste the Parameter attribute for every single set it can belong to

(2) Rather than using a bool, this should be a SwitchParameter that a user can set. #Resolved

darshanhs90 · 2017-12-04T05:37:23Z

@cormacpayne Have removed the property ValueFromPipelineByPropertyName for the switch parameter

cormacpayne · 2017-12-07T06:31:05Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/259/

darshanhs90 · 2017-12-07T06:32:24Z

@cormacpayne Should i change the base branch to release-5.1.0 for it to make into the upcoming release?

cormacpayne · 2017-12-07T06:36:38Z

@darshanhs90 good call, forgot to do that. I just changed the base branch and there looks to be some merge conflicts. Would you mind resolving these?

darshanhs90 · 2017-12-07T06:37:20Z

Sure will do right away

darshanhs90 · 2017-12-07T07:35:48Z

@cormacpayne Fixed the merge conflicts

cormacpayne · 2017-12-07T16:56:33Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/261/

azuresdkci added the needs-review label Nov 23, 2017

darshanhs90 changed the title ~~update Roleasignment calls to have delegation flag~~ [waiting for SDK PR completion] update Roleasignment calls to have delegation flag Nov 23, 2017

darshanhs90 added Do Not Merge 🚫 and removed needs-review labels Nov 23, 2017

azuresdkci assigned cormacpayne Nov 23, 2017

darshanhs90 changed the title ~~[waiting for SDK PR completion] update Roleasignment calls to have delegation flag~~ [waiting for SDK PR completion] update Roleassignment calls to have delegation flag Nov 23, 2017

cormacpayne added the needs-revision label Nov 27, 2017

cormacpayne assigned darshanhs90 and unassigned cormacpayne Nov 27, 2017

darshanhs90 removed the Do Not Merge 🚫 label Nov 30, 2017

darshanhs90 changed the title ~~[waiting for SDK PR completion] update Roleassignment calls to have delegation flag~~ update New Roleassignment creation calls to have delegation flag Nov 30, 2017

maddieclayton assigned darshanhs90 and unassigned darshanhs90 Nov 30, 2017

darshanhs90 assigned cormacpayne and unassigned darshanhs90 Nov 30, 2017

darshanhs90 added needs-review and removed needs-revision labels Nov 30, 2017

darshanhs90 added this to the December 2017 milestone Nov 30, 2017

darshanhs90 added 3 commits December 1, 2017 00:26

update RA calls with new version

4911c26

update test proj

9799e34

update session recorsds

fa56841

darshanhs90 force-pushed the preview branch from 1b2aaf2 to fa56841 Compare December 1, 2017 09:04

darshanhs90 added 2 commits December 1, 2017 09:55

revert extra session recs

18a4dc7

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

404d07f

…nto preview

cormacpayne suggested changes Dec 1, 2017

View reviewed changes

cormacpayne added the needs-revision label Dec 1, 2017

cormacpayne added needs-revision and removed needs-review labels Dec 4, 2017

cormacpayne assigned darshanhs90 and unassigned cormacpayne Dec 4, 2017

update as per the comments

7ebbb37

darshanhs90 assigned cormacpayne and unassigned darshanhs90 Dec 4, 2017

darshanhs90 added needs-review and removed needs-revision labels Dec 4, 2017

markcowl added needs-revision and removed needs-review labels Dec 5, 2017

update commandlet to allowdelegation

576ef2d

darshanhs90 added needs-review and removed needs-revision labels Dec 7, 2017

cormacpayne previously approved these changes Dec 7, 2017

View reviewed changes

cormacpayne added Waiting for On Demand and removed needs-review labels Dec 7, 2017

cormacpayne removed their assignment Dec 7, 2017

cormacpayne changed the base branch from preview to release-5.1.0 December 7, 2017 06:35

fix merge ocnflicts

0d862f6

darshanhs90 dismissed cormacpayne’s stale review via 0d862f6 December 7, 2017 07:28

cormacpayne approved these changes Dec 7, 2017

View reviewed changes

cormacpayne merged commit 394590a into Azure:release-5.1.0 Dec 7, 2017

Conversation

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

Uh oh!

Description

General Guidelines

Testing Guidelines

Cmdlet Signature Guidelines

Cmdlet Parameter Guidelines

Uh oh!

cormacpayne commented Nov 27, 2017

Uh oh!

cormacpayne commented Nov 30, 2017

Uh oh!

darshanhs90 commented Nov 30, 2017

Uh oh!

shawns1 commented Dec 1, 2017

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

cormacpayne Dec 1, 2017 • edited by darshanhs90 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne Dec 1, 2017 • edited by darshanhs90 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne Dec 1, 2017 • edited by darshanhs90 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darshanhs90 Dec 1, 2017 • 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!

cormacpayne Dec 1, 2017 • edited by darshanhs90 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darshanhs90 commented Dec 4, 2017

Uh oh!

cormacpayne commented Dec 7, 2017

Uh oh!

darshanhs90 commented Dec 7, 2017

Uh oh!

cormacpayne commented Dec 7, 2017

Uh oh!

darshanhs90 commented Dec 7, 2017

Uh oh!

darshanhs90 commented Dec 7, 2017

Uh oh!

cormacpayne commented Dec 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

darshanhs90 commented Nov 23, 2017 •

edited

Loading

cormacpayne Dec 1, 2017 •

edited by darshanhs90

Loading

cormacpayne Dec 1, 2017 •

edited by darshanhs90

Loading

cormacpayne Dec 1, 2017 •

edited by darshanhs90

Loading

darshanhs90 Dec 1, 2017 •

edited

Loading

cormacpayne Dec 1, 2017 •

edited by darshanhs90

Loading