Policy parameters support in New-AzureRmPolicyDefinition and New-AzureRmPolicyAssignment #3476

luanshixia · 2017-02-07T22:49:16Z

Description

Added support for policy parameters to the 2 cmdlets to use the feature provided by API version 2016-12-01.

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.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.
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.

azurecla · 2017-02-07T22:49:23Z

Hi @luanshixia, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

It looks like you're working at Microsoft (wayan). If you're full-time, we DON'T require a contribution license agreement.

If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

azuresdkci · 2017-02-07T22:50:12Z

Can one of the admins verify this patch?

maphad · 2017-02-08T18:50:29Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

@@ -54,6 +61,19 @@ public class NewAzurePolicyAssignmentCmdlet : PolicyAssignmentCmdletBase
        public PSObject PolicyDefinition { get; set; }

        /// <summary>
+        /// Gets or sets the policy assignment policy parameter object.
+        /// </summary>
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The pollicy parameter object.")]


Typo in the help message "pollicy" should be "policy"

maphad · 2017-02-08T18:51:30Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+        /// <summary>
+        /// Gets or sets the policy assignment policy parameter file or string.
+        /// </summary>
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The pollicy parameter file or string.")]


Typo in the help message "pollicy"

maphad · 2017-02-08T18:56:14Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

@@ -112,11 +132,88 @@ private JToken GetResource()
                {
                    DisplayName = this.DisplayName ?? null,
                    PolicyDefinitionId = this.PolicyDefinition.Properties["policyDefinitionId"].Value.ToString(),
-                    Scope = this.Scope
+                    Scope = this.Scope,
+                    Parameters = GetParameters()


we usually scope the function calls, so this should be this.GetParameters()

maphad · 2017-02-08T18:58:33Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+        /// Gets or sets the policy assignment policy parameter object.
+        /// </summary>
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The pollicy parameter object.")]
+        public Hashtable PolicyParameterObject { get; set; }


We should have validation so that user cannot specify both PolicyParameterObject and PolicyParameters.
This can be implemented using parameter sets.

maphad · 2017-02-08T18:59:28Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+        /// </summary>
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The pollicy parameter file or string.")]
+        [ValidateNotNullOrEmpty]
+        public string PolicyParameters { get; set; }


Should we rename this to PolicyParametersFile?

Just confirmed with Mike that it is the expected behavior.

maphad · 2017-02-08T19:02:48Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+            // Load parameters from local file or literal
+            if (PolicyParameters != null)
+            {
+                string policyParameterFilePath = this.TryResolvePath(PolicyParameters);


I think it is confusing to use a single parameter as maybe a file name or maybe a string representing parameters.
We should explicitly make them two separate parameters. So in total we should have three parameters:
PolicyParameters
PolicyParametersFile
PolicyParametersObject.
Each should be in its own ParameterSet so that user can only specify one of them.

Were you specifically asked to use just two parameters?

For how to use ParameterSets u can refer to this documentation https://msdn.microsoft.com/en-us/library/dd878348(v=vs.85).aspx. Also several of our existing cmdlets use parameter sets.

For the 1st one: Just confirmed with Mike that it is the expected behavior.
For the 2nd one: Yes will do that.

maphad · 2017-02-08T19:04:59Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+            {
+                return null;
+            }
+            else


No need of else here since you are returning control from if

maphad · 2017-02-08T19:08:02Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+
+        object IDynamicParameters.GetDynamicParameters()
+        {
+            if (PolicyDefinition != null)


scope it as this.PolicyDefinition, everywhere

maphad · 2017-02-08T19:16:53Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyDefinition.cs

+        /// </summary>
+        private JToken GetParametersObject()
+        {
+            string parametersFilePath = this.TryResolvePath(this.Parameters);


As above I think we shouldn't use the same parameter as maybe a filePath or a parameter. It would be cleaner to have two separate parameters in two different parameter sets.

Just confirmed with Mike that it is the expected behavior.

maphad · 2017-02-08T19:18:26Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

                }
            };

            return policyassignmentObject.ToJToken();
        }
+
+        object IDynamicParameters.GetDynamicParameters()


who calls this method and can u point me to how dynamic parameters work?

This interface method is called by the PowerShell engine (thus a callback), so PowerShell can know what parameters are defined dynamically in the cmdlet and allow users to provide values for them.

maphad · 2017-02-08T19:20:21Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyDefinition.cs

@@ -113,7 +120,8 @@ private JToken GetResource()
                {
                    Description = this.Description ?? null,
                    DisplayName = this.DisplayName ?? null,
-                    PolicyRule = JObject.Parse(GetPolicyRuleObject().ToString())
+                    PolicyRule = JObject.Parse(GetPolicyRuleObject().ToString()),
+                    Parameters = this.Parameters == null ? null : JObject.Parse(GetParametersObject().ToString())


this.GetParametersObject

maphad · 2017-02-08T19:26:39Z

...ger/Resources/Commands.ResourceManager/Cmdlets/Entities/Policy/PolicyAssignmentProperties.cs

@@ -15,6 +15,7 @@
 namespace Microsoft.Azure.Commands.ResourceManager.Cmdlets.Entities.Policy
 {
    using Newtonsoft.Json;


This looks like it has 3 commits. We should squash changes to a single commit, so that it is easy to revert the entire change if required.

This can be a 1-click when merging on this page, though I don't have permission to do that now.

ok please squash before merging then.

maphad · 2017-02-08T19:27:35Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyDefinition.cs

+            return File.Exists(parametersFilePath)
+                ? JToken.FromObject(FileUtilities.DataStore.ReadFileAsText(parametersFilePath))
+                : JToken.FromObject(this.Policy);
+        }
    }


You can add scenario tests once you have addressed the PR comments.
Also get this PR reviewed by vivsrini.

cormacpayne · 2017-02-08T21:51:58Z

@azuresdkci add to whitelist

vivsriaus · 2017-02-08T22:42:39Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

                }
            };

            return policyassignmentObject.ToJToken();
        }
+
+        object IDynamicParameters.GetDynamicParameters()


This doesn't look like it belongs here. Can we move this out to a common/util file?

This is the implementation of the interface IDynamicParameters, which the class conforms to.

vivsriaus · 2017-02-08T22:43:52Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs


    /// <summary>
    /// Creates a policy assignment.
    /// </summary>
    [Cmdlet(VerbsCommon.New, "AzureRmPolicyAssignment"), OutputType(typeof(PSObject))]
-    public class NewAzurePolicyAssignmentCmdlet : PolicyAssignmentCmdletBase
+    public class NewAzurePolicyAssignmentCmdlet : PolicyAssignmentCmdletBase, IDynamicParameters


Please add scenario tests/moq tests for both these cmdlets

vivsriaus · 2017-02-08T22:45:23Z

...ger/Resources/Commands.ResourceManager/Cmdlets/Entities/Policy/PolicyDefinitionProperties.cs

+        /// The parameters declaration.
+        /// </summary>
+        [JsonProperty(Required = Required.Default)]
+        public JObject Parameters { get; set; }


Can you please confirm that there are no changes required for other policy cmdlets? i.e.,

Get/set/remove policy definition works with and without parameters

Get/set/remove policy assignment works with and without parameters

AFAIK, no impact at this time.

vivsriaus · 2017-02-08T22:47:18Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyDefinition.cs

+        /// </summary>
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The parameters for policy definition. This can either be a path to a file name containing the parameters declaration, or the parameters declaration as string.")]
+        [ValidateNotNullOrEmpty]
+        public string Parameters { get; set; }


PolicyDefinitionParameters?

vivsriaus · 2017-02-08T22:48:43Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+        [Parameter(ParameterSetName = PolicyParametersStringParameterSetName, 
+            Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The policy parameters file path or policy parameters string.")]
+        [ValidateNotNullOrEmpty]
+        public string PolicyParameters { get; set; }


PolicyAssignmentParameters?

TBH, the names are confusing as-is (what is the difference between parameters in policy def vs policy assignment?)

Also, please ensure the help files are updated accordingly

For 1: So the parameters in policy def are declarations and those in policy assignment are actual values. I know the names are confusing, but I have confirmed with Mike about these names.
For 2: Will do that.

vivsriaus · 2017-02-08T22:51:24Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyDefinition.cs

@@ -59,6 +59,13 @@ public class NewAzurePolicyDefinitionCmdlet : PolicyDefinitionCmdletBase
        public string Policy { get; set; }

        /// <summary>
+        /// Gets or sets the parameters parameter


vivsriaus · 2017-02-08T22:55:37Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs


    /// <summary>
    /// Creates a policy assignment.
    /// </summary>
    [Cmdlet(VerbsCommon.New, "AzureRmPolicyAssignment"), OutputType(typeof(PSObject))]
-    public class NewAzurePolicyAssignmentCmdlet : PolicyAssignmentCmdletBase
+    public class NewAzurePolicyAssignmentCmdlet : PolicyAssignmentCmdletBase, IDynamicParameters


No api-version change for policy cmdlets?

cormacpayne · 2017-02-09T18:46:55Z

@luanshixia the build is failing since you are introducing parameters PolicyParameters and Parameters, which do not follow the PowerShell convention of parameters having a singular noun

maphad · 2017-02-09T22:43:06Z

src/ResourceManager/Resources/Commands.ResourceManager/Cmdlets/Components/Constants.cs

@@ -63,7 +63,7 @@ public static class Constants
        /// <summary>
        /// The default policy API version.
        /// </summary>
-        public static readonly string PolicyApiVersion = "2016-04-01";
+        public static readonly string PolicyApiVersion = "2016-12-01";


just curious as to why the new API version date is set to December instead of a more recent date?

Per the resource schema seen on Azure Portal, the latest version for policy is 2016-12-01:

{ "resourceType": "policyDefinitions", "locations": [], "apiVersions": [ "2016-12-01", "2016-04-01", "2015-10-01-preview" ] }, { "resourceType": "policyAssignments", "locations": [], "apiVersions": [ "2016-12-01", "2016-04-01", "2015-10-01-preview" ] },

maphad · 2017-02-09T22:46:23Z

...ger/Resources/Commands.ResourceManager/Cmdlets/Entities/Policy/PolicyAssignmentProperties.cs

+        /// The parameter values.
+        /// </summary>
+        [JsonProperty(Required = Required.Default)]
+        public JObject Parameters { get; set; }


You can rename this to Parameter to be consistent with the PolicyDefinitionProperties class

Unfortunately this property cannot be renamed because that should match the REST API. (I also only realized this when I caught an error after trying to keep consistency). Of course we can match names using JsonProperty attribute, but no need.

luanshixia · 2017-02-10T18:55:30Z

@cormacpayne Thanks so much for the help. I will rollback my workaround once you have a fix.

maphad · 2017-02-13T22:56:40Z

src/ResourceManager/Resources/Commands.ResourceManager/Cmdlets/Extensions/JsonExtensions.cs

+                parameterObject.Add(paramKey, valueObject);
+            }
+            return parameterObject;
+        }
    }
 }


Add a newline at end of file.

maphad · 2017-02-13T22:58:29Z

src/ResourceManager/Resources/Commands.ResourceManager/Cmdlets/Extensions/JsonExtensions.cs

+                keys = dict.Keys;
+            }
+            var parameterObject = new JObject();
+            foreach (string paramKey in keys)


you can use var instead of string as is the convention.

Here "keys" is of type IEnumerable not IEnumerable<string> so paramKey will not be inferred as string unless declared specific.

maphad · 2017-02-13T23:00:57Z

src/ResourceManager/Resources/Commands.ResourceManager/Cmdlets/Extensions/JsonExtensions.cs

+        /// <param name="dict">The <see cref="IDictionary"/> object.</param>
+        /// <param name="keys">The key set to extract from the <see cref="IDictionary"/> object. Default is all keys.</param>
+        /// <returns>The conversion result.</returns>
+        public static JObject ToParameterObject(this IDictionary dict, IEnumerable keys = null)


The method name ToParameterObject is specific to its use in the policy commandlet.
Since this is a utility method it can be used from elsewhere. Can we use a more generic name like ToJsonObject or ToJObject?

maphad · 2017-02-13T23:07:22Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

@@ -50,10 +61,31 @@ public class NewAzurePolicyAssignmentCmdlet : PolicyAssignmentCmdletBase
        /// <summary>
        /// Gets or sets the policy assignment policy definition parameter.
        /// </summary>
-        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The pollicy definition object.")]
+        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The policy definition object.")]


Is this line required?

So @cormacpayne is working on a fix for BreakingChangeAnalyzer which prevents this PR from passing the build (the reason: the analyzer compares the parameter set with the previous build. In previous build this property belongs to the default parameter set, so it enforces this for later builds). This line is a workaround which can be removed after his fix.

maphad · 2017-02-13T23:09:03Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+            }
+
+            return null;
+        }
    }
 }


Add newline at end of file.

maphad · 2017-02-13T23:11:23Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyDefinition.cs

@@ -113,7 +120,8 @@ private JToken GetResource()
                {
                    Description = this.Description ?? null,
                    DisplayName = this.DisplayName ?? null,
-                    PolicyRule = JObject.Parse(GetPolicyRuleObject().ToString())
+                    PolicyRule = JObject.Parse(GetPolicyRuleObject().ToString()),
+                    Parameters = this.Parameter == null ? null : JObject.Parse(GetParametersObject().ToString())


scope local method calls as this.GetParametersObject()

vivsriaus

LGTM

luanshixia · 2017-02-14T18:55:44Z

@cormacpayne I have already got two peer approvals but I don't have the permission to merge it. Could you please merge it so that it can make the 2/15 deadline? Thanks!

cormacpayne

@luanshixia a couple of comments

cormacpayne · 2017-02-14T20:19:35Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

@@ -50,10 +61,31 @@ public class NewAzurePolicyAssignmentCmdlet : PolicyAssignmentCmdletBase
        /// <summary>
        /// Gets or sets the policy assignment policy definition parameter.
        /// </summary>
-        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The pollicy definition object.")]
+        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The policy definition object.")]


@luanshixia if this parameter is required in every single parameter set, why make this change? Previously it was a globally required parameter, so it was required in any parameter set you used. The same applies now with the changes you are trying to make here.

@cormacpayne So there are 3 usages:

PolicyDefinition only

PolicyDefinition + PolicyParameterObject

PolicyDefinition + PolicyParameter

We don't want users to set both PolicyParameterObject and PolicyParameter, so we use two different sets for them. But we also need to allow users to set none of PolicyParameterObject and PolicyParameter, which comes the third set. Note we have to set Mandatory=true for PolicyParameterObject and PolicyParameter or we will get parameter set resolving conflict.

@luanshixia I am fine with the three different parameter sets, but there is no need to change PolicyDefinition from a global parameter to a parameter that's in each of the three parameter sets (that is what is implied by being a global parameter).

cormacpayne · 2017-02-14T20:22:41Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+
+        protected const string PolicyParameterObjectParameterSetName = "Policy assignment with parameters via policy parameter object";
+        protected const string PolicyParameterStringParameterSetName = "Policy assignment with parameters via policy parameter string";
+        protected const string ParameterlessPolicyParameterSetName = "Policy assignment without parameters";


@luanshixia the name of this parameter set is misleading - Name and Scope (and PolicyDefinition based on its implementation) parameters are globally required parameters, so you are always going to need to provide a value for them, meaning you'll never have a parameter set that is "parameterless"

@cormacpayne No, "parameter" here refers to the policy parameters, not the cmdlet parameters.. This is similar to what is done for template parameters in NewAzureRmResourceGroupDeployment.

cormacpayne · 2017-02-14T20:38:24Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

                }
            };

            return policyassignmentObject.ToJToken();
        }
+
+        object IDynamicParameters.GetDynamicParameters()


@luanshixia why do you need to add dynamic parameters to this cmdlet? We try to avoid using them as much as possible since they cannot be picked up by our StaticAnalysis tooling and don't allow us to easily check any public interface changes

@cormacpayne Because we want to allow users to specify the policy parameters through cmdlet parameters, so we have to dynamically parse the policy definition and extract a list of defined parameters. That is from the requirements. In the future, we also want to add IntelliSense for the dynamic parameters, just like what we did for template deployment.

cormacpayne · 2017-02-14T20:41:02Z

src/ResourceManager/Resources/Commands.Resources/help/New-AzureRmPolicyAssignment.md

@@ -175,6 +190,32 @@ Accept pipeline input: False
 Accept wildcard characters: False
 ```

+### -PolicyParameter
+The policy parameter file path or policy parameter string.```yaml


@luanshixia please move "```yaml" to a new line since it is causing errors with the displayed markdown

cormacpayne · 2017-02-14T20:41:15Z

src/ResourceManager/Resources/Commands.Resources/help/New-AzureRmPolicyAssignment.md

+```
+
+### -PolicyParameterObject
+The policy parameter object.```yaml


@luanshixia same comment

cormacpayne · 2017-02-14T20:41:23Z

src/ResourceManager/Resources/Commands.Resources/help/New-AzureRmPolicyDefinition.md

@@ -171,6 +171,19 @@ Accept pipeline input: False
 Accept wildcard characters: False
 ```

+### -Parameter
+The parameters declaration for policy definition. This can either be a path to a file name containing the parameters declaration, or the parameters declaration as string.```yaml


@luanshixia same comment

cormacpayne · 2017-02-15T20:31:38Z

@vivsriaus can you approve the review you have started? You commented with LGTM, but it still says you have changes requested.

cormacpayne

@luanshixia a couple of more comments after talking with @markcowl

cormacpayne · 2017-02-15T20:34:07Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+                            var dp = new RuntimeDefinedParameter
+                            {
+                                Name = param.Name,
+                                ParameterType = typeString.Equals("array", StringComparison.OrdinalIgnoreCase) ? typeof(object[]) : typeof(string)


@luanshixia can you leverage typeString to include a switch case for types you would want to define (e.g., int, bool, etc.)? It seems like a bad idea to have the default type of non-arrays as string

@cormacpayne Per Policy doc, "the type of a parameter can be either string or array".

cormacpayne · 2017-02-15T20:34:47Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+                            dp.Attributes.Add(new ParameterAttribute
+                            {
+                                Mandatory = false,
+                                ValueFromPipelineByPropertyName = true,


@luanshixia what kind of scenario would lend users to use the ValueFromPipelineByPropertyName for these dynamic parameters?

@cormacpayne Similarly, the user can put dynamic parameters in an object (say $param) and say: $param | New-AzureRmPolicyAssignment

cormacpayne · 2017-02-15T20:35:14Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+                            {
+                                Mandatory = false,
+                                ValueFromPipelineByPropertyName = true,
+                                HelpMessage = param.Name


@luanshixia this is not good help for users, it's just repeating the parameter name

cormacpayne · 2017-02-15T20:36:20Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+                                Name = param.Name,
+                                ParameterType = typeString.Equals("array", StringComparison.OrdinalIgnoreCase) ? typeof(object[]) : typeof(string)
+                            };
+                            dp.Attributes.Add(new ParameterAttribute


@luanshixia the dynamic parameters will need to be in the Parameterless parameter set, or else users will be able to use them with the provided hashtable or json, which will defeat the purpose of having the dynamic parameters

@cormacpayne Good catch.

cormacpayne · 2017-02-15T20:37:45Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Policy/NewAzurePolicyAssignment.cs

+        [Parameter(ParameterSetName = PolicyParameterObjectParameterSetName,
+            Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The policy definition object.")]
+        [Parameter(ParameterSetName = PolicyParameterStringParameterSetName,
+            Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The policy definition object.")]


@luanshixia how will users use this ValueFromPipelineByPropertyName attribute for this parameter? Will there be an object they can pass through the pipeline to this cmdlet that contains the PolicyDefinition property? Will it also include the below parameters you are trying to add since they also have the ValueFromPipelineByPropertyName attribute as well?

@cormacpayne I checked the existing code and found pipeline usage is supported across all Resources cmdlets. So the user can do like: @{"PolicyDefinition": @{...} ...} | New-AzureRmPolicyAssignment

…viding more useful help messages.

cormacpayne · 2017-02-16T01:43:09Z

On-demand: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1397/

cormacpayne · 2017-02-16T01:44:09Z

LGTM once on-demand passes

luanshixia added 3 commits February 3, 2017 11:19

Added -Parameters support for New-AzureRmPolicyDefinition

c5ceee2

A temp edit

1c9e87b

Finished New-AzureRmPolicyAssignment

cfbca37

azurecla added the cla-not-required label Feb 7, 2017

maphad reviewed Feb 8, 2017

View reviewed changes

Resolved code review issues

d4537e9

vivsriaus suggested changes Feb 8, 2017

View reviewed changes

Updated help

b487ba2

cormacpayne added the Changes Requested label Feb 9, 2017

luanshixia added 3 commits February 9, 2017 11:37

PowerShell cmdlet parameter names should be sigular noun.

ef53917

Fixed potential NullReferenceException

bf9aec6

The build server doesn't support ?. syntax

81f9384

maphad reviewed Feb 9, 2017

View reviewed changes

luanshixia added 3 commits February 9, 2017 17:01

Specify DefaultParameterSetName

fb1d10e

Naming consistency

6fc4b9e

To make the BreakingChangeAnalyzer happy...

0b2b101

luanshixia added 2 commits February 10, 2017 12:17

Refactoring; also rolling back a previous wrong renaming

26a48e9

Added scenario tests for this changeset

0bf829f

maphad reviewed Feb 13, 2017

View reviewed changes

Code styles

e80e719

maphad approved these changes Feb 13, 2017

View reviewed changes

vivsriaus reviewed Feb 14, 2017

View reviewed changes

cormacpayne suggested changes Feb 14, 2017

View reviewed changes

cormacpayne self-assigned this Feb 14, 2017

Fixed mark down help

5dfaed9

cormacpayne suggested changes Feb 15, 2017

View reviewed changes

Improved dynamic properties by adding them to a parameter set and pro…

7b5de51

…viding more useful help messages.

vivsriaus approved these changes Feb 15, 2017

View reviewed changes

Disallow pipeline values for dynamic properties; object[]->string[]

5cf1478

cormacpayne approved these changes Feb 16, 2017

View reviewed changes

cormacpayne added Waiting for On Demand and removed Changes Requested labels Feb 16, 2017

cormacpayne merged commit c089f4b into Azure:dev Feb 16, 2017

Policy parameters support in New-AzureRmPolicyDefinition and New-AzureRmPolicyAssignment #3476

Policy parameters support in New-AzureRmPolicyDefinition and New-AzureRmPolicyAssignment #3476

Uh oh!

Conversation

luanshixia commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

azurecla commented Feb 7, 2017

Uh oh!

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

maphad Feb 8, 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!

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!

cormacpayne commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luanshixia Feb 8, 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!

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!

luanshixia commented Feb 7, 2017 •

edited

Loading

maphad Feb 8, 2017 •

edited

Loading

luanshixia Feb 8, 2017 •

edited

Loading

maphad Feb 13, 2017 •

edited

Loading

luanshixia Feb 13, 2017 •

edited

Loading