Review PR #1

corydwood · 2016-08-03T16:24:49Z

Review for issue 101 to add this module to the DscResources repository.

This change is

Fixed duplicate configuration name under Examples.

Fixed typo for $newParams (was $setParams) in Set-TargetResource.

Fixed typo in "Unblock inheritance on OU" example.

kwirkykat · 2016-08-11T20:05:43Z

Reviewed 1 of 26 files at r1.
Review status: 1 of 26 files reviewed at latest revision, 24 unresolved discussions.

appveyor.yml, line 16 [r1] (raw file):

  - cd DscResource.Tests
  - ps: Import-Module .\TestHelper.psm1 -force
  - ps: Pop-Location

We have updated the appveyor file throughout the Resource Kit and in the template since this was written.
Please update this to use PSGallery instead of Chocolately like so:

install:
    - git clone https://github.com/PowerShell/DscResource.Tests
    - ps: |
        Import-Module -Name .\DscResource.Tests\TestHelper.psm1 -Force
        Install-PackageProvider -Name NuGet -MinimumVersion 2.8.5.201 -Force
        Install-Module -Name Pester -Repository PSGallery -Force

GroupPolicyDsc.psd1, line 15 [r1] (raw file):

# Version number of this module.
ModuleVersion = '1.0'

ModuleVersion = '1.0.0.0'

GroupPolicyDsc.psd1, line 21 [r1] (raw file):

# Author of this module
Author = 'Cory Wood'

Author = 'Microsoft Corporation'

GroupPolicyDsc.psd1, line 24 [r1] (raw file):

# Company or vendor of this module
CompanyName = 'Netgain Technology'

CompanyName = 'Microsoft Corporation'

GroupPolicyDsc.psd1, line 27 [r1] (raw file):

# Copyright statement for this module
Copyright = '(c) 2016 Cory Wood. All rights reserved.'

README.md, line 6 [r1] (raw file):

The **GroupPolicyDsc** module contains DSC resources for configuring Group Policy.

Please add snippet about the MS open source code of conduct:

This project has adopted the [Microsoft Open Source Code of Conduct](https://opensource.microsoft.com/codeofconduct/).
For more information see the [Code of Conduct FAQ](https://opensource.microsoft.com/codeofconduct/faq/) or contact [[email protected]](mailto:[email protected]) with any additional questions or comments.

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.psm1, line 1 [r1] (raw file):

function Get-TargetResource

Please modify this file to follow the DSC Resource Kit style guidelines

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.psm1, line 20 [r1] (raw file):

        Ensure = $null
    }
    $params = @{Target = $Target}

$params: Please use a more descriptive variable name such as $getGPInheritanceParams

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.psm1, line 61 [r1] (raw file):

    }
    $Target = Test-TargetDN @PSBoundParameters
    $params = @{

$params: Please use a more descriptive variable name

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.psm1, line 116 [r1] (raw file):

        [string]$Ensure = 'Present'
    )
    $params = @{}

$params: Please use a more descriptive variable name

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.schema.mof, line 7 [r1] (raw file):

  [write] String Domain;
  [write] String Server;
  [write,ValueMap{"Present", "Absent"},Values{"Present", "Absent"}] string Ensure;

Please replace tab characters with 4 spaces.

DSCResources/MSFT_GPOImport/MSFT_GPOImport.psm1, line 1 [r1] (raw file):

function Get-TargetResource

Please modify this file to follow the DSC Resource Kit style guidelines

DSCResources/MSFT_GPOImport/MSFT_GPOImport.psm1, line 21 [r1] (raw file):

    )
    Import-Module GroupPolicy -Verbose:$false
    $params = @{

$params: Please use a more descriptive variable name

DSCResources/MSFT_GPOImport/MSFT_GPOImport.psm1, line 68 [r1] (raw file):

    )
    Import-Module GroupPolicy -Verbose:$false
    $params = @{

$params: Please use a more descriptive variable name

DSCResources/MSFT_GPOImport/MSFT_GPOImport.schema.mof, line 11 [r1] (raw file):

  [write] String MigrationTable;
  [write] String Server;
  [write,ValueMap{"Present", "Absent"},Values{"Present", "Absent"}] string Ensure;

Please replace tab characters with 4 spaces.

DSCResources/MSFT_GPOLink/MSFT_GPOLink.psm1, line 1 [r1] (raw file):

function Get-TargetResource {

Please modify this file to follow the DSC Resource Kit style guidelines

DSCResources/MSFT_GPOLink/MSFT_GPOLink.psm1, line 220 [r1] (raw file):

        [string]$Ensure = 'Present'
    )
    $params = @{}

$params: Please use a more descriptive variable name

DSCResources/MSFT_GPOLink/MSFT_GPOLink.schema.mof, line 12 [r1] (raw file):

  [Write] Sint16 Order;
  [Write] String Server;
  [Write,ValueMap{"Present", "Absent"},Values{"Present", "Absent"}] String Ensure;

Please replace tab characters with 4 spaces.

Tests/Integration/MSFT_GPInheritance.config.ps1, line 16 [r1] (raw file):

       }
    }
}

Missing a newline at end of file

Tests/Integration/MSFT_GPInheritance.Integration.tests.ps1, line 23 [r1] (raw file):

#endregion

# TODO: Other Init Code Goes Here...

Please remove all comments left over from the template

Tests/Integration/MSFT_GPOImport.config.ps1, line 25 [r1] (raw file):

       }
    }
}

Missing a newline at end of file

Tests/Integration/MSFT_GPOImport.Integration.tests.ps1, line 23 [r1] (raw file):

#endregion

# TODO: Other Init Code Goes Here...

Please remove all comments left over from the template

Tests/Integration/MSFT_GPOLink.config.ps1, line 23 [r1] (raw file):

       }
    }
}

Missing a newline at end of file

Tests/Integration/MSFT_GPOLink.Integration.tests.ps1, line 23 [r1] (raw file):

#endregion

# TODO: Other Init Code Goes Here...

Please remove all comments left over from the template

Comments from Reviewable

corydwood · 2016-08-15T14:39:15Z

@kwirkykat I'll start working on these. Do I need to add comment-based help to each of the Get-, Set-, and Test-TargetResource functions?

corydwood · 2016-08-16T14:34:50Z

@kwirkykat The requested changes have been made. If I need to add the comment-based help for the DSC functions, I can.

TravisEz13 · 2016-08-24T01:15:09Z

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.schema.mof

+[ClassVersion("1.0.0"), FriendlyName("GPInheritance")]
+class MSFT_GPInheritance :  OMI_BaseResource
+{
+    [Key] String Target;


Can you add descriptions

TravisEz13 · 2016-08-24T01:48:12Z

Reviewed 5 of 26 files at r1.
Review status: 1 of 27 files reviewed at latest revision, 31 unresolved discussions.

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.schema.mof, line 4 [r1] (raw file):

class MSFT_GPInheritance :  OMI_BaseResource
{
  [Key] String Target;

Target isn't very descriptive can way say TargetOU?

DSCResources/MSFT_GPOImport/MSFT_GPOImport.schema.mof, line 4 [r1] (raw file):

class MSFT_GPOImport :  OMI_BaseResource
{
  [Key] String TargetName;

Target isn't standard naming in DSC... In the Example here they just use Name.

DSCResources/MSFT_GPOImport/MSFT_GPOImport.schema.mof, line 9 [r1] (raw file):

  [write,ValueMap{"Name", "Guid"},Values{"Name", "Guid"}] string BackupIdentityType;
  [write] String Domain;
  [write] String MigrationTable;

Description Specifies the path to a migration table file. You can use a migration table to map security principals and UNC paths across domains. Please add the description to the schema.

DSCResources/MSFT_GPOImport/MSFT_GPOImport.schema.mof, line 10 [r1] (raw file):

  [write] String Domain;
  [write] String MigrationTable;
  [write] String Server;

Should this be required to run on the AD machine? Most resources should target the local machine.

DSCResources/MSFT_GPOLink/MSFT_GPOLink.schema.mof, line 5 [r1] (raw file):

{
  [Key] String Identity;
  [Write,ValueMap{"Name", "Guid"},Values{"Name", "Guid"}] String IdentityType;

Allowing this allows specifying the link twice, we need the key having one type.

DSCResources/MSFT_GPOLink/MSFT_GPOLink.schema.mof, line 6 [r1] (raw file):

  [Key] String Identity;
  [Write,ValueMap{"Name", "Guid"},Values{"Name", "Guid"}] String IdentityType;
  [Key] String Target;

see previous comment

Comments from Reviewable

corydwood · 2016-08-24T15:21:26Z

Review status: 1 of 27 files reviewed at latest revision, 31 unresolved discussions.

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.schema.mof, line 4 [r1] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

Target isn't very descriptive can way say TargetOU?

I can, but I used Target to be consistent with the Target parameter of Microsoft's **Set-GPInheritance** cmdlet.

DSCResources/MSFT_GPOImport/MSFT_GPOImport.schema.mof, line 4 [r1] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

Target isn't standard naming in DSC... In the Example here they just use Name.

I can, but I used TargetName to be consistent with the TargetName parameter of Microsoft's **Import-GPO** cmdlet.

DSCResources/MSFT_GPOImport/MSFT_GPOImport.schema.mof, line 10 [r1] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

Should this be required to run on the AD machine? Most resources should target the local machine.

I made it optional to be consistent with Microsoft's **Import-GPO** cmdlet, which states, "If you do not specify the name by using the Server parameter, the PDC emulator is contacted."

DSCResources/MSFT_GPOLink/MSFT_GPOLink.schema.mof, line 5 [r1] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

Allowing this allows specifying the link twice, we need the key having one type.

This was done in order to have the ability to link the same GPO to multiple OUs, which is a valid use-case. However, this will not allow you to link a GPO to the same OU multiple times.

Comments from Reviewable

corydwood · 2016-08-24T15:22:30Z

@TravisEz13 See comments above and let me know what you'd prefer.

corydwood · 2016-08-24T15:28:53Z

Review status: 1 of 27 files reviewed at latest revision, 31 unresolved discussions.

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.schema.mof, line 4 [r2] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

Can you add descriptions

I can. Should the descriptions be moved from the **README.md** file into each individual schema.mof file to avoid duplication of information?

Comments from Reviewable

TravisEz13 · 2016-08-24T19:49:01Z

Review status: 1 of 27 files reviewed at latest revision, 31 unresolved discussions.

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.schema.mof, line 4 [r1] (raw file):

Previously, corydwood (Cory Wood) wrote…

I can, but I used Target to be consistent with the Target parameter of Microsoft's Set-GPInheritance cmdlet.

Consistency is good if it leads to easy understanding. I'm also not a fan of Target, but I don't have a better suggestion in this case.

DSCResources/MSFT_GPInheritance/MSFT_GPInheritance.schema.mof, line 4 [r2] (raw file):

Previously, corydwood (Cory Wood) wrote…

I can. Should the descriptions be moved from the README.md file into each individual schema.mof file to avoid duplication of information?

Currently, they should be in both places. We should have a way of generating the markdown snippet based on the schema, or a resource class, but I don't think this exists. The MOF is what provides runtime help.

DSCResources/MSFT_GPOImport/MSFT_GPOImport.schema.mof, line 4 [r1] (raw file):

Previously, corydwood (Cory Wood) wrote…

I can, but I used TargetName to be consistent with the TargetName parameter of Microsoft's Import-GPO cmdlet.

I don't find the parameters to be consistent among the GPO CMDlets. I would prefer to be more consistent here. Also, target is imperative, we should try to avoid it.

DSCResources/MSFT_GPOImport/MSFT_GPOImport.schema.mof, line 10 [r1] (raw file):

Previously, corydwood (Cory Wood) wrote…

I made it optional to be consistent with Microsoft's Import-GPO cmdlet, which states, "If you do not specify the name by using the Server parameter, the PDC emulator is contacted."

Ok, I think this is fine.

DSCResources/MSFT_GPOLink/MSFT_GPOLink.schema.mof, line 5 [r1] (raw file):

Previously, corydwood (Cory Wood) wrote…

This was done in order to have the ability to link the same GPO to multiple OUs, which is a valid use-case. However, this will not allow you to link a GPO to the same OU multiple times.

I didn't say that you could actually link them multiple times. This design could allow you to specify a link for a GPO multiple times. This issue leads to convergence issues. I'll give an example. Say you have a GPO named MyGPO, with an GUID of `e9958209-527d-4b46-8874-ed1e43b1f567`.

GPOLink l1
{
    Identity = 'MyGPO'
    Target = 'MyTarget'
    LinkEnabled = 'Yes'
}
GPOLink l2
{
    Identity = 'e9958209-527d-4b46-8874-ed1e43b1f567'
    Target = 'MyTarget'
    LinkEnabled = 'No'
}

Now you have allowed the user to specify a configuration that will never converge. The same link is enabled then disabled in the same configuration, every time you run the configuration.

Comments from Reviewable

corydwood · 2016-08-25T16:15:07Z

Review status: 1 of 27 files reviewed at latest revision, 30 unresolved discussions.

DSCResources/MSFT_GPOLink/MSFT_GPOLink.schema.mof, line 5 [r1] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

I didn't say that you could actually link them multiple times. This design could allow you to specify a link for a GPO multiple times. This issue leads to convergence issues. I'll give an example.
Say you have a GPO named MyGPO, with an GUID of e9958209-527d-4b46-8874-ed1e43b1f567.
GPOLink l1
{
    Identity = 'MyGPO'
    Target = 'MyTarget'
    LinkEnabled = 'Yes'
}
GPOLink l2
{
    Identity = 'e9958209-527d-4b46-8874-ed1e43b1f567'
    Target = 'MyTarget'
    LinkEnabled = 'No'
}
Now you have allowed the user to specify a configuration that will never converge. The same link is enabled then disabled in the same configuration, every time you run the configuration.

I see. Is your recommendation to replace the Identity property with either Name or Guid (I'd go with Name) and remove the IdentityType property? I'm ok with that. A separate resource to link by Guid could be created if needed.

Comments from Reviewable

TravisEz13 · 2016-08-25T18:44:31Z

Yes, I'd recommend removing IdentityType. I think GUID is better long term, but Name is better for usability. I don't have a strong preference.

corydwood · 2016-08-25T19:39:50Z

@TravisEz13 I've made the requested changes.

corydwood · 2016-09-18T23:02:13Z

@TravisEz13 The changes were made. Are any other changes required?

kwirkykat · 2016-09-19T23:29:09Z

@corydwood @TravisEz13 is out on vacation right now. I'll let him know this is waiting when he gets back.

TravisEz13 · 2016-09-26T20:11:46Z

Reviewed 3 of 8 files at r3.
Review status: 4 of 27 files reviewed at latest revision, 29 unresolved discussions.

DSCResources/MSFT_GPOLink/MSFT_GPOLink.schema.mof, line 12 at r1 (raw file):

Previously, kwirkykat (Katie Keim) wrote…

Please replace tab characters with 4 spaces.

Please replace tab characters with 4 spaces.

Comments from Reviewable

corydwood · 2016-09-26T20:24:53Z

@TravisEz13 I think you're looking at an old commit. The tab characters were already replaced with 4 spaces.

corydwood · 2016-11-14T17:28:11Z

@TravisEz13 @kwirkykat Is there anything else either of you are waiting on before this can be transferred?

kwirkykat · 2017-01-20T00:07:30Z

@corydwood No changes needed at the moment. Holdup is on our end.

dcrreynolds · 2018-03-01T16:10:20Z

Hi everyone, what ever happened to this PR?

johlju · 2018-04-16T13:09:05Z

@corydwood Please see issue comment PowerShell/DscResources#101 (comment).

corydwood and others added 10 commits February 23, 2016 11:21

Created xGroupPolicy Module.

095e685

Update README.md

c5d934e

Fixed duplicate configuration name under Examples.

Update MSFT_xGPOLink.psm1

a857081

Fixed typo for $newParams (was $setParams) in Set-TargetResource.

Update README.md

88c9c1e

Fixed typo in "Unblock inheritance on OU" example.

Create LICENSE.md

ffa153a

Start of review

95613df

Merge branch 'master' into reviewTarget

bbb377d

Removed x from in front of resources.

e0f6b25

Removed x from in front of resources.

00a7873

Merge branch 'master' into reviewTarget

d02430a

corydwood mentioned this pull request Aug 3, 2016

New resource module: GroupPolicy PowerShell/DscResources#101

Open

Made requested changes.

cbf186b

TravisEz13 reviewed Aug 24, 2016
View reviewed changes

corydwood added 2 commits August 25, 2016 14:33

Made changes requested by TravisEz13.

21b5edd

Updated Helper.psm1 to reflect changes to parameter names.

70c9d74

corydwood added 2 commits August 25, 2016 14:59

Added $Name parameter to Helper.psm1

7d744de

Updated tests with new parameter names.

9366213

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review PR #1

Review PR #1

corydwood commented Aug 3, 2016 •

edited

Loading

kwirkykat commented Aug 11, 2016

corydwood commented Aug 15, 2016

corydwood commented Aug 16, 2016

TravisEz13 Aug 24, 2016

TravisEz13 commented Aug 24, 2016

corydwood commented Aug 24, 2016

corydwood commented Aug 24, 2016

corydwood commented Aug 24, 2016

TravisEz13 commented Aug 24, 2016

corydwood commented Aug 25, 2016

TravisEz13 commented Aug 25, 2016

corydwood commented Aug 25, 2016

corydwood commented Sep 18, 2016

kwirkykat commented Sep 19, 2016 •

edited

Loading

TravisEz13 commented Sep 26, 2016

corydwood commented Sep 26, 2016

corydwood commented Nov 14, 2016

kwirkykat commented Jan 20, 2017

dcrreynolds commented Mar 1, 2018

johlju commented Apr 16, 2018

Review PR #1

Are you sure you want to change the base?

Review PR #1

Conversation

corydwood commented Aug 3, 2016 • edited Loading

kwirkykat commented Aug 11, 2016

corydwood commented Aug 15, 2016

corydwood commented Aug 16, 2016

TravisEz13 Aug 24, 2016

Choose a reason for hiding this comment

TravisEz13 commented Aug 24, 2016

corydwood commented Aug 24, 2016

corydwood commented Aug 24, 2016

corydwood commented Aug 24, 2016

TravisEz13 commented Aug 24, 2016

corydwood commented Aug 25, 2016

TravisEz13 commented Aug 25, 2016

corydwood commented Aug 25, 2016

corydwood commented Sep 18, 2016

kwirkykat commented Sep 19, 2016 • edited Loading

TravisEz13 commented Sep 26, 2016

corydwood commented Sep 26, 2016

corydwood commented Nov 14, 2016

kwirkykat commented Jan 20, 2017

dcrreynolds commented Mar 1, 2018

johlju commented Apr 16, 2018

corydwood commented Aug 3, 2016 •

edited

Loading

kwirkykat commented Sep 19, 2016 •

edited

Loading