Added LogCustomFields to xWebSite (Fixes #267)

[regedit32]

Modified xWebSite to include the ability to manage logging custom fields. (Fixes #267 )
Below is an example of how to add a logging custom field with the updated resource:

xWebsite NewWebsite
{
    Ensure             = 'Present'
    Name               = 'Default Web Site'
    LogCustomFields    = MSFT_xLogCustomFieldInformation
    {
        LogFieldName   = 'Original-IP'
        SourceName     = 'X-Forwarded-For'
        SourceType     = 'RequestHeader'
    }
}

---

This change is 

[johlju]

Awesoem work on this one! 😄 Just a few review comments.

---

Reviewed 4 of 7 files at r1, 1 of 2 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

README.md, line 170 at r3 (raw file):

informationin

information

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 770 at r3 (raw file):

            }

            # Update LogCustomFields if neeed

Instead of duplicating this code twice, can't we just run this once after the evaluation if the website should be created or already exists?

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 1090 at r3 (raw file):

neeed

We should have two 'e' here :) Throughout.

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 1634 at r3 (raw file):

{
    [CmdletBinding()]
    [OutputType([Microsoft.Management.Infrastructure.CimInstance])]

Should return an array of CimInstance's not just one?

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 1860 at r3 (raw file):

    Set-WebConfigurationProperty -PSPath 'MACHINE/WEBROOT/APPHOST' -Filter "system.applicationHost/sites/site[@name='$Site']/logFile/customFields" -Name "." -Value $setCustomFields

    # The second Set-WebConfigurationProperty is to handle an edge case where logfile.customFields is not updated correctly.  May be caused by a possible bug in the IIS provider

Could we instead loop twice over one of the Set-WebConfigurationProperty?

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 1860 at r3 (raw file):

    Set-WebConfigurationProperty -PSPath 'MACHINE/WEBROOT/APPHOST' -Filter "system.applicationHost/sites/site[@name='$Site']/logFile/customFields" -Name "." -Value $setCustomFields

    # The second Set-WebConfigurationProperty is to handle an edge case where logfile.customFields is not updated correctly.  May be caused by a possible bug in the IIS provider

Could you please submit an issue for this in this repo so this can be tracked. I will label it as external though, since we can't do anything about it in this repo. But once fixed we can remove the workaround.

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 2154 at r3 (raw file):

sourceName

Should we have upper 'S' on this property name? Same for the row below.

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 2156 at r3 (raw file):

            $sourceNameMatch = $customField.SourceName -eq $presentCustomField.sourceName
            $sourceTypeMatch = $customField.SourceType -eq $presentCustomField.sourceType
            if(-not ($sourceNameMatch -and $sourceTypeMatch))

Please add a space between if an the parenthesis .

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.schema.mof, line 54 at r3 (raw file):

    [Write, Description ("Use the localtime for file naming and rollover")] Boolean LoglocalTimeRollover;
    [Write, Description ("Format of the Logfiles. Only W3C supports LogFlags"), ValueMap{"IIS","W3C","NCSA"}, Values{"IIS","W3C","NCSA"}] String LogFormat;
    [Write, EmbeddedInstance("MSFT_xLogCustomFieldInformation"), Description("Custom logging field information")] String LogCustomFields[];

Could we extent this description a bit to something more like that first phrase in the README.md?

---

Tests/Integration/MSFT_xWebsite.config.ps1, line 157 at r3 (raw file):

                    LogFieldName = $Node.LogFieldName
                    SourceName   = $Node.SourceName
                    SourceType   = $Node.SourceType

Maybe we could add another SourceType for this test?

---

Tests/Unit/MSFT_xWebsite.Tests.ps1, line 84 at r3 (raw file):

$MockLogCustomFields

Could we start the variable names with lower-case? Throughout (only have to do on your changes).

I know the others aren't, but we need to start somewhere to follow the guideline 😄

---

Tests/Unit/MSFT_xWebsite.Tests.ps1, line 97 at r3 (raw file):

                truncateSize      = '1048576'
                localTimeRollover = 'False'
                customFields      = @{Collection = @($MockLogCustomFields)}

Since we can add an array maybe we should test with an array of two objects? Throughout.

---

Tests/Unit/MSFT_xWebsite.Tests.ps1, line 3529 at r3 (raw file):

            Context 'Expected behavior'{
                $Result = ConvertTo-CimLogCustomFields -InputObject $MockLogCustomFields

Maybe test this with an array?

---

Tests/Unit/MSFT_xWebsite.Tests.ps1, line 3569 at r3 (raw file):

               It 'should return True' {
                    Test-LogCustomField -Site $MockWebsiteName -LogCustomField $MockCimLogCustomFields | Should Be $True

Maybe test this with an array?

---

Tests/Unit/MSFT_xWebsite.Tests.ps1, line 3615 at r3 (raw file):

                It 'should not throw an error' {
                    { Set-LogCustomField  -Site $MockWebsiteName -LogCustomField $MockCimLogCustomFields } | Should Not Throw

Maybe test this with an array?

---

Comments from Reviewable

[regedit32]

Thanks for the feedback. I'll start working on these.

[regedit32]

Review status: 1 of 8 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

README.md, line 170 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

informationin

information

Done.

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 770 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Instead of duplicating this code twice, can't we just run this once after the evaluation if the website should be created or already exists?

I agree. I applied the same logic as the other settings in Set-TargetResource, but it is actually redundant like you pointed out. Most of the other settings in that function can probably be adjusted in the same way.

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 1090 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

neeed

We should have two 'e' here :) Throughout.

Done.

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 1634 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should return an array of CimInstance's not just one?

Done.

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 1860 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we instead loop twice over one of the Set-WebConfigurationProperty?

Done.

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 1860 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please submit an issue for this in this repo so this can be tracked. I will label it as external though, since we can't do anything about it in this repo. But once fixed we can remove the workaround.

Yes, Opened Issue #348 for this

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 2154 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

sourceName

Should we have upper 'S' on this property name? Same for the row below.

Done.

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 2156 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a space between if an the parenthesis .

Done.

---

DSCResources/MSFT_xWebsite/MSFT_xWebsite.schema.mof, line 54 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we extent this description a bit to something more like that first phrase in the README.md?

Done.

---

Tests/Integration/MSFT_xWebsite.config.ps1, line 157 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe we could add another SourceType for this test?

Done.

---

Tests/Unit/MSFT_xWebsite.Tests.ps1, line 84 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$MockLogCustomFields

Could we start the variable names with lower-case? Throughout (only have to do on your changes).

I know the others aren't, but we need to start somewhere to follow the guideline 😄

Done.

---

Tests/Unit/MSFT_xWebsite.Tests.ps1, line 97 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Since we can add an array maybe we should test with an array of two objects? Throughout.

Done.

---

Tests/Unit/MSFT_xWebsite.Tests.ps1, line 3529 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe test this with an array?

Done.

---

Tests/Unit/MSFT_xWebsite.Tests.ps1, line 3569 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe test this with an array?

Done.

---

Tests/Unit/MSFT_xWebsite.Tests.ps1, line 3615 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe test this with an array?

Done.

---

Comments from Reviewable

[johlju]

Reviewed 7 of 7 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[johlju]

@regedit32 Can you rebase this one to resolve the merge conflicts? Also, when you push next time the tests should pass, we got fixes merged for the failing tests.

[codecov-io]

Codecov Report

Merging #342 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##            dev   #342    +/-   ##
====================================
+ Coverage    89%    89%   +<1%     
====================================
  Files        14     14            
  Lines      2171   2210    +39     
====================================
+ Hits       1944   1983    +39     
  Misses      227    227

[regedit32]

@johlju done, thanks.

---

Review status: 5 of 8 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[johlju]

Just a tiny review comment left, I must have missed it before.

---

Reviewed 3 of 3 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

README.md, line 265 at r5 (raw file):

### Unreleased
* Updated **xWebSite** to include ability to manage custom logging fields

Can we remove this extra blank line?

---

Comments from Reviewable

[regedit32]

done. Did another rebase and it took care of that, thanks.

---

Review status: 7 of 8 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[regedit32]

Review status: 7 of 8 files reviewed at latest revision, 1 unresolved discussion.

---

README.md, line 265 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we remove this extra blank line?

Done.

---

Comments from Reviewable

[johlju]

---

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[johlju]

@regedit32 Awesome work on this one! Thanks!