xDscWebService: FIX The IIS apppool account and credentials cannot be set and fixing some bugs from PR #614 #613

tmeckel · 2019-04-06T12:15:39Z

Pull Request (PR) description

This PR contains a fix for issue #463 where the IIS application pool account cannot be set for a DSC Pull Server.

This Pull Request (PR) fixes the following issues

Fixes xDscWebService: The IIS apppool account and credentials cannot be set #463
Fixes some bugs introduced with PR xDSCWebservice: FIX Firewall Exception #614

Task list

This PR is currently marked as draft because work isn't finished yet. The following task list will be updated.

Added an entry under the Unreleased section of the change log in
CHANGELOG.md. Entry should say what was changed, and how that affects
users (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 Resource Testing Guidelines.
Integration tests added/updated (where possible). See
DSC Resource Testing Guidelines.
New/changed code adheres to
DSC Resource Style Guidelines and Best Practices

This change is

codecov-io · 2019-04-06T12:53:20Z

Codecov Report

Merging #613 into dev will increase coverage by 1%.
The diff coverage is 70%.

@@         Coverage Diff         @@
##            dev   #613   +/-   ##
===================================
+ Coverage    73%    75%   +1%     
===================================
  Files        28     28           
  Lines      4164   4187   +23     
  Branches      5      4    -1     
===================================
+ Hits       3066   3147   +81     
+ Misses     1093   1036   -57     
+ Partials      5      4    -1

…ng the code

* Fixed firewall handling * Restructured unit tests for MSFT_xDSCWebService so that it can be tested more thoroughly in the future

…ployed PullServer

…rvice

…s.ps1

…licaiton pool handling

…sample

….ps1

…viceAppPool.ps1

…mple_xDscWebServiceAppPool.ps1

tmeckel · 2019-05-05T09:47:07Z

@mhendric @PlagueHO @johlju the PR is now ready. Would be nice if one of you guys would do a review.

Some additional remarks about this PR. Aside the actual reason for this PR to have issue #463 fixed, I took the chance to expand the test coverage for xDscWebService. Especially the new code paves the way for more indepth unit testing of the resource because I removed the mock New-PSWSEndpoint which limits the unit tests to the xDscWebService resource itsself and not to the existing submodules which do the actual work and have never been under unit testing up to now.

While working on the enahncement of the testing for xDscWebService I discovered some bugs that I introduced with PR #614, which @mhendric already merged into DEV. So I think before we can release another version of *xPSDesiredStateConfiguration we should get this PR merged so that those bugs get fixed.

To prevent any regressions on the handling of the firewall and application pool features of xDscWebService I added additional integration tests, which now passes successfully with this PR.

johlju

Reviewed 3 of 11 files at r2.
Reviewable status: 3 of 11 files reviewed, 3 unresolved discussions (waiting on @tmeckel)

README.md, line 174 at r2 (raw file):

module

End the sentence with full stop (.)

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 25 at r2 (raw file):

-f $port

This does not work since the string does not contain {0}. Throughout.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 39 at r2 (raw file):

$DefaultAppPoolName

Where is this set?

tmeckel

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

README.md, line 174 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

module
End the sentence with full stop (.)

Done.

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 25 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-f $port
This does not work since the string does not contain {0}. Throughout.

I know I had to revert the format string when I discovered that would break backward compatibility. In my changed version I added the port for the Firewall rule at the end of the display name so that's easier to distinguish which rule belongs to which PullServer instance. But for ensuring backwards compatibility I removed the {0} from the format string so that the parameter won't be expanded anymore. But since I didn't want to change my code the -f $port still there. I don't think it's an issue because PowerShell (basically .NET) will simply return the 'static' string. If you insist I can remove the the left over -f of course.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 39 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$DefaultAppPoolName
Where is this set?

It's defined in the PSWSIISEndpoint.psm1 (sub-)module of MSFT_xDSCWebService

https://github.com/PowerShell/xPSDesiredStateConfiguration/blob/de65a11e71449ea457fc1a88824bbc87551afafd/DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1#L4

and exported as module variable at the end of the module file using Export-ModuleMember

https://github.com/PowerShell/xPSDesiredStateConfiguration/blob/de65a11e71449ea457fc1a88824bbc87551afafd/DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1#L1004

johlju

I just took a quick look at this, realized that what I did the last time. I have just review a couple of the files. I need to dig into this PR deeper. Try to do that as soon as possible.

Reviewed 1 of 1 files at r3.
Reviewable status: 3 of 11 files reviewed, 3 unresolved discussions (waiting on @tmeckel)

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 25 at r2 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

I know I had to revert the format string when I discovered that would break backward compatibility. In my changed version I added the port for the Firewall rule at the end of the display name so that's easier to distinguish which rule belongs to which PullServer instance. But for ensuring backwards compatibility I removed the {0} from the format string so that the parameter won't be expanded anymore. But since I didn't want to change my code the -f $port still there. I don't think it's an issue because PowerShell (basically .NET) will simply return the 'static' string. If you insist I can remove the the left over -f of course.

I think we need to remove the -f, first because it is unnecessary code, and secondly because otherwise another contributor might think it's a bug in the localized string and add back {0} to it.

Throughout.

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 2 at r3 (raw file):

'DSCPullServer_IIS_Port'

I'm not familiar with the prior change. Just want to understand. Before the initial PR this was the name of the firewall rule that was created. This PR fixes it back to the original firewall rule name so it is not a breaking change?
Or did the last release of this resource never create a firewall rule?

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 39 at r2 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

It's defined in the PSWSIISEndpoint.psm1 (sub-)module of MSFT_xDSCWebService

https://github.com/PowerShell/xPSDesiredStateConfiguration/blob/de65a11e71449ea457fc1a88824bbc87551afafd/DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1#L4

and exported as module variable at the end of the module file using Export-ModuleMember

https://github.com/PowerShell/xPSDesiredStateConfiguration/blob/de65a11e71449ea457fc1a88824bbc87551afafd/DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1#L1004

Curious. Why is this exporting that variable name? What is it meant to be used for?

johlju · 2019-05-09T15:49:45Z

@tmeckel I have some time now again, I will continue with the review. I have long days at the day job, so I don't have a lot of time to put in reviewing this week. :)

johlju

Reviewed 2 of 11 files at r2.
Reviewable status: 5 of 11 files reviewed, 9 unresolved discussions (waiting on @tmeckel)

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 2 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'DSCPullServer_IIS_Port'
I'm not familiar with the prior change. Just want to understand. Before the initial PR this was the name of the firewall rule that was created. This PR fixes it back to the original firewall rule name so it is not a breaking change?
Or did the last release of this resource never create a firewall rule?

I saw in PR #614 that this was the case, PR #614 introduced a breaking change. This reverts that breaking change. This comment looks good to me. 🙂

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 161 at r3 (raw file):

ApplicationPoolName          = $ApplicationPoolName

Shouldn't we return this value from the $webSite.applicationPool so we are returning the actual ApplicationPool if it is not set to the desired state yet?

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 4 at r3 (raw file):

New-Variable -Name DefaultAppPoolName  -Value 'PSWS' -Option ReadOnly -Force -Scope Script

Why do we have this? Why is not the helper functions (these cmdlets in this module) called with the app pool name 'PSWS' for the appPool parameter name when that application pool name should be used (when the user have not specified the app pool name in the configuration)?

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 261 at r3 (raw file):

Write-Error

Maybe we should use throw here instead.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 270 at r3 (raw file):

Quoted 7 lines of code…

                    <#
                    There may be running requests, wait a little
                    I had an issue where the files were still in use
                    when I tried to delete them
                    #>
                    Write-Verbose -Message 'Waiting for IIS to stop website'
                    Start-Sleep -Milliseconds 1000

Should we loop with a timeout and verify that the site is actually stope. If not, throw an error if the timeout is reached. Would be good for the timeout to be able to be configurable when calling this function.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 353 at r3 (raw file):

Write-Verbose -Message "Unable to delete application pool [$appPool] because it's still bound to a site or application"

Shouldn't we throw here`

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 359 at r3 (raw file):

Write-Verbose -Message "ApplicationPool name is different from built in name [$DefaultAppPoolName]. Unable to delete."

I think this should be a Warning message. Also maybe the warning message should say how they should remove the application pool account if they want to?

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 1006 at r3 (raw file):

-Variable DefaultAppPoolName

See other comment. I'm curious why we are exporting this variable name?

johlju

No new comments, just agree to open issues for some of the previous comments.
There was one previous comment left, the removing -f port one. Did not see a change there.

After that I looks good to me.

Reviewed 11 of 11 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johlju and @tmeckel)

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

Previously, tmeckel (Thomas Meckel) wrote…

I agree with you. We should deprecate the functionality which creates an IIS Application Pool inside xDSCWebService, like we did with the Firewall stuff. But I think we should open an issue to have this discussed openly in the community and managed separately from this PR.

Sounds good to open an issue.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 261 at r3 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

Okaaay ... something learned! Subtle difference here between Write-Error and throw. So I'll change this to a throw then.

For my personal records: what would you recommend or what pattern do you follow on a new Powershell project/module/script? I mean when would you use the Write-Error stuff and when throw?

Honestly I have never used Write-Error, I always use throw. I don't see a scenario where I would want to use Write-Error. Suggest you ask the question in the Slack channel. Interested in the answer myself. 🙂

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 270 at r3 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

Sounds good and reasonable for me but currently xDSCWebService does not support an option to control such a timeout value, i.e. you cannot specifiy this in a configuration. So I think this should be something to be managed in different issue and pr respectively.

Sounds good, please write an issue for this.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 359 at r3 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

Done.

We maintainers appreciate to awesome work you put in! 🙇‍♂️

Examples/Sample_xDscWebServiceAppPool.ps1, line 1 at r3 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

What about Sample_xDscWebService_BestPractice.ps1 or Sample_xDscWebService_Preferred.ps1

'Preferred' sounds best, as the other way are more or less deprecated. 'Preferred' might best describe the example that shows the method that will be used going forward.

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 75 at r3 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

Should be able to call Get-DscConfiguration without throwing
Yeeeee ... that's all over the place in the xPSDesiredStateConfiguration lol not only in xDSCWebService. But yes I can add an issue to in which this fact is addressed.

Yes, I'm okay with just submitting an issue to track that.

…scWebService_Preferred.ps1

tmeckel

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johlju)

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

Previously, johlju (Johan Ljunggren) wrote…

Sounds good to open an issue.

#619

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 25 at r2 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

I can understand und follow your opinion on this. I'll remove the formatting code.

Done.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 261 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Honestly I have never used Write-Error, I always use throw. I don't see a scenario where I would want to use Write-Error. Suggest you ask the question in the Slack channel. Interested in the answer myself. 🙂

I replaced all Write-Errorwith throw and I though about what you wrote about Write-Error. I think Write-Errorshould/could be used in scripts not in modules. That's the only intended use I can see now. And I'll change my own coding stlye from now on.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 270 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Sounds good, please write an issue for this.

#620

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 359 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We maintainers appreciate to awesome work you put in! 🙇‍♂️

Perhaps I wrote my last sentence in my answer a bit misleading. If I wouldn't had fun working on it, I'd never have done it, but there are other bugs in other resources that should be fixed with higher priority than making xDSCWebService a first class quality resource although it's end of the lifetime is foreseeable.

Examples/Sample_xDscWebServiceAppPool.ps1, line 1 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'Preferred' sounds best, as the other way are more or less deprecated. 'Preferred' might best describe the example that shows the method that will be used going forward.

Changed it to Sample_xDscWebService_Preferred.ps1

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 75 at r3 (raw file):

Get-DscConfiguration -ErrorAction Stop
#621

johlju

Just one more comment. 🙂

Reviewed 6 of 6 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tmeckel)

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 56 at r5 (raw file):

$ruleName = $FireWallRuleDisplayName -f $port

This should also be resolved. Please resolve throughput. Not sure if there are any more?

tmeckel

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

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 56 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$ruleName = $FireWallRuleDisplayName -f $port
This should also be resolved. Please resolve throughput. Not sure if there are any more?

Hmmmm ... I'm abit baffled ... I pushed this change already :-O

92c2757

johlju

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

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 56 at r5 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

Hmmmm ... I'm abit baffled ... I pushed this change already :-O

92c2757

That was on line 25, this is for the line 56, and also see the same problem on line 96. 🙂

I thought you would do a search and replace, so didn't comment on the others at first. 🙂

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 359 at r3 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

Perhaps I wrote my last sentence in my answer a bit misleading. If I wouldn't had fun working on it, I'd never have done it, but there are other bugs in other resources that should be fixed with higher priority than making xDSCWebService a first class quality resource although it's end of the lifetime is foreseeable.

Well, I think a lot of people still use this, so these changes will benefit many I think.

…MSFT_xDSCWebService\Firewall.psm1

tmeckel

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

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 56 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

That was on line 25, this is for the line 56, and also see the same problem on line 96. 🙂

I thought you would do a search and replace, so didn't comment on the others at first. 🙂

Oh boy! Stupid me... sorry .. your're right ... just pushed another commit.

johlju

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 56 at r5 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

Oh boy! Stupid me... sorry .. your're right ... just push another commit.

No worries! 😃

johlju

Reviewable status: complete! all files reviewed, all discussions resolved

johlju · 2019-05-12T18:43:34Z

@mhendric @PlagueHO can anyone of you look at this and merge it? This looks good to me as far as I can see.

PlagueHO · 2019-05-12T19:11:00Z

@johlju - will get onto this tonight unless @mhendric gets there first! 😁

PlagueHO · 2019-05-13T08:24:07Z

On this now.

PlagueHO

Hi @mhendric, great stuff! I did comment on a lot of code that you didn't change, sorry about that. But it does seem like this module needs a bit of TLC as it is got some consistency issues. I think many of the consistency and style issues can be addressed in future PR's.

Reviewed 2 of 11 files at r2, 1 of 11 files at r4, 3 of 6 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 46 unresolved discussions (waiting on @tmeckel)

DSCPullServerSetup/PullServerDeploymentVerificationTest/PullServerSetupTests.ps1, line 32 at r6 (raw file):

        # Skip all tests if web.config is not found
        if (-not (Test-Path $DscWebConfigPath)){

Needs a named parameter here.

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 23 at r6 (raw file):

    Write-Verbose -Message 'Disable Inbound Firewall Notification'
    $null = & $script:netsh advfirewall set currentprofile settings inboundusernotification disable

FYI, I note that we're checking for native cmdlets and using them if available inRemove-PullServerFirewallConfiguration. Is it worth using the same method here? It will take a little bit more PS Code though... I'm OK with leaving it for now. But it might be worth adding simple helper functions for each of the instructions that try to use the native Cmdlets where possible.

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 97 at r6 (raw file):

    Write-Verbose -Message "Testing Firewall Rule for port $Port"
    $ruleName = $FireWallRuleDisplayName
    $result = & $script:netsh advfirewall firewall show rule name=$ruleName | Select-String -Pattern "LocalPort:\s*$Port"

FYI, we probably want to try and use the native cmdlets here as well when possible as it'll be much more reliable. But this can be done in a future update.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 81 at r6 (raw file):

    )

    # If Certificate Subject is not specified then a value for CertificateThumbprint must be explicitly set instead.

Should be a block comment here.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 127 at r6 (raw file):

        $urlPrefix = $website.bindings.Collection[0].protocol + "://"

        $ipProperties = [System.Net.NetworkInformation.IPGlobalProperties]::GetIPGlobalProperties()

Add blank line after this one.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 345 at r6 (raw file):

        Write-Warning -Message $LocalizedData.ConfigFirewallDeprecated
    }
    # If the Pull Server Site should be bound to the non default AppPool

Should be a block comment here and add blank line before this one (surround code blocks in blank lines).

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 366 at r6 (raw file):

    $language = $cultureInfo.TwoLetterISOLanguageName

    # the two letter iso languagename is not actually implemented in the source path, it's always 'en'

Start comment with capital 'T'

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 403 at r6 (raw file):

            }

            # there is a web site, but there shouldn't be one

Start comment with capital 'T'

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 404 at r6 (raw file):

            # there is a web site, but there shouldn't be one
            Write-Verbose -Message "Removing web site [$EndpointName]"

FYI, we should probably move all these strings out into a localization string. But leave till a future update.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 410 at r6 (raw file):

        }

        # we are done here, all stuff below is for 'Present'

Start comment with capital 'W'

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 413 at r6 (raw file):

        return
    }
    # ===========================================================

Divider comments adds no value. Instead regions should be used.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 415 at r6 (raw file):

    # ===========================================================

    Write-Verbose -Message "Create the IIS endpoint"

Single quotes around this string.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 443 at r6 (raw file):

                Add-PullServerFirewallConfiguration -Port $port
            }
        }

Needs blank line after this one.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 455 at r6 (raw file):

    Update-LocationTagInApplicationHostConfigForAuthentication -WebSite $EndpointName -Authentication "windows"

    if($SqlProvider)

Space after if.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 475 at r6 (raw file):

    else
    {
        if($isDownlevelOfBlue)

Space after if.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 498 at r6 (raw file):

    # Create the application data directory calculated above
    $null = New-Item -path $DatabasePath -itemType 'directory' -Force

Uppercase P in -Path and throughout. Same with other parameters (e.g -itemType)

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 500 at r6 (raw file):

    $null = New-Item -path $DatabasePath -itemType 'directory' -Force

    $null = New-Item -path "$ConfigurationPath" -itemType 'directory' -Force

Quotes around $ConfigurationPath aren't required

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 504 at r6 (raw file):

    PSWSIISEndpoint\Set-AppSettingsInWebconfig -path $PhysicalPath -key 'ConfigurationPath' -value $configurationPath

    $null = New-Item -path "$ModulePath" -itemType 'directory' -Force

Quotes around $ModulePath aren't required

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 508 at r6 (raw file):

    PSWSIISEndpoint\Set-AppSettingsInWebconfig -path $PhysicalPath -key 'ModulePath' -value $ModulePath

    $null = New-Item -path "$RegistrationKeyPath" -itemType 'directory' -Force

Quotes around $RegistrationKeyPath aren't required.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 512 at r6 (raw file):

    PSWSIISEndpoint\Set-AppSettingsInWebconfig -path $PhysicalPath -key 'RegistrationKeyPath' -value $registrationKeyPath

    if($AcceptSelfSignedCertificates)

Space after if.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 657 at r6 (raw file):

    {
        Write-Verbose -Message 'Check Ensure'
        if(($Ensure -eq 'Present' -and $null -eq $website))

Add blank line before line and space after if

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 663 at r6 (raw file):

            break
        }
        if(($Ensure -eq 'Absent' -and $null -ne $website))

Add blank line before line and space after if

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 669 at r6 (raw file):

            break
        }
        if(($Ensure -eq 'Absent' -and $null -eq $website))

Add blank line before line and space after if

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 675 at r6 (raw file):

            break
        }
        # the other case is: Ensure and exist, we continue with more checks

Start comment with capital letter.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 679 at r6 (raw file):

        Write-Verbose -Message 'Check Port'
        $actualPort = $website.bindings.Collection[0].bindingInformation.Split(":")[1]
        if ($Port -ne $actualPort)

Add blank line before line

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 687 at r6 (raw file):

        Write-Verbose -Message 'Check Application Pool'
        if ($ApplicationPoolName -ne $website.applicationPool)

Add blank line before line

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 20 at r6 (raw file):

        [ValidateNotNullOrEmpty()]
        [System.String]
        $appPool,

FYI, All these parameter names should start with a capital letter, but this will require changes to all the places it is called. So perhaps leave this to another update.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 243 at r6 (raw file):

    {
        $site = Get-Website -Name $siteName
    }

blank line after line.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 252 at r6 (raw file):

                Write-Verbose -Message "Starting IIS Website [$($site.name)]"
                Start-Website -Name $site.name
            }

blank line after line.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 259 at r6 (raw file):

                    Write-Verbose -Message "Stopping WebSite $($site.name)"
                    $website = Stop-Website -Name $site.name -Passthru
                    if ('Started' -eq $website.state)

blank line before line.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 265 at r6 (raw file):

                    <#
                    There may be running requests, wait a little

Indent comment

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 270 at r6 (raw file):

                    #>
                    Write-Verbose -Message 'Waiting for IIS to stop website'
                    Start-Sleep -Milliseconds 1000

This looks like it is pretty risky as we'll never know if 1000ms is going to be "enough time". Suggest implementing a loop with a timeout and try catch to do this in a more resilient way.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 276 at r6 (raw file):

                    Write-Verbose -Message "IIS Website [$($site.name)] already stopped"
                }
            }

blank line after line

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 279 at r6 (raw file):

            'Remove'
            {
                Update-Site -site $site -siteAction Stop

It looks like this can be replaced with Stop-WebSite -Name $site.name -ErrorAction SilentlyContinue

I think that Update-Site actually increases complexity without adding any value. It just abstracts simple functions into a command function but doesn't add anything. I'd get rid of this entirely.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 306 at r6 (raw file):

        [ValidateNotNullOrEmpty()]
        [System.String]
        $appPool

Capital first letter for parameters.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 338 at r6 (raw file):

        [ValidateNotNullOrEmpty()]
        [System.String]
        $appPool

Capital first letter for parameters.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 346 at r6 (raw file):

        if (Test-Path -Path "IIS:\AppPools\$appPool")
        {
            $cntBindings = (Get-AppPoolBinding -apppool $appPool | Measure-Object).Count

Can we use a variable name without abbreviation? E.g. $bindingCount

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 347 at r6 (raw file):

        {
            $cntBindings = (Get-AppPoolBinding -apppool $appPool | Measure-Object).Count
            if (0 -ge $cntBindings)

Blank line before this one.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 485 at r6 (raw file):

        [ValidateNotNullOrEmpty()]
        [System.String]
        $site,

FYI, see previous comments about using capital first letters for parameter name.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 533 at r6 (raw file):

        Write-Verbose -Message 'Set App Pool Properties'
        $appPoolIdentity = 4
        if ($applicationPoolIdentityType)

Blank line before this one.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 542 at r6 (raw file):

                    $appPoolIdentity = 0
                }
                'LocalService'

Blank line before this one.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 546 at r6 (raw file):

                    $appPoolIdentity = 1
                }
                'NetworkService'

Blank line before this one.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 550 at r6 (raw file):

                    $appPoolIdentity = 2
                }
                'ApplicationPoolIdentity'

Blank line before this one.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 554 at r6 (raw file):

                    $appPoolIdentity = 4
                }
                default {

Blank line before this one.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 560 at r6 (raw file):

        }

        $appPoolItem = Get-Item IIS:\AppPools\$appPool

Needs -Path parameter.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 569 at r6 (raw file):

    Write-Verbose -Message 'Add and Set Site Properties'
    if ($certificateThumbPrint -eq 'AllowUnencryptedTraffic')

blank line before this one.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 642 at r6 (raw file):

        [ValidateNotNullOrEmpty()]
        [System.String]
        $site = 'PSWS',

FYI, see previous comments about capital first letter for parameter names.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 755 at r6 (raw file):

    $svcName = Split-Path $svc -Leaf
    $protocol = 'https:'
    if ($certificateThumbPrint -eq 'AllowUnencryptedTraffic')

Blank line before this one.

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 763 at r6 (raw file):

    $cimInstance = Get-CimInstance -ClassName Win32_ComputerSystem -Verbose:$false

    Write-Verbose ("Setting up endpoint at - $protocol//" + $cimInstance.Name + ':' + $port + '/' + $svcName)

Needs Parameter name.

Also, using string concatenation is not recommended. Can we use a format string instead?

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 816 at r6 (raw file):

    # Get the site to remove
    $site = Get-Website -Name $siteName
    if ($site)

Add blank line before

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 824 at r6 (raw file):

        # Remove the actual site.
        Update-Site -site $site -siteAction Remove

Can we remove Update-Site completely? It looks like this can be replaced with:
Remove-Website -Name $site.name

DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1, line 827 at r6 (raw file):

        # Remove the files for the site
        If (Test-Path -Path $filePath)

Lower case 'I' in if.

tmeckel · 2019-05-13T10:03:46Z

Hey @PlagueHO I'll go through your comments later this afternoon.
And yes you blame me here for the old code I only copied around LOL 😜

tmeckel · 2019-05-13T10:06:06Z

Hi @mhendric, great stuff! I did comment on a lot of code that you didn't change, sorry about that. But

And Mike (@mhendric ) gonna take the credit now? 😆 😆 😆

tmeckel

Reviewable status: all files reviewed, 46 unresolved discussions (waiting on @PlagueHO)

DSCPullServerSetup/PullServerDeploymentVerificationTest/PullServerSetupTests.ps1, line 32 at r6 (raw file):