Add UNC Path support to WebVirtualDirectory

[RWebster-Noble]

Pull Request (PR) description

If you tried to configure a WebVirtualDirectory with a UNC PhysicalPath you would get the error
Parameter 'PhysicalPath' should point to existing path.

This fixes this error and also introduces a Credential parameter for IIS to use to access the path.

Parts of this code is based on #428

This Pull Request (PR) fixes the following issues

Fixes #94

Task list

• Added an entry to the change log under the Unreleased section of the file CHANGELOG.md.
  Entry should say what was changed and how that affects users (if applicable), and
  reference the issue being resolved (if applicable).
• Resource documentation added/updated in README.md.
• Resource parameter descriptions added/updated in README.md, schema.mof and comment-based
  help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Community Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
• New/changed code adheres to DSC Community Style Guidelines.

---

This change is 

[RWebster-Noble]

If the Credential parameter is not provided or is $null then this will not clear existing credentials. This is intentional to maintain backwards compatibility.

This also means that there is no way to use this to clear credentials. I couldn't think of a clean way to implement this (without breaking backwards compatibility) but I am very welcome to suggestions.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88%. Comparing base (cfced5c) to head (65f8bed).
Report is 1 commits behind head on main.

Additional details and impacted files

@@         Coverage Diff         @@
##           main   #634   +/-   ##
===================================
  Coverage    88%    88%           
===================================
  Files        15     15           
  Lines      1925   1952   +27     
===================================
+ Hits       1698   1726   +28     
+ Misses      227    226    -1     

Files	Coverage Δ
...C_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1	94% <100%> (+4%)	⬆️

[RWebster-Noble]

The failing checks don't appear to be relevant to anything I have changed. @johlju can you advice?

Also the code coverage didn't update after I added an additional test.

[johlju]

The failing checks don't appear to be relevant to anything I have changed. @johlju can you advice?

Most likely because there are two Unreleased sections in the CHANGELOG.md. Make sure to to only have the one that existed and put new entries under it.

Also the code coverage didn't update after I added an additional test.

Don't see the additional test actually testing the code that are missed. To get coverage for line 302-306 the mock of Get-WebVirtualDirectory need to return and empty array and also have the parameter Ensure set to Absent. 🤔

[RWebster-Noble]

Thanks for your advice @johlju. I've fixed the changelog and managed to get the code test coverage up to 100% of the new code (and even got some additional test coverage on the existing code)

Some tests cover multiple aspects; if you prefer, I can split them into separate tests.

[johlju]

Reviewed 3 of 6 files at r1, 1 of 2 files at r4, all commit messages.
Reviewable status: 4 of 6 files reviewed, 8 unresolved discussions (waiting on @RWebster-Noble)

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 41 at r4 (raw file):

        [Parameter()]
        [pscredential]

We should change this to full type name [System.Management.Automation.PSCredential]. Throughout the Test- and Set-function too.

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 60 at r4 (raw file):

        $Ensure = 'Present'

        if ($WebApplication.Length -gt 0)

We should probably change this to if (-not [System.String]:IsNullOrEmpty($WebApplication)). Or just switch the if and else-block then we can skip the -not to make it easier to read.

Throughout below in Test- and Set-function.

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 62 at r4 (raw file):

        if ($WebApplication.Length -gt 0)
        {
            $ItemPath = "IIS:Sites\$Website\$WebApplication\$Name"

We should use camelCase on local variables, so $itemPath, throughout below. Throughout below in Test- and Set-function.

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 69 at r4 (raw file):

        }

        $userName = (Get-ItemProperty $ItemPath -Name userName).Value

Is the property names using camelCase? Or should it be 'UserName'?

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 70 at r4 (raw file):

        $userName = (Get-ItemProperty $ItemPath -Name userName).Value
        if ($userName -ne '')

Can we use if (-not [System.String]:IsNullOrEmpty($userName)) here?

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 72 at r4 (raw file):

        if ($userName -ne '')
        {
            #$password = (Get-ItemProperty $ItemPath -Name password).Value

We should remove this commented code if it not used.

---

source/DSCResources/DSC_WebVirtualDirectory/en-US/DSC_WebVirtualDirectory.strings.psd1 line 4 at r4 (raw file):

ConvertFrom-StringData -StringData @'
    VerboseGetTargetResource               = Get-TargetResource has been run.
    VerboseSetTargetPhysicalPath           = Updating PhysicalPath and Credential for Web Virtual Directory '{0}'.

The two properties are updated seperatly from each other, so there should probably be two separate localization messages, it would otherwise look like it update both properties every time when it not necessarily does.

---

source/DSCResources/DSC_WebVirtualDirectory/en-US/DSC_WebVirtualDirectory.strings.psd1 line 7 at r4 (raw file):

    VerboseSetTargetCreateVirtualDirectory = Creating new Web Virtual Directory '{0}'.
    VerboseSetTargetRemoveVirtualDirectory = Removing existing Virtual Directory '{0}'.
    VerboseTestTargetFalse                 = Physical path '{0}' and Credential for Web Virtual Directory '{1}' is not in desired state.

The word 'and' should probbakly be 'or' if we should keep this as one message. But it would probably be better that this one is also split into two messages, one for each property being tested. 🤔

[RWebster-Noble]

Reviewed 1 of 2 files at r4.
Reviewable status: 3 of 6 files reviewed, 8 unresolved discussions (waiting on @johlju)

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 41 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should change this to full type name [System.Management.Automation.PSCredential]. Throughout the Test- and Set-function too.

Done.

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 60 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should probably change this to if (-not [System.String]:IsNullOrEmpty($WebApplication)). Or just switch the if and else-block then we can skip the -not to make it easier to read.

Throughout below in Test- and Set-function.

Done.

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 62 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should use camelCase on local variables, so $itemPath, throughout below. Throughout below in Test- and Set-function.

This code block was copied from the existing code and I did notice the inconsistent vatable casing.
I have updated it as requested.

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 69 at r4 (raw file):
I thought it look inconsistent too but I initially went with keeping the same casing consistent with output of Get-ItemProperty:

> Get-ItemProperty "IIS:\Sites\Default Web Site\vdir" -Name UserName
...
Name                        : userName

I just tested it and it's not case sensitive so would you still prefer UserName (I have this updated it as requested)?

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 70 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we use if (-not [System.String]:IsNullOrEmpty($userName)) here?

Done.

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 72 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should remove this commented code if it not used.

Done.

[RWebster-Noble]

Reviewable status: 3 of 6 files reviewed, 8 unresolved discussions (waiting on @johlju)

---

source/DSCResources/DSC_WebVirtualDirectory/en-US/DSC_WebVirtualDirectory.strings.psd1 line 4 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The two properties are updated seperatly from each other, so there should probably be two separate localization messages, it would otherwise look like it update both properties every time when it not necessarily does.

Done.

---

source/DSCResources/DSC_WebVirtualDirectory/en-US/DSC_WebVirtualDirectory.strings.psd1 line 7 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The word 'and' should probbakly be 'or' if we should keep this as one message. But it would probably be better that this one is also split into two messages, one for each property being tested. 🤔

Done.

[johlju]

The tests look good to me. I just glanced them but they seem to cover the changes, and have several aspects in the same tests is OK.

Just a minor thing left.

Reviewed 1 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RWebster-Noble)

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 62 at r4 (raw file):

Previously, RWebster-Noble wrote…

This code block was copied from the existing code and I did notice the inconsistent vatable casing.
I have updated it as requested.

Great! Yes, old code might not have been fixed according to style guideline, but good if we don't add more to fix. 🙂

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 69 at r4 (raw file):

Previously, RWebster-Noble wrote…

I thought it look inconsistent too but I initially went with keeping the same casing consistent with output of Get-ItemProperty:

> Get-ItemProperty "IIS:\Sites\Default Web Site\vdir" -Name UserName
...
Name                        : userName

I just tested it and it's not case sensitive so would you still prefer UserName (I have this updated it as requested)?

Since the the property returned is using camelCase I good with either. Keep as-is or change it as you prefer 🙂

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 160 at r5 (raw file):

        {
            Write-Verbose -Message ($script:localizedData.VerboseSetTargetCreateVirtualDirectory -f $Name)
            if ([bool]([System.Uri]$PhysicalPath).IsUnc)

Can we change to if ([System.Boolean] ([System.Uri] $PhysicalPath).IsUnc). Full type name and space after both type names.

But I think we could instead do if ($true -eq ([System.Uri] $PhysicalPath).IsUnc) to forced it to evaluate against a boolean. 🤔 Either way is good for me. Not sure either is better or worse. 🤔

Btw. Did not know about the IsUnc property, learned something new 🙂

[RWebster-Noble]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 160 at r5 (raw file):
I took that code from PR #428

I did think it was a bit strange but thought that there was a reason the original author wrote it like that but turns out that ([System.Uri] $PhysicalPath).IsUnc is already a System.Boolean

> (([System.Uri] "\\a").IsUnc).GetType()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     True     Boolean                                  System.ValueType

Given that wouldn't it be fine to simplify that to just to if (([System.Uri]$PhysicalPath).IsUnc)?

[johlju]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @RWebster-Noble)

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 160 at r5 (raw file):

Previously, RWebster-Noble wrote…

I took that code from PR #428

I did think it was a bit strange but thought that there was a reason the original author wrote it like that but turns out that ([System.Uri] $PhysicalPath).IsUnc is already a System.Boolean

> (([System.Uri] "\\a").IsUnc).GetType()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     True     Boolean                                  System.ValueType

Given that wouldn't it be fine to simplify that to just to if (([System.Uri]$PhysicalPath).IsUnc)?

I think it was converted to boolean because the property is not set to $false but $null when the URI is not an UNC path. But the way you written it, it should work to since it will only enter the if-block if the property IsUnc has a value other than $null or $false.

But add a space after the type name,, e.g

if (([System.Uri] $PhysicalPath).IsUnc)

[RWebster-Noble]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @johlju)

---

source/DSCResources/DSC_WebVirtualDirectory/DSC_WebVirtualDirectory.psm1 line 160 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I think it was converted to boolean because the property is not set to $false but $null when the URI is not an UNC path. But the way you written it, it should work to since it will only enter the if-block if the property IsUnc has a value other than $null or $false.

But add a space after the type name,, e.g

if (([System.Uri] $PhysicalPath).IsUnc)

Done.