Skip to content

Fix UseIdenticalMandatoryParametersForDSC rule logic #770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Jun 6, 2017

Conversation

kapilmb
Copy link

@kapilmb kapilmb commented Jun 3, 2017

Resolves #596

This PR also fixes the violation extent of the rule, which prior to this would mark the entire script. This prevented rule suppression from working when the suppression attribute was placed inside a function.

Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

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

Looks great man, nice work!

/// <param name="paramBlockAst">If a parameter block is present, set this argument's value to the parameter block.</param>
/// <returns></returns>
public static IEnumerable<ParameterAst> GetParameterAsts(
this FunctionDefinitionAst functionDefinitionAst,
out ParamBlockAst paramBlockAst)
{
// todo instead of returning null return an empty enumerator if no parameter is found
Copy link
Contributor

Choose a reason for hiding this comment

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

Always a good thing to do!

@kapilmb
Copy link
Author

kapilmb commented Jun 5, 2017

Thanks @daviwil

@@ -4,13 +4,24 @@

## Description

The `Get-TargetResource`, `Test-TargetResource` and `Set-TargetResource` functions of DSC Resource must have the same mandatory parameters.
For script based DSC resources the `Get-TargetResource`, `Test-TargetResource` and `Set-TargetResource` functions must have identical mandatory parameters that are also the keys in the corresponding `mof` file.
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the 'that are also the keys in the corresponding mof file.' While this is usually true, sometimes there are mandatory parameters that aren't keys.

Copy link
Author

Choose a reason for hiding this comment

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

But isn't the rule supposed to look for identical mandatory parameters in Get/Test/Set functions only if they are present as keys in the mof file?

Copy link
Author

Choose a reason for hiding this comment

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

According to this, the rule should be looking for all the properties in the mof file.

All three functions must take a parameter set that is identical to the set of properties defined in the MOF schema that you created for your resource. In this document, this set of properties is referred to as the “resource properties.”

It appears I had the wrong understanding about the rule :(. Guess I need to update the logic then, because the current logic only looks for keys that defined as identical mandatory parameters.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sorry about the confusion. It's a little confusing

Copy link
Author

Choose a reason for hiding this comment

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

Oh no worries.
So, I went through some of the script based resources and this is what I have found out:

  • If a parameter is declared as mandatory in get-targetresource, then it has to be mandatory in the corresponding set and test functions.
  • If a field is declared with attributes Key or Required in a mof file, then it should present as mandatory parameter in get/set/test functions.
  • set and test functions should have identical mandatory parameters.

Is that correct?

Copy link
Author

Choose a reason for hiding this comment

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

The last point should be: set and test functions should have identical parameters regardless of whether they are mandatory or not.

Copy link
Member

Choose a reason for hiding this comment

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

Yes - except the last point is a little misleading. Set and Test functions should have all identical parameters, not just mandatory.

Also the first point should read: If a parameter is declared as mandatory in any of Get/Set/Test then it should be a mandatory parameter in all of Get/Set/Test

Copy link
Author

Choose a reason for hiding this comment

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

Got it. Thanks!


## How

Correct the mandatory parameters for the functions in DSC resource.
Make sure all the keys have equivalent mandatory parameters in the `Get/Set/Test` functions.

## Example
Copy link
Member

Choose a reason for hiding this comment

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

I would modify this example as such:

Wrong

function Get-TargetResource
{
    [CmdletBinding()]
    param
    (
        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Message
    )
}

function Set-TargetResource
{
    [CmdletBinding()]
    param
    (
        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Message,

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Name
    )
}

function Test-TargetResource
{
    [CmdletBinding()]
    param
    (
        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Message,

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Name
    )
}

Correct

function Get-TargetResource
{
    [CmdletBinding()]
    param
    (
        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Message,

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Name
    )
}

function Set-TargetResource
{
    [CmdletBinding()]
    param
    (
        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Message,

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Name
    )
}

function Test-TargetResource
{
    [CmdletBinding()]
    param
    (
        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Message,

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [String]
        $Name
    )
}

Because this is most often what we see people doing wrong.

Kapil Borle added 4 commits June 5, 2017 17:31
This update the logic in check for properties that have the `required` attribute. Also updates the error string to report if the missing parameters is `key` or `required`.
If a parameter is declared as `mandatory` in any of the `Get/Set/Test` functions, then it should be a mandatory parameter in all the three functions.
This removes the constraint that a parameter needs to be declared `mandatory` in the `Get/Test/Set` functions if any of them declares the parameter as `mandatory`.

These DSC functions are supposed to take in parameters that declared in the corresponding mof file and only the parameters that are declared as `key` or `required` should be mandatory in all the three functions.
@kapilmb kapilmb merged commit 81fafcc into development Jun 6, 2017
@kapilmb kapilmb deleted the kapilmb/fix-dsc-rule branch June 14, 2017 21:50
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.

Update PSDSCUseIdenticalMandatoryParametersForDSC rule
4 participants