PR for module Review

PR for issue PowerShell/DscResources#86

This change is

Reviewed 6 of 21 files at r1.
Review status: 6 of 21 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

AuditPolicyDsc.psd1, line 101 [r1] (raw file):

        # A URL to the main website for this project.
        ProjectUri = 'https://github.com/PowerShell/AuditPolicyDsc'

license uri doesn't match

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

### Unreleased

### <IP_ADDRESS>

Should this be in unreleased?

Comments from Reviewable

Review status: 6 of 21 files reviewed at latest revision, 1 unresolved discussion.

README.md, line 32 at r1 (raw file):
Previously, TravisEz13 (Travis Plunk) wrote…
> Should this be in unreleased?

This is how it should look:
## Versions

### Unreleased

### <IP_ADDRESS>

* Initial release with the following resources:

  * AuditCategory
  * AuditOption

Comments from Reviewable

Can you respond to the comments on Reviewable as you fix them so that the reviewer knows that you've made those changes?

Review status: 6 of 21 files reviewed at latest revision, 2 unresolved discussions.

AuditPolicyDsc.psd1, line 101 at r1 (raw file):
Previously, TravisEz13 (Travis Plunk) wrote…
> license uri doesn't match

Done.

README.md, line 32 at r1 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> This is how it should look:
>
> ```powershell
> ## Versions
>
> ### Unreleased
>
> ### <IP_ADDRESS>
>
> * Initial release with the following resources:
>
>   * AuditCategory
>   * AuditOption
> ```

I just reverted this back to the correct format.

Comments from Reviewable

Review status: 6 of 21 files reviewed at latest revision, 1 unresolved discussion.

README.md, line 32 at r1 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> I just reverted this back to the correct format.

Done.

Comments from Reviewable

@athaynes Just from looking at the documentation here, I think AuditCategory should be renamed to AuditPolicySubcategory since it modifies the settings of a 'subcategory' rather than a 'category'.
In addition, I think it makes more sense for the resources to be named AuditPolicyOption and AuditPolicySubcategory since these are resources to modify the 'audit policy' of a machine rather than an 'audit'.
@travisez13 @mbreakey3 @zjalexander What do you think?

Reviewed 2 of 21 files at r1.
Review status: 7 of 21 files reviewed at latest revision, 29 unresolved discussions.

appveyor.yml, line 18 at r2 (raw file):

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

We recently updated the environment configuration section of our appveyor.yml templates across the DSC Resource Kit.
Can you please update this section to match?
#---------------------------------#
#      environment configuration  #
#---------------------------------#
version: 1.0.{build}.0
install:
    - git clone https://github.com/PowerShell/DscResource.Tests
    - ps: |
        Import-Module -Name .\DscResource.Tests\TestHelper.psm1 -Force
        Install-PackageProvider -Name NuGet -MinimumVersion <IP_ADDRESS> -Force
        Install-Module -Name Pester -Repository PSGallery -Force

appveyor.yml, line 4 at r4 (raw file):

# This is template for appveyor.yml
# Before using it in your repository, modify places marked with # TODO Modify
################################################################################

Please remove the template comments

AuditPolicyDsc.psd1, line 107 at r2 (raw file):

# ReleaseNotes = ''

The release notes here should match those in the most recent version release notes in the README.
Like this:
ReleaseNotes =  '* Initial release with the following resources:
  * AuditCategory
  * AuditOption'

---

*[AuditPolicyDsc.psd1, line 33 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYtFFc0x5DT6iS_x6R:-KUYtFFc0x5DT6iS_x6S:b-kr420m) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/AuditPolicyDsc.psd1#L33)):*
> PowerShellVersion = '3.0'

Should be 4.0

---

*[README.md, line 6 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYu67xox5PTpga3Xo5:-KUYu67xox5PTpga3Xo6:bsn7hk) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/README.md#L6)):*
> ```Markdown
>
> The **AuditPolicyDsc** module allows you to configure and manage the advanced audit policy on all currently supported versions of Windows.
> ```

Please add this snippet about the Microsoft open source code of conduct here:

This project has adopted the Microsoft Open Source Code of Conduct.
For more information see the Code of Conduct FAQ or contact<EMAIL_ADDRESS>with any additional questions or comments.

---

*[README.md, line 8 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYw01wL6TwKeo8l--I:-KUYw01xG4XfUk9M6JvJ:b5xuxdy) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/README.md#L8)):*
> https://github.com/PowerShell/DscResource.Kit/blob/master/CONTRIBUTING.md

Should be https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md

---

*[README.md, line 12 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYueJjkBLyrpDUm408:-KUYueJjkBLyrpDUm409:b-7sh1bt) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/README.md#L12)):*
> AuditPolicy

Is this supposed to be AuditCategory?

---

*[README.md, line 26 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYuo1NSytPN_pC1bAP:-KUYuo1NSytPN_pC1bAQ:b-s450mj) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/README.md#L26)):*
> Vaule

Value

---

*[README.md, line 47 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYv4pVfASE2iGrRgTz:-KUYv4pVfASE2iGrRgU-:b-ne2thc) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/README.md#L47)):*
> AuditPolicy

There isn't a resource named AuditPolicy in this module...?

---

*[DSCResources/Helper.psm1, line 2 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYzcrnOf_chgjPEsuU:-KUYzcrnOf_chgjPEsuV:b66f7f2) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/Helper.psm1#L2)):*
> ```PowerShell
> #Requires -Version 4.0
> ```

Why is all the functionality of two resources combined here instead of included in separate files for each resource?
Does the resource functionality overlap?

Ideally all the functions relating to AuditOption should be included in MSFT_AuditOption.psm1, and all the functions relating to AuditCategory should be included in MSFT_AuditCategory.psm1

Also, why separate out the 'private' functions?

---

*[DSCResources/Helper.psm1, line 34 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYzOgsWGkKOKbaVJZ9:-KUYzOgsWGkKOKbaVJZA:bu09sbe) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/Helper.psm1#L34)):*
> ```PowerShell
>     Invoke_AuditPol -Command 'Get' -SubCommand 'Subcategory:Logon'
> #>
> function Invoke_AuditPol
> ```

All PowerShell functions must be named in the Verb-Noun format.
Specifying private functions with the '_' isn't

---

*[DSCResources/Helper.psm1, line 118 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYz8r5Yv0aEGEFcC67:-KUYz8r5Yv0aEGEFcC68:btiz5sf) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/Helper.psm1#L118)):*
> ```PowerShell
>     The third string of the auditpol results
> #>
> function Get_AuditCategory
> ```

Is there a reason this function is separate?
This function should be combined with Get-AuditCategory.

---

*[DSCResources/Helper.psm1, line 193 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYzp1tinVpeJ_p-_Cy:-KUYzp1uHDN37x-rAwyl:bngs3lk) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/Helper.psm1#L193)):*
> ```PowerShell
>     A string that is further processed by the public version of this function.
> #>
> function Get_AuditOption
> ```

Is there a reason this function is separate?
This function should be combined with Get-AuditOption.

---

*[DSCResources/en-US/helper.psd1, line 21 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYyqu3W9I-GUrEUzkH:-KUYyqu3W9I-GUrEUzkI:bz14h4p) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/en-US/helper.psd1#L21)):*
> ```PowerShell
>         TestAuditpolSubcategoryCorrect      = '{0}':'{1}' is '{2}'
>         TestAuditpolSubcategoryIncorrect    = '{0}':'{1}' is NOT '{2}'
>         GetAuditpolResourceSACLSucceed      =
> ```

If these blank string properties are not used, please remove them.

If they are used, please provide an appropriate string.

---

*[DSCResources/MSFT_AuditCategory/MSFT_AuditCategory.psm1, line 2 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYyo9f18g5fCTc0kf9:-KUYyo9f18g5fCTc0kfA:b-mvs25b) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/MSFT_AuditCategory/MSFT_AuditCategory.psm1#L2)):*
> -Verbose:0

Please remove the hard-coded Verbose value here

---

*[DSCResources/MSFT_AuditCategory/MSFT_AuditCategory.psm1, line 4 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYykfyXEVnVtbBgSmQ:-KUYykfyXEVnVtbBgSmR:b4mp68f) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/MSFT_AuditCategory/MSFT_AuditCategory.psm1#L4)):*
> ```PowerShell
> Import-Module $PSScriptRoot\..\Helper.psm1 -Verbose:0
>
> function Get-TargetResource
> ```

Please add help comments to all functions in this file

---

*[DSCResources/MSFT_AuditCategory/ResourceDesignerScript/Generate_AuditCategory.ps1, line 1 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYyYbnj5X8rjcZbU4M:-KUYyYbnj5X8rjcZbU4N:b-qsogk0) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/MSFT_AuditCategory/ResourceDesignerScript/Generate_AuditCategory.ps1#L1)):*
> ```PowerShell
>
> ```

This file is not needed.
Please remove it.

---

*[DSCResources/MSFT_AuditOption/MSFT_AuditOption.psm1, line 2 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYyEkDT3TuNG0-K68O:-KUYyEkDT3TuNG0-K68P:b-mvs25b) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/MSFT_AuditOption/MSFT_AuditOption.psm1#L2)):*
> -Verbose:0

Please remove the hard-coded Verbose value here

---

*[DSCResources/MSFT_AuditOption/MSFT_AuditOption.psm1, line 4 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYyKS5ZDk2ewHFgRG3:-KUYyKS5ZDk2ewHFgRG4:b4mp68f) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/MSFT_AuditOption/MSFT_AuditOption.psm1#L4)):*
> ```PowerShell
> Import-Module $PSScriptRoot\..\Helper.psm1 -Verbose:0
>
> function Get-TargetResource
> ```

Please add help comments to all functions in this file

---

*[DSCResources/MSFT_AuditOption/ResourceDesignerScript/Generate_AuditOption.ps1, line 1 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYxuE02-ap-6AyZzTf:-KUYxuE02-ap-6AyZzTg:b-qsogk0) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/DSCResources/MSFT_AuditOption/ResourceDesignerScript/Generate_AuditOption.ps1#L1)):*
> ```PowerShell
>
> ```

This file is not needed.
Please remove it.

---

*[Examples/Example_AuditPolicyDsc.ps1, line 1 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYxiGax3CMlONb7LqC:-KUYxiGax3CMlONb7LqD:bka79y3) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/Examples/Example_AuditPolicyDsc.ps1#L1)):*
> ```PowerShell
>
> ```

Can you please include a comment here that explains what this example does and how to run it?

---

*[Tests/Integration/MSFT_AuditCategory.config.ps1, line 10 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYxYmEdlh0Vnqyl-jq:-KUYxYmEdlh0Vnqyl-jr:bwreh6m) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/Tests/Integration/MSFT_AuditCategory.config.ps1#L10)):*
> ```PowerShell
>
> .NOTES
> #>
> ```

Please remove all template comments.

---

*[Tests/Integration/MSFT_AuditCategory.Integration.tests.ps1, line 13 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYxXb_8WyshNHMKyKp:-KUYxXb_8WyshNHMKyKq:bwreh6m) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/Tests/Integration/MSFT_AuditCategory.Integration.tests.ps1#L13)):*
> ```PowerShell
>    Code in HEADER, FOOTER and DEFAULT TEST regions are standard and may be moved into
>    DSCResource.Tools in Future and therefore should not be altered if possible.
> #>
> ```

Please remove all template comments.

---

*[Tests/Integration/MSFT_AuditCategory.Integration.tests.ps1, line 16 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUZ-gXw8RTIYb_MTUZi:-KUZ-gXxo6PibsiLGEg6:b-vuqd9d) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/Tests/Integration/MSFT_AuditCategory.Integration.tests.ps1#L16)):*
> ```PowerShell
>
> $Global:DSCModuleName      = 'AuditPolicyDsc'
> $Global:DSCResourceName    = 'MSFT_AuditCategory'
> ```

I know these global variables were in the old test template, but these globals are no longer allowed for high quality modules in the DSC Resource Kit. Please replace these, and the ones in the other test files as well. I believe our new test template can help you to remove these if you are confused.

You could just change them to script variables or hard-code the values in this test.

---

*[Tests/Integration/MSFT_AuditOption.config.ps1, line 10 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYwaphfa0Ifd1n2Lgl:-KUYwaphfa0Ifd1n2Lgm:bwreh6m) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/Tests/Integration/MSFT_AuditOption.config.ps1#L10)):*
> ```PowerShell
>
> .NOTES
> #>
> ```

Please remove all template comments.

---

*[Tests/Integration/MSFT_AuditOption.Integration.tests.ps1, line 14 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYw_UwRCpS6IPWZ-mz:-KUYw_UwRCpS6IPWZ-n-:bwreh6m) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/Tests/Integration/MSFT_AuditOption.Integration.tests.ps1#L14)):*
> ```PowerShell
>    Code in HEADER, FOOTER and DEFAULT TEST regions are standard and may be moved into
>    DSCResource.Tools in Future and therefore should not be altered if possible.
> #>
> ```

Please remove all template comments.

---

*[Tests/Unit/MSFT_AuditCategory.tests.ps1, line 12 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYwVklj5vd-3hykjke:-KUYwVklj5vd-3hykjkf:bwreh6m) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/Tests/Unit/MSFT_AuditCategory.tests.ps1#L12)):*
> ```PowerShell
>    Code in HEADER and FOOTER regions are standard and may be moved into DSCResource.Tools in
>    Future and therefore should not be altered if possible.
> #>
> ```

Please remove all template comments.

---

*[Tests/Unit/MSFT_AuditOption.tests.ps1, line 12 at r4](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-KUYwGEHJxIThhcRZe1D:-KUYwGEHJxIThhcRZe1E:bwreh6m) ([raw file](https://github.com/athaynes/auditpolicydsc/blob/99c251e12968a0c7da167ffa2ef2fdb871f3b94a/Tests/Unit/MSFT_AuditOption.tests.ps1#L12)):*
> ```PowerShell
>    Code in HEADER and FOOTER regions are standard and may be moved into DSCResource.Tools in
>    Future and therefore should not be altered if possible.
> #>
> ```

Please remove all template comments.

---

*Comments from [Reviewable](https://reviewable.io:443/reviews/athaynes/auditpolicydsc/24#-:-KUYvV9xLgm4sdp-Rruw:b-3w4cos)*
<!-- Sent from Reviewable.io -->

@kwirkykat - i don't have a strong opinion on the specific questions you asked, but here's some more data:

I think AuditCategory should be renamed to AuditPolicySubcategory since it modifies the settings of a 'subcategory' rather than a 'category'.

The way these settings appear in the UI and in the CSV is under "subcategory" so that is consistent at least.

In addition, I think it makes more sense for the resources to be named AuditPolicyOption and AuditPolicySubcategory

This matches the pattern we established for GPRegistryPolicyResource. If we go with "policy" lets make a note to enforce the same on the SecEdit since I do expect usage of all 3 to go hand-in-hand.

Review status: 7 of 21 files reviewed at latest revision, 29 unresolved discussions.

appveyor.yml, line 18 at r2 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> We recently updated the environment configuration section of our appveyor.yml templates across the DSC Resource Kit.
> Can you please update this section to match?
>
> ```
> #---------------------------------#
> #      environment configuration  #
> #---------------------------------#
> version: 1.0.{build}.0
> install:
>     - git clone https://github.com/PowerShell/DscResource.Tests
>     - ps: |
>         Import-Module -Name .\DscResource.Tests\TestHelper.psm1 -Force
>         Install-PackageProvider -Name NuGet -MinimumVersion <IP_ADDRESS> -Force
>         Install-Module -Name Pester -Repository PSGallery -Force
> ```

Done.

appveyor.yml, line 4 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please remove the template comments

Done.

AuditPolicyDsc.psd1, line 107 at r2 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > \# ReleaseNotes = ''
>
> The release notes here should match those in the most recent version release notes in the README.
> Like this:
>
> ```
> ReleaseNotes =  '* Initial release with the following resources:
>   * AuditCategory
>   * AuditOption'

Done and I used the new module names as well.

AuditPolicyDsc.psd1, line 33 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > PowerShellVersion = '3.0'
>
> Should be 4.0

Done.

README.md, line 6 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please add this snippet about the Microsoft open source code of conduct here:
>
> ```
> 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 [opencode<EMAIL_ADDRESS>with any additional questions or comments.
> ```

Done.

README.md, line 8 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > https://github.com/PowerShell/DscResource.Kit/blob/master/CONTRIBUTING.md
>
> Should be https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md

Done.

README.md, line 12 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > AuditPolicy
>
> Is this supposed to be AuditCategory?

Done, yes that was a mistake and has been updated to the new module name.

README.md, line 26 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > Vaule
>
> Value

Doh! Done.

README.md, line 47 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > AuditPolicy
>
> There isn't a resource named AuditPolicy in this module...?

Done. I updated to use the new module name.

DSCResources/Helper.psm1, line 2 at r4 (raw file):

Why is all the functionality of two resources combined here instead of included in separate files for each resource?
Does the resource functionality overlap?

The resource functionality does not overlap, but everything has to be sent to auditpol.exe for processing. The helper module centralizes all of that work.

Ideally all the functions relating to AuditOption should be included in MSFT_AuditOption.psm1, and all the functions relating to AuditCategory should be included in MSFT_AuditCategory.psm1
Also, why separate out the 'private' functions?

The intent here was to future proof the resource, meaning that I assumed that auditpol.exe would be replaced by cmdlets in Server 2016 and I would use the new cmdlets in the public functions, but still be able to use auditpol for backwards compatibility.
For example Get-AuditCategory would call the audit policy cmdlet directly if it found it and if not fall back to auditpol.exe. Then in the distant future, when auditpol was no longer supported, all the private functions could be clipped out with no other changes needed.

DSCResources/Helper.psm1, line 118 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Is there a reason this function is separate?
> This function should be combined with Get-AuditCategory.

See comment on forward/backwards compatibility.

DSCResources/Helper.psm1, line 193 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Is there a reason this function is separate?
> This function should be combined with Get-AuditOption.

See comment on forward/backwards compatibility.

DSCResources/en-US/helper.psd1, line 21 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> If these blank string properties are not used, please remove them.
>
> If they are used, please provide an appropriate string.

Done.

DSCResources/MSFT_AuditCategory/MSFT_AuditCategory.psm1, line 2 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > -Verbose:0
>
> Please remove the hard-coded Verbose value here

Done.

DSCResources/MSFT_AuditCategory/ResourceDesignerScript/Generate_AuditCategory.ps1, line 1 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> This file is not needed.
> Please remove it.

Done.

DSCResources/MSFT_AuditOption/MSFT_AuditOption.psm1, line 2 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > -Verbose:0
>
> Please remove the hard-coded Verbose value here

Done.

DSCResources/MSFT_AuditOption/ResourceDesignerScript/Generate_AuditOption.ps1, line 1 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> This file is not needed.
> Please remove it.

Done.

Tests/Integration/MSFT_AuditCategory.config.ps1, line 10 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please remove all template comments.

Done.

Tests/Integration/MSFT_AuditCategory.Integration.tests.ps1, line 13 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please remove all template comments.

Done.

Tests/Integration/MSFT_AuditOption.config.ps1, line 10 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please remove all template comments.

Done.

Tests/Integration/MSFT_AuditOption.Integration.tests.ps1, line 14 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please remove all template comments.

Done.

Tests/Unit/MSFT_AuditCategory.tests.ps1, line 12 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please remove all template comments.

Done.

Tests/Unit/MSFT_AuditOption.tests.ps1, line 12 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please remove all template comments.

Done.

Comments from Reviewable

@zjalexander @kwirkykat 100% agree on the module names and have updated them to be consistent across the security tools. I have a few more open items that I will get finished up when I get home tonight.

Review status: 7 of 21 files reviewed at latest revision, 29 unresolved discussions.

DSCResources/Helper.psm1, line 34 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> All PowerShell functions must be named in the Verb-Noun format.
> Specifying private functions with the '_' isn't

Done.

DSCResources/MSFT_AuditCategory/MSFT_AuditCategory.psm1, line 4 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please add help comments to all functions in this file

Done.

DSCResources/MSFT_AuditOption/MSFT_AuditOption.psm1, line 4 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please add help comments to all functions in this file

Done.

Examples/Example_AuditPolicyDsc.ps1, line 1 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Can you please include a comment here that explains what this example does and how to run it?

Done.

Tests/Integration/MSFT_AuditCategory.Integration.tests.ps1, line 16 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> I know these global variables were in the old test template, but these globals are no longer allowed for high quality modules in the DSC Resource Kit. Please replace these, and the ones in the other test files as well. I believe our new test template can help you to remove these if you are confused.
>
> You could just change them to script variables or hard-code the values in this test.

Done. I also updated the Option module as well.

Comments from Reviewable

Reviewed 7 of 27 files at r5.
Review status: 5 of 19 files reviewed at latest revision, 8 unresolved discussions.

README.md, line 6 at r4 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Done.

It looks like you missed the hyperlinks here.

DSCResources/Helper.psm1, line 2 at r4 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> > Why is all the functionality of two resources combined here instead of included in separate files for each resource?
> > Does the resource functionality overlap?
> >
> The resource functionality does not overlap, but everything has to be sent to auditpol.exe for processing. The helper module centralizes all of that work.
>
> > Ideally all the functions relating to AuditOption should be included in MSFT_AuditOption.psm1, and all the functions relating to AuditCategory should be included in MSFT_AuditCategory.psm1
> >
> > Also, why separate out the 'private' functions?
>
> The intent here was to future proof the resource, meaning that I assumed that auditpol.exe would be replaced by cmdlets in Server 2016 and I would use the new cmdlets in the public functions, but still be able to use auditpol for backwards compatibility.
>
> For example Get-AuditCategory would call the audit policy cmdlet directly if it found it and if not fall back to auditpol.exe. Then in the distant future, when auditpol was no longer supported, all the private functions could be clipped out with no other changes needed.
>

Thinking about this...

Tests/Integration/MSFT_AuditPolicyOption.config.ps1, line 11 at r6 (raw file):

        {
            Name  = $optionName
            Value = $optionValue

These parameters/variables are not defined

Tests/Integration/MSFT_AuditPolicySubcategory.config.ps1, line 12 at r6 (raw file):

            Subcategory = $Subcategory
            AuditFlag   = $AuditFlag
            Ensure      = $AuditFlagEnsure

These parameters/variables are not defined

Comments from Reviewable

Review status: 5 of 19 files reviewed at latest revision, 8 unresolved discussions.

README.md, line 6 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> It looks like you missed the hyperlinks here.

Done. Sorry about that.

DSCResources/Helper.psm1, line 2 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Thinking about this...

I am not married to it and probably over thought the whole thing since there isn't an auditpol replacement in 2016. I was just hoping to have the whole thing be native PowerShell.

Tests/Integration/MSFT_AuditPolicyOption.config.ps1, line 11 at r6 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> These parameters/variables are not defined

Those are defined in the Init section of the integration test. Let me know if you want me to move them to this file.

Tests/Integration/MSFT_AuditPolicySubcategory.config.ps1, line 12 at r6 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> These parameters/variables are not defined

Those are defined in the Init section of the integration test. Let me know if you want me to move them to this file.

Comments from Reviewable

Review status: 5 of 19 files reviewed at latest revision, 9 unresolved discussions.

a discussion (no related file):
I found a bug in how I was inputting the strings into the auditpol exe. If any subcommands had a space, it would fail, because I was splitting the string on the space. I changed Invoke-Auditpol it to accept arrays and updated how the parameters are passed to the exe.
All of the tests pass, I removed 2 tests, that were asserting mocked calls. I have learned a lot in the past few months and realized this is an implementation detail test that doesn't provide any value to the resource.

Comments from Reviewable

Reviewed 1 of 5 files at r7.
Review status: 6 of 19 files reviewed at latest revision, 7 unresolved discussions.

DSCResources/Helper.psm1, line 2 at r4 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> I am not married to it and probably over thought the whole thing since there isn't an auditpol replacement in 2016. I was just hoping to have the whole thing be native PowerShell.

I see why this module was written this way with the Helper file, but the functionality of the resources should be moved into their separate resource modules.
Having all the functionality of all the resources in one file is pretty confusing and makes for a a lot of code in one file. Plus this may cause a lot of merge conflicts if contributors try to change different resources all in one file.
Putting the functions into their resource files also eliminates the need for keeping 'private' functions separate from 'public' functions since they can all be 'private' then. (Though I understand this separation was originally done with an AuditPolicy PowerShell module in mind.)
I don't see overlapping functionality, but if there is functionality that overlaps between the resources, that can stay in a common module.

Tests/Integration/MSFT_AuditPolicyOption.config.ps1, line 11 at r6 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Those are defined in the Init section of the integration test. Let me know if you want me to move them to this file.

These variables should be defined as parameters to this configuration. The values can then be passed in from the integration tests.
See the Integration tests for any of the resources in PSDscResources for examples of how to do this.

Tests/Integration/MSFT_AuditPolicySubcategory.config.ps1, line 12 at r6 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Those are defined in the Init section of the integration test. Let me know if you want me to move them to this file.

These variables should be defined as parameters to this configuration. The values can then be passed in from the integration tests.
See the Integration tests for any of the resources in PSDscResources for examples of how to do this.

Comments from Reviewable

Review status: 6 of 19 files reviewed at latest revision, 7 unresolved discussions.

DSCResources/Helper.psm1, line 2 at r4 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> I see why this module was written this way with the Helper file, but the functionality of the resources should be moved into their separate resource modules.
>
> Having all the functionality of all the resources in one file is pretty confusing and makes for a a lot of code in one file. Plus this may cause a lot of merge conflicts if contributors try to change different resources all in one file.
>
> Putting the functions into their resource files also eliminates the need for keeping 'private' functions separate from 'public' functions since they can all be 'private' then. (Though I understand this separation was originally done with an AuditPolicy PowerShell module in mind.)
>
> I don't see overlapping functionality, but if there is functionality that overlaps between the resources, that can stay in a common module.

I will start reworking this tonight.

Tests/Integration/MSFT_AuditPolicyOption.config.ps1, line 11 at r6 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> These variables should be defined as parameters to this configuration. The values can then be passed in from the integration tests.
> See the Integration tests for any of the resources in PSDscResources for examples of how to do this.

Done.

Tests/Integration/MSFT_AuditPolicySubcategory.config.ps1, line 12 at r6 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> These variables should be defined as parameters to this configuration. The values can then be passed in from the integration tests.
> See the Integration tests for any of the resources in PSDscResources for examples of how to do this.

Done.

Comments from Reviewable

Review status: 6 of 19 files reviewed at latest revision, 7 unresolved discussions.

DSCResources/Helper.psm1, line 2 at r4 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> I will start reworking this tonight.

Done. I moved everything to the proper resource modules, but left Invoke-Auditpol in the helper module since all resources have to use it and it centralizes the verbose output of the resource. I removed the intermediate functions since the auditpol cmdlets do not exist. Less is more right :) I updated all of the unit and integration test as well to reflect the new location of the supporting functions.

Comments from Reviewable

Reviewed 2 of 29 files at r1, 7 of 27 files at r5, 1 of 5 files at r7, 2 of 6 files at r9, 1 of 2 files at r10.
Review status: 9 of 19 files reviewed at latest revision, 72 unresolved discussions.

README.md, line 20 at r10 (raw file):

### AuditPolicySubcategory

Under each of these resource descriptions could you change it to the format used in PSDscResources. So:
ResourceName
Requirements
Parameters
Read-Only Properties from Get-TargetResource
Examples

DSCResources/Helper.psm1, line 14 at r10 (raw file):

 .SYNOPSIS
    Invoke-AuditPol is a private function that wraps auditpol.exe providing a
    centralized function to mange access to and the output of auditpol.exe.

'manage'  instead of 'mange'?

DSCResources/Helper.psm1, line 21 at r10 (raw file):

    call Invoke-AuditPol.

    Since the call operators is being used to run auditpol, the input is restricted to only execute

'operator' - singular

DSCResources/Helper.psm1, line 24 at r10 (raw file):

    against auditpol.exe. Any input that is an invalid flag or parameter in
    auditpol.exe will return an error to prevent abuse of the call.
    The call operator will not parse the parameters, so they are split in the fuction.

'function.'

DSCResources/Helper.psm1, line 28 at r10 (raw file):

    The action that audtipol should take on the subcommand.
 .PARAMETER SubCommand
    The The subcommand the execute.

'The subcommand to execute.'

DSCResources/Helper.psm1, line 40 at r10 (raw file):

    param
    (
        [parameter(Mandatory = $true)]

'Parameter' - in all cases below as well.

DSCResources/Helper.psm1, line 75 at r10 (raw file):

        $return = & "auditpol.exe" $commandString 2>&1

        # auditpol does not thrown exceptions, so test the results and throw if needed

'does not throw...'

DSCResources/Helper.psm1, line 91 at r10 (raw file):

    {
        # catch the error thrown if the lastexitcode is not 0
        [string] $errorString = $error[0].Exception

'[String]'

DSCResources/en-US/helper.psd1, line 22 at r9 (raw file):

        TestAuditpolSubcategoryIncorrect    = '{0}':'{1}' is NOT '{2}'
'@

rename this file to 'AuditPolicyDsc.strings.psd1'

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 2 at r9 (raw file):

Import-Module $PSScriptRoot\..\Helper.psm1

Should use 'Join-Path' instead of '' :
Import-Module -Name (Join-Path -Path (Split-Path $PSScriptRoot -Parent) `
                               -ChildPath (Join-Path -Path 'DscResources' `
                                                     -ChildPath 'Helper.psm1')) `
                                                     -Force

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 6 at r9 (raw file):

<#
    .SYNOPSIS
    Gets the value of the audit policy option.

tab in the information bits:
<#
    .SYNOPSIS
        Gets the value of the audit policy option.
    .PARAMETER Name
        Specifies the option to get.
#>

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 24 at r9 (raw file):

    # get the option's current value
    $optionValue = Get-AuditOption @PSBoundParameters

since this is always going to pass in just $Name - I would replace '@PSBoundParameters' with '-Name $Name'

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 26 at r9 (raw file):

    $optionValue = Get-AuditOption @PSBoundParameters

    Write-Verbose ( $localizedData.GetAuditpolOptionSucceed -f $Name )

Need to include the named parameter so:
Write-Verbose -Message ( $localizedData.GetAuditpolOptionSucceed -f $Name )

This goes for all function calls

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 33 at r9 (raw file):

    }

    $returnValue

this line is not needed - remove

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 38 at r9 (raw file):

<#
    .SYNOPSIS
    Sets the value of the audit policy option.

indent as specified in above comment

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 42 at r9 (raw file):

    Specifies the option to set.
    .PARAMETER Value
    Specifies the value to set on the option.

'Specifies the value to set the option to'

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 55 at r9 (raw file):

        $Name,

        [ValidateSet('Enabled', 'Disabled')]

Is there a reason this param doesn't have a default value and/or isn't mandatory? Won't Set-AuditOption fail if $Value isn't passed in?

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 62 at r9 (raw file):

    try
    {
        Set-AuditOption @PSBoundParameters

If $Value is always passed in here, I would change this as well to:
Set-AuditOption -Name $Name -Value $Value

Just because it makes it a bit more readable.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 63 at r9 (raw file):

    {
        Set-AuditOption @PSBoundParameters
        Write-Verbose ( $localizedData.SetAuditpolOptionSucceed -f $Name, $Value )

-Message

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 67 at r9 (raw file):

    catch
    {
        Write-Verbose ( $localizedData.SetAuditpolOptionFailed -f $Name, $Value )

-Message

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 73 at r9 (raw file):

<#
    .SYNOPSIS
    Sets the value of the audit policy option.

Fix indents

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 73 at r9 (raw file):

<#
    .SYNOPSIS
    Sets the value of the audit policy option.

Should this be 'Tests that the audit policy option is in the desired state' or something along those lines

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 98 at r9 (raw file):

    if ( ( Get-AuditOption -Name $Name ) -eq $Value )
    {
        Write-Verbose ( $localizedData.TestAuditpolOptionCorrect -f $Name,$value )

'... -f $Name, $Value'

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 99 at r9 (raw file):

    {
        Write-Verbose ( $localizedData.TestAuditpolOptionCorrect -f $Name,$value )
        $return = $true

this should be a more descriptive name: '$returnValue' or something and it should be declared at the top of the function.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 103 at r9 (raw file):

    else
    {
        Write-Verbose ( $localizedData.TestAuditpolOptionIncorrect -f $Name,$value )

-Message

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 114 at r9 (raw file):

<#
 .SYNOPSIS

indent everything within the block comment by 4 spaces

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 140 at r9 (raw file):

        get the 5th item in the list and return it
    #>
    ( ( ( Invoke-AuditPol -Command "Get" -SubCommand "Option:$Name" )[2] ) -split ',' )[4]

'-Split'

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 144 at r9 (raw file):

<#
 .SYNOPSIS

indent this comment as well

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 150 at r9 (raw file):

    that will be passed to Set-AuditOptionCommand and aligns to a specifc parameterset.
 .PARAMETER Name
    The specifc Option to set

'specific option'

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 152 at r9 (raw file):

    The specifc Option to set
 .PARAMETER Value
    The value to set on the provided Option

'The value to set the provide option to'

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 153 at r9 (raw file):

 .PARAMETER Value
    The value to set on the provided Option
 .OUTPUTS

this can be removed since there are no outputs

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 179 at r9 (raw file):

    {
        <#
        The output text of auditpol is in simple past tense, but the input is in simple

indent the contents of this block comment by 4 spaces.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 180 at r9 (raw file):

        <#
        The output text of auditpol is in simple past tense, but the input is in simple
        present tense the hashtable corrects the tense for the input.

'present tense, so the hashtable converts the input accordingly.'

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 182 at r9 (raw file):

        present tense the hashtable corrects the tense for the input.
        #>
        $valueHashTable = @{

change to something a bit more descriptive like, $pastToPresentValues

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 187 at r9 (raw file):

        }

        [string[]] $SubCommand = @( "Option:$Name", "/value:$($valueHashTable[$value])" )

'[String[]] $subCommand...'

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 189 at r9 (raw file):

        [string[]] $SubCommand = @( "Option:$Name", "/value:$($valueHashTable[$value])" )

        Invoke-AuditPol -Command "Set" -SubCommand $SubCommand | Out-Null

'Set' - single quotes  and $subCommand - lowercase

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.schema.mof, line 5 at r10 (raw file):

class MSFT_AuditPolicyOption : OMI_BaseResource
{
    [Key, Description("Sets the audit policy for the CrashOnAuditFail, FullPrivilegeAuditing, AuditBaseObjects, or AuditBaseDirectories options."), ValueMap{"CrashOnAuditFail","FullPrivilegeAuditing","AuditBaseObjects","AuditBaseDirectories"}, Values{"CrashOnAuditFail","FullPrivilegeAuditing","AuditBaseObjects","AuditBaseDirectories"}] String Name;

Description should be more along the lines of "The name of the audit policy option to set"

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.schema.mof, line 6 at r10 (raw file):

{
    [Key, Description("Sets the audit policy for the CrashOnAuditFail, FullPrivilegeAuditing, AuditBaseObjects, or AuditBaseDirectories options."), ValueMap{"CrashOnAuditFail","FullPrivilegeAuditing","AuditBaseObjects","AuditBaseDirectories"}, Values{"CrashOnAuditFail","FullPrivilegeAuditing","AuditBaseObjects","AuditBaseDirectories"}] String Name;
    [Write, Description("Describe the auditpol option state"), ValueMap{"Enabled","Disabled"}, Values{"Enabled","Disabled"}] String Value;

Description should be more along the line of: "The value to set the audit policy to"

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 2 at r10 (raw file):

Import-Module $PSScriptRoot\..\Helper.psm1

change to use Join-Path

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 5 at r10 (raw file):

<#
    .SYNOPSIS

fix indentation

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 8 at r10 (raw file):

    Returns the current audit flag for the given subcategory.
    .PARAMETER Subcategory
    Specifies the subcategory to get.

maybe change to 'retrieve' instead of 'get'?

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 18 at r10 (raw file):

    param
    (
        [parameter(Mandatory = $true)]

'Parameter'

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 41 at r10 (raw file):

        $Subcategory,

        [parameter(Mandatory = $true)]

'Parameter'

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 50 at r10 (raw file):

    {
        $currentAuditFlag = Get-AuditCategory -SubCategory $Subcategory
        Write-Verbose ( $localizedData.GetAuditpolSubcategorySucceed -f $Subcategory, $AuditFlag )

-Message

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 54 at r10 (raw file):

    catch
    {
        Write-Verbose ( $localizedData.GetAuditPolSubcategoryFailed -f $Subcategory, $AuditFlag )

-Message

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 82 at r10 (raw file):

<#
    .SYNOPSIS

indent

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 96 at r10 (raw file):

    param
    (
        [parameter(Mandatory = $true)]

'Parameter'

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 132 at r10 (raw file):

    {
        Set-AuditCategory -SubCategory $Subcategory -AuditFlag $AuditFlag -Ensure $Ensure
        Write-Verbose ( $localizedData.SetAuditpolSubcategorySucceed `

-Message

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 143 at r10 (raw file):

<#
    .SYNOPSIS

fix indentation

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 158 at r10 (raw file):

    param
    (
        [parameter(Mandatory = $true)]

'Parameter'

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 181 at r10 (raw file):

        $Subcategory,

        [parameter(Mandatory = $true)]

Parameter

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 194 at r10 (raw file):

    {
        $currentAuditFlag = Get-AuditCategory -SubCategory $Subcategory
        Write-Verbose ( $localizedData.GetAuditpolSubcategorySucceed -f $Subcategory, $AuditFlag )

-Message

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 204 at r10 (raw file):

    if ( $Ensure -eq 'Present' )
    {
        $return = $currentAuditFlag -match $AuditFlag

Again change to $returnValue and declare it outside of the if/else blocks

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 234 at r10 (raw file):

<#
 .SYNOPSIS

fix indentation

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 252 at r10 (raw file):

    param
    (
        [parameter(Mandatory = $true)]

Parameter

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 261 at r10 (raw file):

    #>
    # get the auditpol raw csv output
    $returnCsv = Invoke-AuditPol -Command "Get" -SubCommand "Subcategory:""$SubCategory"""

'Get' - single quotes

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 272 at r10 (raw file):

<#
 .SYNOPSIS

fix indentation

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 277 at r10 (raw file):

    Calls the private function to execute a set operation on the given subcategory
 .PARAMETER SubCategory
    The name of an audit category to set

'The name of the audit category to set'

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 284 at r10 (raw file):

 .EXAMPLE
    Set-AuditCategory -SubCategory 'Logon' -AuditFlag 'Success' -Ensure 'Present'
 .OUTPUTS

this can be deleted since there's no output.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 292 at r10 (raw file):

    param
    (
        [parameter( Mandatory = $true )]

Parameter

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 296 at r10 (raw file):

        $SubCategory,

        [parameter( Mandatory = $true )]

Parameter

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 301 at r10 (raw file):

        $AuditFlag,

        [parameter( Mandatory = $true )]

Parameter

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 318 at r10 (raw file):

        }

        # create the line needed auditpol to set the category flag

' # Create the line needed for auditpol to set the category flag

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 321 at r10 (raw file):

        if ( $AuditFlag -eq 'Success' )
        {
            [string[]] $subcommand = @( "Subcategory:""$SubCategory""", "/success:$($auditState[$Ensure])" )

[String[]]

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 325 at r10 (raw file):

        else
        {
            [string[]] $subcommand = @( "Subcategory:""$SubCategory""", "/failure:$($auditState[$Ensure])" )

[String[]]

Comments from Reviewable

Reviewed 3 of 27 files at r5, 1 of 7 files at r6, 2 of 4 files at r8.
Review status: 13 of 19 files reviewed at latest revision, 77 unresolved discussions.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.schema.mof, line 7 at r9 (raw file):

    [Key, Description("The Category to be configured"), ValueMap{"Security System Extension","System Integrity","IPsec Driver","Other System Events","Security State Change","Logon","Logoff","Account Lockout","IPsec Main Mode","IPsec Quick Mode","IPsec Extended Mode","Special Logon","Other Logon/Logoff Events","Network Policy Server","User / Device Claims","Group Membership","File System","Registry","Kernel Object","SAM","Certification Services","Application Generated","Handle Manipulation","File Share","Filtering Platform Packet Drop","Filtering Platform Connection","Other Object Access Events","Detailed File Share","Removable Storage","Central Policy Staging","Non Sensitive Privilege Use","Other Privilege Use Events","Sensitive Privilege Use","Process Creation","Process Termination","DPAPI Activity","RPC Events","Plug and Play Events","Authentication Policy Change","Authorization Policy Change","MPSSVC Rule-Level Policy Change","Filtering Platform Policy Change","Other Policy Change Events","Audit Policy Change","User Account Management","Computer Account Management","Security Group Management","Distribution Group Management","Application Group Management","Other Account Management Events","Directory Service Changes","Directory Service Replication","Detailed Directory Service Replication","Directory Service Access","Kerberos Service Ticket Operations","Other Account Logon Events","Kerberos Authentication Service","Credential Validation"}, Values{"Security System Extension","System Integrity","IPsec Driver","Other System Events","Security State Change","Logon","Logoff","Account Lockout","IPsec Main Mode","IPsec Quick Mode","IPsec Extended Mode","Special Logon","Other Logon/Logoff Events","Network Policy Server","User / Device Claims","Group Membership","File System","Registry","Kernel Object","SAM","Certification Services","Application Generated","Handle Manipulation","File Share","Filtering Platform Packet Drop","Filtering Platform Connection","Other Object Access Events","Detailed File Share","Removable Storage","Central Policy Staging","Non Sensitive Privilege Use","Other Privilege Use Events","Sensitive Privilege Use","Process Creation","Process Termination","DPAPI Activity","RPC Events","Plug and Play Events","Authentication Policy Change","Authorization Policy Change","MPSSVC Rule-Level Policy Change","Filtering Platform Policy Change","Other Policy Change Events","Audit Policy Change","User Account Management","Computer Account Management","Security Group Management","Distribution Group Management","Application Group Management","Other Account Management Events","Directory Service Changes","Directory Service Replication","Detailed Directory Service Replication","Directory Service Access","Kerberos Service Ticket Operations","Other Account Logon Events","Kerberos Authentication Service","Credential Validation"}] String Subcategory;
    [Key, Description("The setting to be configured"), ValueMap{"Success","Failure"}, Values{"Success","Failure"}] String AuditFlag;
    [Write, Description("Describe the setting state"), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;

Change this description to:
"Specifies whether the setting should be enabled or disabled. To enable the setting, set this property to Present. To disable the setting, set this property to Absent."

Tests/Integration/MSFT_AuditPolicyOption.Integration.tests.ps1, line 56 at r9 (raw file):

            Get-DscConfiguration -OutVariable DscConfiguration

            It "AuditOption configured is $optionName " {

all 'It' statements should start with It 'Should....'  So something like It 'Should return the correct option name'

Tests/Integration/MSFT_AuditPolicyOption.Integration.tests.ps1, line 60 at r9 (raw file):

            }

            It "$optionName is set to $optionValue"{

same as above It 'Should return the correct option value'

Tests/Integration/MSFT_AuditPolicyOption.Integration.tests.ps1, line 66 at r9 (raw file):

        }

        It 'Test-DscConfiguration should equal True' {

It 'Should return $true'

Tests/Integration/MSFT_AuditPolicyOption.Integration.tests.ps1, line 71 at r9 (raw file):

    }
    #endregion

Could you add another Context block of tests to ensure that the resource works as expected for disabling an option?

Comments from Reviewable

Reviewed 1 of 27 files at r5, 2 of 4 files at r8, 1 of 6 files at r9.
Review status: 16 of 19 files reviewed at latest revision, 101 unresolved discussions.

DSCResources/Helper.psm1, line 1 at r10 (raw file):

#Requires -Version 4.0

This file needs to be renamed to something more descriptive like: AuditPolicyResourceHelper.psm1

Tests/Integration/MSFT_AuditPolicySubcategory.config.ps1, line 12 at r6 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Done.

This still need to be changed to use params. Check out: [UserResource](https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Integration/MSFT_UserResource.config.ps1)

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 22 at r10 (raw file):

# set the Subcategory details being tested
$Subcategory     = 'Credential Validation'

these should be script variables and they shouldn't be capitalized.

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 27 at r10 (raw file):

# set the system Subcategory to the incorrect state to ensure a valid test.
Invoke-Expression "auditpol /set /subcategory:'Credential Validation' /failure:disable"

Invoke-Expression should not be used.

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 40 at r10 (raw file):

        It 'Should compile without throwing' {
            {
                Invoke-Expression -Command "$($script:DSCResourceName)_Config -OutputPath `$TestEnvironment.WorkingFolder"

Invoke-Expression should not be used. You should be able to replace it with '&' in this case. As in this example . This goes for all Integration tests.  I know this was in one of our integration test templates, but we've since refactored it to use '&'.

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 46 at r10 (raw file):

        }

        It 'should be able to call Get-DscConfiguration without throwing' {

'Should...'

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 55 at r10 (raw file):

            Get-DscConfiguration -OutVariable DscConfiguration

            It "Subcategory Should be $Subcategory" {

all of these 'It's should be in the form of It 'Should....'

Tests/Unit/Helper.Tests.ps1, line 1 at r10 (raw file):

#requires -RunAsAdministrator

This file should be renamed to something more descriptive like 'AuditPolicyTestHelper.ps1

Tests/Unit/Helper.Tests.ps1, line 7 at r10 (raw file):

# get the module name to import
[string] $sut = ( Split-Path -Leaf $MyInvocation.MyCommand.Path )  -Replace "\.tests\.ps1", ".psm1"

$sut needs to be a more descriptive variable name. Also [String] should be capitalized.

Tests/Unit/Helper.Tests.ps1, line 9 at r10 (raw file):

[string] $sut = ( Split-Path -Leaf $MyInvocation.MyCommand.Path )  -Replace "\.tests\.ps1", ".psm1"

Import-Module "$moduleRoot\DSCResources\$sut" -Force

This needs to be changed to use Join-Path instead of ''

Tests/Unit/Helper.Tests.ps1, line 16 at r10 (raw file):

    The auditpol utility outputs the list of categories and subcategories in a couple
    of different ways and formats. Using the /list flag only returns the categories
    without the associated audit setting, so it is easier to filter on.

'...so it is easier to filter later on.'  ?

Tests/Unit/Helper.Tests.ps1, line 39 at r10 (raw file):

}

$Categories, $SubCategories = Get-AuditpolCategories

These should be declared at the top of the file as script variables if they're going to be used throughout.

Tests/Unit/Helper.Tests.ps1, line 45 at r10 (raw file):

Describe 'Prereq' {

    # There are several dependencies for both Pester and auditpol resource that need to be validated.

'...Pester and AuditPolicyDsc that need...'

Tests/Unit/Helper.Tests.ps1, line 47 at r10 (raw file):

    # There are several dependencies for both Pester and auditpol resource that need to be validated.

    It "Checks if the tests are running as admin" {

It 'Should....' for all of these tests

Tests/Unit/Helper.Tests.ps1, line 67 at r10 (raw file):

    It "Checks auditpol default return with no parameters      " {

        ( auditpol.exe )[0] | should BeExactly 'Usage: AuditPol command [<sub-command><options>]'

'should' should be capitalized 'Should'

Tests/Unit/Helper.Tests.ps1, line 109 at r10 (raw file):

            It "Checks auditpol category returns $auditpolCategoryCount items  " {

                # The header row has been stripped, so greater than 1 is required to account for multiple subcategories

'...so there should be more than one category to account for multiple subcategories'

Tests/Unit/Helper.Tests.ps1, line 114 at r10 (raw file):

            # loop through the subcategories returned by the current category that was queried
            for ( $i=0;$i -lt $auditpolCategoryCount;$i++ )

can you put spacing around the '=' and after the ';'

Tests/Unit/Helper.Tests.ps1, line 119 at r10 (raw file):

                    # Verify that each filtered row that is returned, is in the expected format
                    $auditpolCategory[$i] | should Match "$env:ComputerName,System,"                   # <- Add more specific regexto account for positional GUID

this comment should be on it's own line above the one it's commenting about

Tests/Unit/Helper.Tests.ps1, line 164 at r10 (raw file):

            Select-String -Pattern $env:ComputerName

            It "Checks auditpol Subcategory returns one item         " {

remove extra whitespace at end of 'It' name

Tests/Unit/Helper.Tests.ps1, line 172 at r10 (raw file):

            It "Checks auditpol Subcategory returns entry on line 0  " {

                    # Verify that each filtered row that is returned, is in the expected format

'... that is returned is in the expected format'  (comma not needed)

Tests/Unit/Helper.Tests.ps1, line 173 at r10 (raw file):

                    # Verify that each filtered row that is returned, is in the expected format
                    $auditpolSubcategory[0] | should Match "$env:ComputerName,System,"                   # <- Add more specific regex

comments need to be on their own line

Tests/Unit/Helper.Tests.ps1, line 189 at r10 (raw file):

    InModuleScope Helper {

        $command = Get-Command Invoke-Auditpol

Get-Command -Name Invoke-AuditPol

Tests/Unit/Helper.Tests.ps1, line 190 at r10 (raw file):

        $command = Get-Command Invoke-Auditpol
        # parameters listed in the Hashtable name = type, Mandatory, validateSet

I'm a little confused with this hashtable. Can you provide a more descriptive name and a clearer comment about what it's holding?

Tests/Unit/Helper.Tests.ps1, line 196 at r10 (raw file):

                        }

        It "Should Exist" {

What is this It testing for? Isn't this trivial?

Tests/Unit/Helper.Tests.ps1, line 213 at r10 (raw file):

        }

        # verify all of ther parameters are correct

'...all of the parameters...'

Comments from Reviewable

Review status: 16 of 19 files reviewed at latest revision, 119 unresolved discussions.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 2 at r10 (raw file):

$Global:DSCModuleName      = 'AuditPolicyDsc'

these need to be script variables - not globals : $script:moduleName, $script:resourceName  (again, that was from an old template)

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 6 at r10 (raw file):

#region HEADER
[String] $moduleRoot = Split-Path -Parent (Split-Path -Parent (Split-Path -Parent $Script:MyInvocation.MyCommand.Path))

$script:moduleRoot

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 28 at r10 (raw file):

    #region Pester Tests

    # The InModuleScope command allows you to perform white-box unit testing on the internal

this comment should be removed

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 32 at r10 (raw file):

    InModuleScope $Global:DSCResourceName {

        #region Pester Test Initialization

add a newline after #region ... and before #endregion

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 43 at r10 (raw file):

            # mock call to the helper module to isolate Get-TargetResource
            Mock Get-AuditOption { return $optionState } -ModuleName MSFT_AuditPolicyOption

Mock -CommandName Get-AuditOption -MockWith {...}   - make sure to add named parameters to all function calls including mocks.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 45 at r10 (raw file):

            Mock Get-AuditOption { return $optionState } -ModuleName MSFT_AuditPolicyOption

            $get = Get-TargetResource -Name $optionName

more descriptive name: $getTargetResourceResult

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 47 at r10 (raw file):

            $get = Get-TargetResource -Name $optionName

            It "Return object is a hashtable" {

all of these Its need to be in It 'Should....' form

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 52 at r10 (raw file):

            }

            It " that has a 'Name' key" {

this test isn't really needed since we check for the value in the test below.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 62 at r10 (raw file):

            }

            It " that has a 'Value' key" {

this test is also not really needed.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 79 at r10 (raw file):

            Mock Get-AuditOption { return $optionState } -ModuleName MSFT_AuditPolicyOption

            $test = Test-TargetResource -Name $optionName -Value $optionState

$testTargetResourceResult

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 79 at r10 (raw file):

            Mock Get-AuditOption { return $optionState } -ModuleName MSFT_AuditPolicyOption

            $test = Test-TargetResource -Name $optionName -Value $optionState

It think that this call should be moved to inside your second It statement and the result tested for directly.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 82 at r10 (raw file):

            It "Return object is a Boolean" {
                $isBool = $test.GetType().Name -eq "Boolean"

this test can be removed

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 88 at r10 (raw file):

            It " that is true when matching" {
                $valueMatches = $test
                $valueMatches | Should Be $true

why not just test: '$test | Should Be $true'  ?

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 92 at r10 (raw file):

            It " and is false when not matching" {
                $valueNotMatches = Test-TargetResource -Name $optionName -Value $optionStateSwap[$optionState]

this should still be called $testTargetResourcesResult - the It statement should describe what the test is testing

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 102 at r10 (raw file):

            # mock call to the helper module to isolate Set-TargetResource
            Mock Set-AuditOption { return } -ModuleName MSFT_AuditPolicyOption

this '-MockWith' can be empty: {}

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 104 at r10 (raw file):

            Mock Set-AuditOption { return } -ModuleName MSFT_AuditPolicyOption

            $set = Set-TargetResource -Name $optionName -Value $optionState

$setTargetResourceResult

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 108 at r10 (raw file):

            It " returns no object" {

                $set | Should BeNullOrEmpty

this test should be removed, but the function does need to be actually tested. One of the best methods of testing Set-TargetResource is using Assert-MockCalled. As in this example

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 121 at r10 (raw file):

            It "Should Exist" {

                $command | Should Be $command

all of these Should Exist tests and should be of type tests aren't really that helpful and should be replaced with tests that really check the functionality of the code. I would recommend looking at some of these unit tests for example: Service, WindowsOptionalFeature

Comments from Reviewable

Review status: 16 of 19 files reviewed at latest revision, 119 unresolved discussions.

DSCResources/Helper.psm1, line 14 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'manage'  instead of 'mange'?

Done.

DSCResources/Helper.psm1, line 21 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'operator' - singular

Done.

DSCResources/Helper.psm1, line 24 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'function.'

Done.

DSCResources/Helper.psm1, line 28 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'The subcommand to execute.'

Done.

DSCResources/Helper.psm1, line 40 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'Parameter' - in all cases below as well.

Done.

DSCResources/Helper.psm1, line 75 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'does not throw...'

Done.

DSCResources/Helper.psm1, line 91 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> '[String]'

Done.

DSCResources/en-US/helper.psd1, line 22 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> rename this file to 'AuditPolicyDsc.strings.psd1'

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 2 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Should use 'Join-Path' instead of '\' :
> ```powershell
> Import-Module -Name (Join-Path -Path (Split-Path $PSScriptRoot -Parent) `
>                                -ChildPath (Join-Path -Path 'DscResources' `
>                                                      -ChildPath 'Helper.psm1')) `
>                                                      -Force
> ```

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 6 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> tab in the information bits:
> ```powershell
> <#
>     .SYNOPSIS
>         Gets the value of the audit policy option.
>     .PARAMETER Name
>         Specifies the option to get.
> #>
> ```

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 24 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> since this is always going to pass in just $Name - I would replace '@PSBoundParameters' with '-Name $Name'

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 26 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Need to include the named parameter so:
>
> ```powershell
> Write-Verbose -Message ( $localizedData.GetAuditpolOptionSucceed -f $Name )
> ```
>
> This goes for all function calls

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 33 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this line is not needed - remove

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 42 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'Specifies the value to set the option to'

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 55 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Is there a reason this param doesn't have a default value and/or isn't mandatory? Won't Set-AuditOption fail if $Value isn't passed in?

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 62 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> If $Value is always passed in here, I would change this as well to:
>
> ```powershell
> Set-AuditOption -Name $Name -Value $Value
> ```
> Just because it makes it a bit more readable.

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 63 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> -Message

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 67 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> -Message

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 73 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Fix indents

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 73 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Should this be 'Tests that the audit policy option is in the desired state' or something along those lines

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 98 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> ```powershell
> '... -f $Name, $Value'
> ```

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 99 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this should be a more descriptive name: '$returnValue' or something and it should be declared at the top of the function.

Done. I removed the variable altogether and just return the bool in side the condition, since there is nothing else to process after the test.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 103 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> -Message

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 114 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> indent everything within the block comment by 4 spaces

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 140 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> '-Split'

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 144 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> indent this comment as well

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 150 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'specific option'

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 152 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'The value to set the provide option to'

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 153 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this can be removed since there are no outputs

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 179 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> indent the contents of this block comment by 4 spaces.

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 180 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'present tense, so the hashtable converts the input accordingly.'

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 182 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> change to something a bit more descriptive like, $pastToPresentValues

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 189 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'Set' - single quotes  and $subCommand - lowercase

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.schema.mof, line 5 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Description should be more along the lines of "The name of the audit policy option to set"

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.schema.mof, line 6 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Description should be more along the line of: "The value to set the audit policy to"

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 2 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> change to use Join-Path

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 5 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> fix indentation

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 8 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> maybe change to 'retrieve' instead of 'get'?

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 18 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'Parameter'

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 41 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'Parameter'

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 50 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> -Message

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 54 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> -Message

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 82 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> indent

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 96 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'Parameter'

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 132 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> -Message

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 143 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> fix indentation

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 158 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'Parameter'

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 181 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Parameter

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 194 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> -Message

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 204 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Again change to $returnValue and declare it outside of the if/else blocks

I updated this to $isInDesiredState to make it easier to read through the function.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 234 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> fix indentation

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 252 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Parameter

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 261 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'Get' - single quotes

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 272 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> fix indentation

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 277 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'The name of the audit category to set'

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 284 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this can be deleted since there's no output.

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 292 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Parameter

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 296 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Parameter

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 301 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Parameter

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 318 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> ' # Create the line needed for auditpol to set the category flag

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 321 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> [String[]]

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 325 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> [String[]]

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.schema.mof, line 7 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Change this description to:
> "Specifies whether the setting should be enabled or disabled. To enable the setting, set this property to Present. To disable the setting, set this property to Absent."

Done.

Tests/Integration/MSFT_AuditPolicyOption.Integration.tests.ps1, line 56 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> all 'It' statements should start with It 'Should....'  So something like It 'Should return the correct option name'

Done.

Tests/Integration/MSFT_AuditPolicyOption.Integration.tests.ps1, line 60 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> same as above It 'Should return the correct option value'

Done.

Tests/Integration/MSFT_AuditPolicyOption.Integration.tests.ps1, line 66 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> It 'Should return $true'

Done.

Tests/Integration/MSFT_AuditPolicyOption.Integration.tests.ps1, line 71 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Could you add another Context block of tests to ensure that the resource works as expected for disabling an option?

When I look at the UserResource test, the configuration is generated in the Context, not the Describe. The integration test template doesn't do this, but I like the UserResource format better, so let me know if what I did is satisfactory.

Tests/Integration/MSFT_AuditPolicySubcategory.config.ps1, line 12 at r6 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> This still need to be changed to use params. Check out: [UserResource](https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Integration/MSFT_UserResource.config.ps1)

Done. Let me know if what I did is satisfactory

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 22 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> these should be script variables and they shouldn't be capitalized.

Done.

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 27 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Invoke-Expression should not be used.

Done.

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 40 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Invoke-Expression should not be used. You should be able to replace it with '&' in this case. As in [this example](https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Integration/MSFT_WindowsPackageCab.Integration.Tests.ps1#L80) . This goes for all Integration tests.  I know this was in one of our integration test templates, but we've since refactored it to use '&'.

Done.

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 46 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'Should...'

Done.

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 55 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> all of these 'It's should be in the form of It 'Should....'

Done.

Tests/Unit/Helper.Tests.ps1, line 1 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> This file should be renamed to something more descriptive like 'AuditPolicyTestHelper.ps1

Done.

Tests/Unit/Helper.Tests.ps1, line 7 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> $sut needs to be a more descriptive variable name. Also [String] should be capitalized.

Done.

Tests/Unit/Helper.Tests.ps1, line 9 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> This needs to be changed to use Join-Path instead of '\'

Done.

Tests/Unit/Helper.Tests.ps1, line 16 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> '...so it is easier to filter later on.'  ?

Done.

Tests/Unit/Helper.Tests.ps1, line 39 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> These should be declared at the top of the file as script variables if they're going to be used throughout.

Done.

Tests/Unit/Helper.Tests.ps1, line 45 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> '...Pester and AuditPolicyDsc that need...'

Done.

Tests/Unit/Helper.Tests.ps1, line 47 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> It 'Should....' for all of these tests

Done.

Tests/Unit/Helper.Tests.ps1, line 67 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'should' should be capitalized 'Should'

Done.

Tests/Unit/Helper.Tests.ps1, line 109 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> '...so there should be more than one category to account for multiple subcategories'

Done.

Tests/Unit/Helper.Tests.ps1, line 114 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> can you put spacing around the '=' and after the ';'

Done.

Tests/Unit/Helper.Tests.ps1, line 119 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this comment should be on it's own line above the one it's commenting about

Done.

Tests/Unit/Helper.Tests.ps1, line 164 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> remove extra whitespace at end of 'It' name

Done.

Tests/Unit/Helper.Tests.ps1, line 172 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> '... that is returned is in the expected format'  (comma not needed)

Done.

Tests/Unit/Helper.Tests.ps1, line 173 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> comments need to be on their own line

Done.

Tests/Unit/Helper.Tests.ps1, line 189 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Get-Command -Name Invoke-AuditPol

Done.

Tests/Unit/Helper.Tests.ps1, line 190 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> I'm a little confused with this hashtable. Can you provide a more descriptive name and a clearer comment about what it's holding?

Done. I was trying to reuse the tests and loop through a list of parameters attributes, but it is confusing and I removed it altogether and updated the tests.

Tests/Unit/Helper.Tests.ps1, line 196 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> What is this It testing for? Isn't this trivial?

Done. I updated it to test the actual results of Get-Command

Tests/Unit/Helper.Tests.ps1, line 213 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> '...all of the parameters...'

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 2 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> these need to be script variables - not globals : $script:moduleName, $script:resourceName  (again, that was from an old template)

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 6 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> $script:moduleRoot

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 28 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this comment should be removed

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 32 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> add a newline after #region ... and before #endregion

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 43 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Mock -CommandName Get-AuditOption -MockWith {...}   - make sure to add named parameters to all function calls including mocks.

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 45 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> more descriptive name: $getTargetResourceResult

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 52 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this test isn't really needed since we check for the value in the test below.

Removed

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 62 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this test is also not really needed.

Removed

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 79 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> $testTargetResourceResult

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 79 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> It think that this call should be moved to inside your second It statement and the result tested for directly.

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 82 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this test can be removed

Removed

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 88 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> why not just test: '$test | Should Be $true'  ?

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 92 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this should still be called $testTargetResourcesResult - the It statement should describe what the test is testing

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 102 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this '-MockWith' can be empty: {}

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 104 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> $setTargetResourceResult

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 108 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this test should be removed, but the function does need to be actually tested. One of the best methods of testing Set-TargetResource is using Assert-MockCalled. As in [this example](https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Unit/MSFT_ServiceResource.Tests.ps1#L179)

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 121 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> all of these Should Exist tests and should be of type tests aren't really that helpful and should be replaced with tests that really check the functionality of the code. I would recommend looking at some of these unit tests for example: [Service](https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Unit/MSFT_ServiceResource.Tests.ps1), [WindowsOptionalFeature](https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Unit/MSFT_WindowsOptionalFeature.Tests.ps1)

I took my first swipe at testing in different contexts.

Comments from Reviewable

Review status: 16 of 19 files reviewed at latest revision, 119 unresolved discussions.

DSCResources/Helper.psm1, line 1 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> This file needs to be renamed to something more descriptive like: AuditPolicyResourceHelper.psm1

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 38 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> indent as specified in above comment

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 187 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> '[String[]] $subCommand...'

Done.

Comments from Reviewable

Reviewed 7 of 28 files at r5, 13 of 16 files at r11.
Review status: 17 of 19 files reviewed at latest revision, 49 unresolved discussions, some commit checks failed.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 55 at r9 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Done.

After further looking at this, I think this value should just be made mandatory since there doesn't really seem to be a scenario when a user wouldn't want to specify it - update in Test-TargetResource/the mof as well.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 150 at r9 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Done.

'specific' is still spelled wrong here

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 152 at r9 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Done.

Sorry, my typo - keep 'provided'  so:  'The value to set the provided option to'

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 189 at r9 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Done.

sorry - only the variable $subCommand should be lowercase -  the param '-SubCommand' should still be uppercase.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 25 at r11 (raw file):

    # get the option's current value
    $optionValue = Get-AuditOption -Name $name

$Name - capitalized

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 61 at r11 (raw file):

    try
    {
        Set-AuditOption -Name $name -Value $Value

$Name

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 261 at r10 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Done.

-SubCommand should still be uppercase

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 116 at r11 (raw file):

        'Other Account Logon Events', 'Kerberos Authentication Service', 'Credential Validation')]
        [System.String]
        $Subcategory,

$Subcategory I think should be mandatory too - unless there's a scenario when it wouldn't be passed in.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 125 at r11 (raw file):

        [ValidateSet('Present', 'Absent')]
        [System.String]
        $Ensure

This should have a default value of 'Present' and that should be included in the function help comment and the readme.

Tests/Integration/MSFT_AuditPolicyOption.Integration.tests.ps1, line 71 at r9 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> When I look at the UserResource test, the configuration is generated in the Context, not the Describe. The integration test template doesn't do this, but I like the UserResource format better, so let me know if what I did is satisfactory.

Yeah, this is great :)  - this format makes it a lot cleaner to test different configurations.

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 22 at r10 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Done.

so it should be $script:subCategory

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 6 at r10 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Done.

This still needs to be fixed.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 121 at r10 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> I took my first swipe at testing in different contexts.

No problem :) I appreciate all the work that you've put into these tests.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 47 at r11 (raw file):

                It 'Should not throw an exception' {
                    { $script:getTargetResourceResult = Get-TargetResource -Name $optionName } |

this shouldn't be a script variable. just  $getTargetResourceResult

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 161 at r11 (raw file):

                Value = $null
            }
            context 'Option Enabled' {

Context - Capitalized

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 176 at r11 (raw file):

            }

            context 'Option Disabled' {

Context

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 181 at r11 (raw file):

                It 'Should not throw an exception' {
                    { $script:setTargetResourceResult = Set-TargetResource @target } | Should Not Throw

this variable $script:setTargetResourceResult can be removed since Set-TargetResource doesn't return anything

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 196 at r11 (raw file):

            Context 'Get-AuditOption with Mock Invoke-Auditpol' {

                [string] $name  = 'CrashOnAuditFail'

you can delete the type declarations here - they're not needed

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 198 at r11 (raw file):

                [string] $name  = 'CrashOnAuditFail'
                [string] $value = 'Enabled'
                # the return is 3 lines Header, blank line, data

Can you make this a block comment with <# #>

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 207 at r11 (raw file):

                It "Should return the correct value" {
                    $auditOption | should Be $value

'Should Be..'

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 216 at r11 (raw file):

            Context "Set-AuditOption to enabled" {

                [string] $name  = "CrashOnAuditFail"

again - these String type declarations are not needed

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 231 at r11 (raw file):

            }

            Context "Set-AuditOption to disabled" {

'...to Disabled'

Comments from Reviewable

Reviewed 2 of 28 files at r5, 1 of 16 files at r11, 2 of 3 files at r12.
Review status: 18 of 19 files reviewed at latest revision, 31 unresolved discussions.

Tests/Unit/AuditPolicyResourceHelper.Tests.ps1, line 2 at r12 (raw file):

#requires -RunAsAdministrator

Change the name of this file to AuditPolicyTestHelper.ps1

Tests/Unit/AuditPolicyResourceHelper.Tests.ps1, line 4 at r12 (raw file):

# get the root path of the resourse
[String] $moduleRoot = Split-Path -Parent ( Split-Path -Parent $PSScriptRoot )

$script:moduleRoot

Tests/Unit/AuditPolicyResourceHelper.Tests.ps1, line 21 at r12 (raw file):

#>

$categories = @()

$script:categories
$script:subCategories

Tests/Unit/AuditPolicyResourceHelper.Tests.ps1, line 176 at r12 (raw file):

        }

        It 'Should output a type "String"' {

you can delete this test

Tests/Unit/AuditPolicyResourceHelper.Tests.ps1, line 203 at r12 (raw file):

        }

        It 'Should return a CSV format when a option is passed in' {

'...when an option is passed in'

Comments from Reviewable

Review status: all files reviewed at latest revision, 44 unresolved discussions.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 150 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'specific' is still spelled wrong here

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 152 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Sorry, my typo - keep 'provided'  so:  'The value to set the provided option to'

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 189 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> sorry - only the variable $subCommand should be lowercase -  the param '-SubCommand' should still be uppercase.

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 25 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> $Name - capitalized

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 61 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> $Name

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 261 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> -SubCommand should still be uppercase

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 116 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> $Subcategory I think should be mandatory too - unless there's a scenario when it wouldn't be passed in.

This is already set to Mandatory and a Key property in schema. Was there something else I needed to do?

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 125 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> This should have a default value of 'Present' and that should be included in the function help comment and the readme.

Done.

Tests/Integration/MSFT_AuditPolicySubcategory.Integration.tests.ps1, line 22 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> so it should be $script:subCategory

O duh! Sorry about that. Done

Tests/Unit/AuditPolicyResourceHelper.Tests.ps1, line 2 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Change the name of this file to AuditPolicyTestHelper.ps1

Done. I updated the Import-Module path as well

Tests/Unit/AuditPolicyResourceHelper.Tests.ps1, line 4 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> $script:moduleRoot

Done.

Tests/Unit/AuditPolicyResourceHelper.Tests.ps1, line 21 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> $script:categories
> $script:subCategories

Done.

Tests/Unit/AuditPolicyResourceHelper.Tests.ps1, line 176 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> you can delete this test

Done.

Tests/Unit/AuditPolicyResourceHelper.Tests.ps1, line 203 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> '...when an option is passed in'

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 6 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> This still needs to be fixed.

Extra Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 47 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> all of these Its need to be in It 'Should....' form

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 47 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this shouldn't be a script variable. just  $getTargetResourceResult

If I remove the script scope the rest of the tests fail, since I only call the function once and reuse the results in the following tests. Do you want me leave the script scope or add the function call to each test?

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 161 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Context - Capitalized

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 176 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Context

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 181 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> this variable $script:setTargetResourceResult can be removed since Set-TargetResource doesn't return anything

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 196 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> you can delete the type declarations here - they're not needed

I think your comments and a push crossed on the internet since that context name doesn't exist anymore. Let me know if the current tests are good.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 198 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Can you make this a block comment with <# #>

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 207 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> 'Should Be..'

Done.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 216 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> again - these String type declarations are not needed

They had been removed in a previous push

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 231 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> '...to Disabled'

Done. I updated the Enabled context as well.

Tests/Unit/MSFT_AuditPolicySubcategory.tests.ps1, line 6 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> $script:moduleRoot

Done.

Tests/Unit/MSFT_AuditPolicySubcategory.tests.ps1, line 28 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> delete this comment

Done.

Tests/Unit/MSFT_AuditPolicySubcategory.tests.ps1, line 32 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> delete these three lines of comments

Done.

Tests/Unit/MSFT_AuditPolicySubcategory.tests.ps1, line 38 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> delete the regions around the describe blocks as they don't help with readability

agree and done

Tests/Unit/MSFT_AuditPolicySubcategory.tests.ps1, line 41 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Maybe rename this to $testParameters ?

Done.

Tests/Unit/MSFT_AuditPolicySubcategory.tests.ps1, line 46 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> why not just assign these values in the hashtable?

Gute Frage. Fixed

Tests/Unit/MSFT_AuditPolicySubcategory.tests.ps1, line 743 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> It 'Should...'

Done.

Tests/Unit/MSFT_AuditPolicySubcategory.tests.ps1, line 767 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> It 'Should...'

Done.

Tests/Unit/MSFT_AuditPolicySubcategory.tests.ps1, line 793 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> It 'Should...'

Done.

Tests/Unit/MSFT_AuditPolicySubcategory.tests.ps1, line 817 at r12 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> It 'Should...'

Done.

Comments from Reviewable

Review status: 10 of 20 files reviewed at latest revision, 41 unresolved discussions.

README.md, line 20 at r10 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Under each of these resource descriptions could you change it to the format used in [PSDscResources](https://github.com/PowerShell/PSDscResources/blob/dev/README.md#group). So:
> ### ResourceName
> #### Requirements
> #### Parameters
> #### Read-Only Properties from Get-TargetResource
> #### Examples

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 55 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> After further looking at this, I think this value should just be made mandatory since there doesn't really seem to be a scenario when a user wouldn't want to specify it - update in Test-TargetResource/the mof as well.

Done.

Comments from Reviewable

Reviewed 5 of 28 files at r5, 6 of 9 files at r13, 1 of 2 files at r14, 4 of 4 files at r15.
Review status: 19 of 20 files reviewed at latest revision, 17 unresolved discussions.

README.md, line 29 at r15 (raw file):

### AuditPolicySubcategory
Provides a mechanism to manage advanced audit policy subcategory audit flags.
This resource works on Nano Server.

Has this resource been tested on Nano Server?

README.md, line 40 at r15 (raw file):

manage.

* **[String] AuditFlag _(Key)_**: The name of the audit flag to apply to the subcategory. This is

can you put the options for this param in the format like 'Ensure' in this resource

README.md, line 43 at r15 (raw file):

can be Success or Failure.

* **[String] Ensure _(Write)_**: Indicates if the audit flag should exist or not. To add a flag to

can you put the options and default value for this param in the format like 'Ensure' in this resource

README.md, line 59 at r15 (raw file):

### AuditPolicyOption
Provides a mechanism to manage audit policy options.
This resource works on Nano Server.

has this resource been tested on Nano Server?

README.md, line 69 at r15 (raw file):

* **[String] Name _(Key)_**: The name of the option to configure.

* **[String] Value _(Key)_**: The value to apply to the option. This can be either Enabled or

change format as above.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 10 at r15 (raw file):

    .PARAMETER Name
        Specifies the option to get.
#>

add the Param value here too - just mention that it's not used in Get-TargetResource

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 116 at r11 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> This is already set to Mandatory and a Key property in schema. Was there something else I needed to do?

oh, no sorry, my mistake. This is good.

Tests/Unit/MSFT_AuditPolicyOption.tests.ps1, line 47 at r11 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> If I remove the script scope the rest of the tests fail, since I only call the function once and reuse the results in the following tests. Do you want me leave the script scope or add the function call to each test?

This is OK as is.

Comments from Reviewable

Reviewed 1 of 28 files at r5, 1 of 2 files at r14.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

Comments from Reviewable

Review status: all files reviewed at latest revision, 15 unresolved discussions.

README.md, line 29 at r15 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Has this resource been tested on Nano Server?

in my home lab. I talked to Zach and Auditpol is still included in nano as well as the new cmdlets.

README.md, line 40 at r15 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> can you put the options for this param in the format like 'Ensure' in [this resource](https://github.com/PowerShell/PSDscResources/blob/dev/README.md#parameters-1)

Done.

README.md, line 43 at r15 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> can you put the options and default value for this param in the format like 'Ensure' in [this resource](https://github.com/PowerShell/PSDscResources/blob/dev/README.md#parameters-1)

Done.

README.md, line 59 at r15 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> has this resource been tested on Nano Server?

yes in my home lab.

README.md, line 69 at r15 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> change format as above.

Done.

DSCResources/MSFT_AuditPolicyOption/MSFT_AuditPolicyOption.psm1, line 10 at r15 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> add the Param value here too - just mention that it's not used in Get-TargetResource

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 116 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> oh, no sorry, my mistake. This is good.

I'll allow your one mistake to my 100 :)

Comments from Reviewable

Reviewed 1 of 28 files at r5, 2 of 2 files at r16.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

README.md, line 43 at r15 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> Done.

Almost.. can you italicize the default value:
```
{ *Present* | Absent }
```

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 116 at r11 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> I'll allow your one mistake to my 100 :)

Not mistakes just small improvements :) Overall, you've done a great job with this resource design.

Comments from Reviewable

Reviewed 1 of 1 files at r17.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

Comments from Reviewable

@mbreakey3 @zjalexander since Zach has a PR with the CSV module do you want me to bring that in and clean it up before taking over the resource?

Merge this, and then we'll take care of audit CSV next - there's so much refactoring that needs to happen that I want to get this closed down first :)

Yes - merge this first. @kwirkykat  just needs to sign off on the review which should happen at the beginning of next week.

@mbreakey3 @zachal @athaynes AuditPolicyOption looks great, but I have some concerns about AuditPolicySubCategory.
Since AuditCategory has been renamed to AuditPolicySubcategory, the functions in that module need to renamed to match (Get-AuditCategory --> Get-AuditPolicySubcategory or Get-AuditSubcategory, etc).
Along that same line, it seems confusing for the AuditPolicySubcategory resource to take a parameter called 'Subcategory'. This parameter should be renamed to 'Name' as it is the name of the subcategory to get/test/set.
I'm also concerned that the Subcategory/Name parameter has such a large ValidateSet attribute. The large, hard-coded list has to match for both Test and Set, and it's going to be difficult to maintain that list. I would suggest moving it to a function that validates the the given subcateory list is one of the values returned from auditpol /list /subcategory:* or some similar dynamic way to retrieve the appropriate subcategory values.  If the list cannot be generated dynamically, then at least having it contained in one place will make the list easier to update if needed.
This function would then be called at the very top of both Set and Test. The downside to this is that the parameter validation won't happen as early in the process of calling the function, but I think the list is so large that it would be more valuable to validate the list dynamically/in one place with a function.

Reviewed 4 of 28 files at r5, 1 of 3 files at r12, 1 of 9 files at r13, 1 of 2 files at r14, 1 of 4 files at r15, 1 of 2 files at r16, 1 of 1 files at r17.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

AuditPolicyDsc.psd1, line 75 at r17 (raw file):

# Variables to export from this module
VariablesToExport = '*'

This line should be commented out (VariablesToExport)
To make sure LocalizedData from the AuditPolicyResourceHelper or any other variables are not unintentionally exported.
Along the same line you can also comment out all the other exports (Cmdlets, Aliases, Functions) as well.

DSCResources/AuditPolicyResourceHelper.psm1, line 9 at r17 (raw file):

#>

Import-LocalizedData -BindingVariable LocalizedData -Filename AuditPolicyDsc.strings.psd1

The use of Import-LocalizedData with only the en-us file available means that thismodule will throw an error on any machine that does not have the system locale set to en-us.
We solved this problem in PSDscResources with the Get-LocalizedData function in the CommonResourceHelper which you can see here

Tests/Integration/MSFT_AuditPolicyOption.config.ps1, line 1 at r17 (raw file):

# Integration Test Config Template Version 1.0.0

Please remove template comment

Tests/Integration/MSFT_AuditPolicySubcategory.config.ps1, line 1 at r17 (raw file):

# Integration Test Config Template Version 1.0.0

Please remove template comment

Tests/Unit/AuditPolicyTestHelper.ps1, line 2 at r17 (raw file):

#requires -RunAsAdministrator

This file should be renamed.
It is not really a 'TestHelper' (a module that contains functions to help run the unit/integration tests), but rather tests for the AuditPolicyResourceHelper module.
These also are not unit tests. Nothing is mocked. auditpol.exe is actually called in this file, so these are integration tests that rely on auditpol.exe rather than independent unit tests.
This file should be moved to the Tests/Integration folder and be renamed AuditPolicyResourceHelper.Integration.Tests.ps1

Tests/Unit/AuditPolicyTestHelper.ps1, line 64 at r17 (raw file):

Foreach(

foreach (

The 'F' should not be capitalized and there should be a space before the first parenthesis.
This should be fixed throughout the file.

Tests/Unit/AuditPolicyTestHelper.ps1, line 162 at r17 (raw file):

Private function

This is no longer a private function

Comments from Reviewable

@kwirkykat I renamed the Subcategory parameter to Name in the AuditPolicySubcategory schema, module, readme, examples, and tests.
I pulled the Subcategory list from the schema ValueMap/Values and ValidateSet in the module . I create the subcategory list in the Helper module when it is loaded and added the function Test-ValidSubcategory for the Set\Test to use. Both Set and Test will throw an error if the Name parameter is invalidated by Test-ValidSubcategory.

Review status: 12 of 21 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

AuditPolicyDsc.psd1, line 75 at r17 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> This line should be commented out (VariablesToExport)
>
> To make sure LocalizedData from the AuditPolicyResourceHelper or any other variables are not unintentionally exported.
>
> Along the same line you can also comment out all the other exports (Cmdlets, Aliases, Functions) as well.

Done.

README.md, line 43 at r15 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Almost.. can you italicize the default value:
> ```
> { *Present* | Absent }
> ```

Bah. Should have looked at the Raw example. Done.

DSCResources/AuditPolicyResourceHelper.psm1, line 9 at r17 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> The use of Import-LocalizedData with only the en-us file available means that thismodule will throw an error on any machine that does not have the system locale set to en-us.
>
> We solved this problem in PSDscResources with the Get-LocalizedData function in the CommonResourceHelper which you can see [here](https://github.com/PowerShell/PSDscResources/blob/dev/DscResources/CommonResourceHelper.psm1#L112)

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 116 at r11 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Not mistakes just small improvements :) Overall, you've done a great job with this resource design.

Thanks for the feedback and help getting it ready.

Tests/Integration/MSFT_AuditPolicyOption.config.ps1, line 1 at r17 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please remove template comment

Done.

Tests/Integration/MSFT_AuditPolicySubcategory.config.ps1, line 1 at r17 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Please remove template comment

Done.

Tests/Unit/AuditPolicyTestHelper.ps1, line 2 at r17 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> This file should be renamed.
>
> It is not really a 'TestHelper' (a module that contains functions to help run the unit/integration tests), but rather tests for the AuditPolicyResourceHelper module.
>
> These also are not unit tests. Nothing is mocked. auditpol.exe is actually called in this file, so these are integration tests that rely on auditpol.exe rather than independent unit tests.
>
> This file should be moved to the Tests/Integration folder and be renamed AuditPolicyResourceHelper.Integration.Tests.ps1

Done.

Tests/Unit/AuditPolicyTestHelper.ps1, line 64 at r17 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > Foreach(
>
> ```powershell
> foreach (
> ```
>
> The 'F' should not be capitalized and there should be a space before the first parenthesis.
> This should be fixed throughout the file.

Done.

Tests/Unit/AuditPolicyTestHelper.ps1, line 162 at r17 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > Private function
>
> This is no longer a private function

Done.

Comments from Reviewable

@athaynes @zachal Yay! Thank you Adam! Just a few last comments and then this should be good to go :smiley:

Reviewed 3 of 29 files at r5, 1 of 19 files at r11, 1 of 10 files at r13, 1 of 4 files at r15, 6 of 11 files at r18, 7 of 11 files at r19, 1 of 1 files at r20.
Review status: 18 of 21 files reviewed at latest revision, 9 unresolved discussions.

DSCResources/AuditPolicyResourceHelper.psm1, line 72 at r20 (raw file):

$localizedData

This variable is no longer initialized in this file.
Occurs below in this function as well.
This function will crash if it hits this line and the variable has not been declared.
You will probably still need a separate .strings file for this module.
You can then call Get-LocalizedData at the top of this function since it isn't used in any other functions.

DSCResources/AuditPolicyResourceHelper.psm1, line 116 at r20 (raw file):

# Populating $validSubcategory uses Invoke-AuditPol, so it needs to come after the definition
Invoke-AuditPol -Command List -SubCommand "Subcategory:*" |

Quoted 7 lines of code…
>     Where-Object { $_ -notlike 'Category/Subcategory*' } |
>         ForEach-Object {
>     # The categories do not have any space in front of them, but the subcategories do.
>     if ( $_ -like " *" )
>     {
>         $validSubcategory += $_.trim()
>     }

> }
This code should be contained within its own function.
I suggest putting in a function like this:
$script:validSubcategoryList = $null

function Get-ValidSubcategoryList
{
    [OutputType([String[]])]
    [CmdletBinding()]
    param ()

    if ($null -eq $script:validSubcategoryList)
    {
        # Initialize list via the script variable here
    }

    return $script:validSubcategoryList
}

Then in Test-ValidSubcategory you would call Get-ValidSubcategoryList rather than the script variable directly.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 12 at r20 (raw file):

Subcategory

Name

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 68 at r20 (raw file):

Subcategory

Name

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 115 at r20 (raw file):

Subcategory

Name

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 233 at r20 (raw file):

SubCategory

Name

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.schema.mof, line 5 at r20 (raw file):

The Subcategory to be configured

The name of the subcategory to be configured

Tests/Integration/AuditPolicyResourceHelper.Integration.Tests.ps1, line 220 at r20 (raw file):

        }
    }
}

Missing a newline at end of file.
(GitHub won't like this)

Comments from Reviewable

Review status: 18 of 21 files reviewed at latest revision, 10 unresolved discussions.

DSCResources/AuditPolicyResourceHelper.psm1, line 135 at r20 (raw file):

if ( $script:validSubcategory -contains $Name )

Quoted 7 lines of code…
>     {
>         return $true
>     }
>     else
>     {
>         return $false
>     }

    return ($script:validSubcategory -contains $Name)

Does the name have to be case-sensitive?
If not you could use -icontains instead
Also, this function needs [OutputType([Boolean])] above the [CmdletBinding()] attribute

Comments from Reviewable

Thanks for all of the help and ideas. I have learned a ton.

Review status: 14 of 22 files reviewed at latest revision, 10 unresolved discussions.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 12 at r20 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > Subcategory
>
> Name

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 68 at r20 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > Subcategory
>
> Name

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 115 at r20 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > Subcategory
>
> Name

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.psm1, line 233 at r20 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > SubCategory
>
> Name

Done.

DSCResources/MSFT_AuditPolicySubcategory/MSFT_AuditPolicySubcategory.schema.mof, line 5 at r20 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > The Subcategory to be configured
>
> The name of the subcategory to be configured

Done.

Tests/Integration/AuditPolicyResourceHelper.Integration.Tests.ps1, line 220 at r20 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> Missing a newline at end of file.
> (GitHub won't like this)

Done.

DSCResources/AuditPolicyResourceHelper.psm1, line 72 at r20 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > $localizedData
>
> This variable is no longer initialized in this file.
> Occurs below in this function as well.
>
> This function will crash if it hits this line and the variable has not been declared.
>
> You will probably still need a separate .strings file for this module.
> You can then call `Get-LocalizedData` at the top of this function since it isn't used in any other functions.

I moved Get-LocalizedData to the top of the helper module so that Invoke-Auditpol could use it and called it from within Invoke-Audtpol. I moved the helper function to a dedicated folder so the culture string file wasn't just sitting in the DscResources folder. I hadn't seen any other resources like that, so let me know if this is ok. I updated the Get-LocalizedData function to account for the new module location. I added a second parameterset to account for the helper module not being a DscResource by just updating the variables as if it were a resource.  Then I updated the resources to point to the new helper module location.
Let me know what you think.

DSCResources/AuditPolicyResourceHelper.psm1, line 116 at r20 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > \# Populating $validSubcategory uses Invoke-AuditPol, so it needs to come after the definition
> > Invoke-AuditPol -Command List -SubCommand "Subcategory:*" |
> >     Where-Object { $_ -notlike 'Category/Subcategory*' } |
> >         ForEach-Object {
> >     # The categories do not have any space in front of them, but the subcategories do.
> >     if ( $_ -like " *" )
> >     {
> >         $validSubcategory += $_.trim()
> >     }
> > }
>
> This code should be contained within its own function.
>
> I suggest putting in a function like this:
> ```powershell
> $script:validSubcategoryList = $null
>
> function Get-ValidSubcategoryList
> {
>     [OutputType([String[]])]
>     [CmdletBinding()]
>     param ()
>
>     if ($null -eq $script:validSubcategoryList)
>     {
>         # Initialize list via the script variable here
>     }
>
>     return $script:validSubcategoryList
> }
> ```
>
> Then in Test-ValidSubcategory you would call `Get-ValidSubcategoryList` rather than the script variable directly.

Nice, I never thought to use a function to keep from regenerating the list over and over. That is going in the memory bank.

DSCResources/AuditPolicyResourceHelper.psm1, line 135 at r20 (raw file):
Previously, kwirkykat (Katie Keim) wrote…
> > if ( $script:validSubcategory -contains $Name )
> >     {
> >         return $true
> >     }
> >     else
> >     {
> >         return $false
> >     }
>
> ```powershell
>     return ($script:validSubcategory -contains $Name)
> ```
>
> Does the name have to be case-sensitive?
> If not you could use `-icontains` instead
>
> Also, this function needs `[OutputType([Boolean])]` above the `[CmdletBinding()]` attribute

Added icontains and OutputType

Comments from Reviewable

This module is now ready to go since it has 3 approvals from the DSC Resource Kit Design Team! :tada:
@athaynes You can transfer the repo to me (@kwirkykat) or directly to (@nanalakshmanan). I will just transfer it over to him since he's the one with PowerShell org admin rights. Then he will add it to the PowerShell org with you as a maintainer.
Great work! Thanks Adam!!!
@zachal @mbreakey3

Reviewed 3 of 29 files at r5, 2 of 19 files at r11, 1 of 10 files at r13, 2 of 11 files at r18, 9 of 9 files at r21.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

DSCResources/AuditPolicyResourceHelper.psm1, line 72 at r20 (raw file):
Previously, athaynes (Adam Haynes) wrote…
> I moved Get-LocalizedData to the top of the helper module so that Invoke-Auditpol could use it and called it from within Invoke-Audtpol. I moved the helper function to a dedicated folder so the culture string file wasn't just sitting in the DscResources folder. I hadn't seen any other resources like that, so let me know if this is ok. I updated the Get-LocalizedData function to account for the new module location. I added a second parameterset to account for the helper module not being a DscResource by just updating the variables as if it were a resource.  Then I updated the resources to point to the new helper module location.
>
> Let me know what you think.
>

That works

Comments from Reviewable

Awesome! @kwirkykat @nanalakshmanan Do you want me to merge everything together first or just transfer as is?

I found my answer in the contribution guide.