MSFT_xDSCWebService.psm1: Applied fix for issue #460 #507
Conversation
mhendric
changed the title
Codecov Report
@@ Coverage Diff @@
## dev #507 +/- ##
===================================
- Coverage 74% 74% -1%
===================================
Files 27 27
Lines 4030 4074 +44
Branches 4 4
===================================
+ Hits 3009 3032 +23
- Misses 1017 1038 +21
Partials 4 4 |
|
@tmeckel , it look like this change does in fact introduce new issues in the corresponding Unit tests. Those will need to be fixed before this can be merged. Let us know if you need help troubleshooting those tests. Also, it looks like the encoding was changed in README.md (Notepad often does this). You can try the helper function to fix that if you like, although sometimes it gets me in more trouble than I was already in. Another option is to just revert the file to its original state, then make the change using an editor like Notepad++ which won't alter the encoding or special characters. |
|
@mhendric Seems this fix is getting a real pest now LOL Anyway ... removing the BOM from the Readme.md is an easy task. I'll correct the encoding an commit the change on the PR. And yes I used notepad to add the required lines to Readme.md and I wasn't aware of the fact that notepad changed the encoding. I only know that behavior from PowershellISE. With the failing unit tests I'm definitely a bit baffled, because the only thing what's in the PR is a check if the Okay a quick analysis showed that the tests always fail at the new added if-test
I know that Unit tests must be able to run on systems even without an IIS installation. Should I encapsultate the installation of the |
|
Hey @tmeckel . This is a weird one. I've found a fix, although I'm not sure I fully understand yet why the tests were implemented this way. Essentially, in Tests\Unit\MSFT_xDSCWebService.Tests.ps1, you need to change the $allowedArgs variable from: to |
mhendric
added
the
waiting for code fix
|
@mhendric Nice catch .. I wasn't aware of this feature in Pester 👍 Learned something! But I would call this fix the 'Get the stuff running syndrome' LOL because this wouldn't improve the testability and the robustness of the tests. The fix only allows us to complete the PR. But if we would act often like that we would put additional technical debts in here and I'm pretty sure there're enough of them right now. This said my proposal would be to write three additional functions: If you think I'm overstating here, please say so. Then, of course, we can apply the cheap fix. |
|
@tmeckel , I totally agree! I'd much rather have a highly functional, easier to understand, well written fix, then a cheap fix. If you're willing to take that on, then by all means, go for it! |
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 24 unresolved discussions (waiting on @tmeckel)
a discussion (no related file):
Hey @tmeckel. Awesome work here so far. I only reviewed about half your changes, but have some general observations that will need to be addressed throughout:
Lots of new functions need comment based help.
Lots of calls to Write-Verbose or Write-Error need -Message parameter, and non parenthesis
Lots of new function parameter blocks needs to be updated to follow: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-block-at-top-of-function
A lot of the new strings should probably be created as localized strings instead.
There seems to be lots of similar code that joins similar paths together. Can any of this be consolidated into a helper function to avoid duplicate code?
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 445 at r1 (raw file):
Write-Verbose ('Accepting self signed certificates from incoming hosts')
Needs a -Message parameter on Write-Verbose. The parenthesis are not required.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 745 at r1 (raw file):
Write-Verbose ("AcceptSelfSignedCertificates is enabled. Checking if module Selfsigned IIS module is configured for web site at [$webConfigFullPath].")
Needs a -Message parameter on Write-Verbose. The parenthesis are not required.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 750 at r1 (raw file):
Write-Verbose ("Module not enabled in web site. Current configuration does not match the desired state.")
Needs a -Message parameter on Write-Verbose. The parenthesis are not required. Needs single quotes.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 755 at r1 (raw file):
Write-Verbose ("Module present in web site. Current configuration match the desired state.")
Needs a -Message parameter on Write-Verbose. The parenthesis are not required. Needs single quotes.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 760 at r1 (raw file):
Write-Verbose ("Selfsigned module not installed in IIS. Current configuration does not match the desired state.")
Needs a -Message parameter on Write-Verbose. The parenthesis are not required. Needs single quotes.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 784 at r1 (raw file):
function Get-OSVersion
New function needs comment based help.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 795 at r1 (raw file):
function Get-WebConfigModulesSetting
New function needs comment based help.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 827 at r1 (raw file):
# Allow this Website to enable/disable specific Auth Schemes by adding <location> tag in applicationhost.config function Update-LocationTagInApplicationHostConfigForAuthentication
New function needs comment based help.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 838 at r1 (raw file):
[ValidateSet('anonymous', 'basic', 'windows')]
Not positive, but should the first letter of each of these be capitalized?
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 853 at r1 (raw file):
function Get-IISServerManager
New function needs comment based help.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 873 at r1 (raw file):
# Helper function to Test the specified Web.Config Modules Setting function Test-WebConfigModulesSetting {
New function needs comment based help.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 876 at r1 (raw file):
param
Function should have an output type of System.Boolean
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 904 at r1 (raw file):
Write-Verbose ("Test-WebConfigModulesSetting: web.config file not found at [$WebConfigFullPath]")
Needs a -Message parameter on Write-Verbose. The parenthesis are not required.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1015 at r1 (raw file):
Quoted 6 lines of code…
function Get-IISAppCmd { Push-Location -Path "$env:windir\system32\inetsrv" $appCmd = Get-Command -Name '.\appcmd.exe' -CommandType 'Application' -ErrorAction:Stop Pop-Location $appCmd }
Function needs comment based help. Also needs cmdletbinding and param statements.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1022 at r1 (raw file):
function Test-IISSelfSignedModuleInstalled
Function needs comment based help.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1024 at r1 (raw file):
[OutputType([bool])] param ( [switch]$Enable32BitAppOnWin64
OutputType should be Boolean or System.Boolean. param style should follow https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-block-at-top-of-function
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1033 at r1 (raw file):
function Install-IISSelfSignedModule
Needs comment based help.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1036 at r1 (raw file):
Quoted 8 lines of code…
param ( [switch]$Enable32BitAppOnWin64
Param style should follow https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-block-at-top-of-function
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1042 at r1 (raw file):
Write-Verbose ("Install-IISSelfSignedModule: Providing $iisSelfSignedModuleAssemblyName to run in a 32 bit process")
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1042 at r1 (raw file):
if ($Enable32BitAppOnWin64) { Write-Verbose ("Install-IISSelfSignedModule: Providing $iisSelfSignedModuleAssemblyName to run in a 32 bit process")
Needs -Message and no parens.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1050 at r1 (raw file):
Write-Verbose ("Install-IISSelfSignedModule: module $iisSelfSignedModuleName already installed")
Needs -Message and no parens.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1054 at r1 (raw file):
Write-Verbose ("Install-IISSelfSignedModule: Installing module $iisSelfSignedModuleName")
Needs -Message and no parens.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1118 at r1 (raw file):
Write-Error ("Website [$EndpointName] not found")
Needs -Message and no parens.
|
Hi @mhendric thanks for starting reviewing my PR. Actually I think it's not in a reviewable state. I admit I should've added a comment respectively or worked on another branch which doesn't push here. As you noticed I started moving code into separate functions, especially for handling of the IIS self signed module. I already moved those functions to separate (helper) modules like To be honest, while I was working on this resource, I discovered so many parts which are implemented inconsistently. The methods for the IIS module are a good example for this. Partly they use appcmd.exe to test or modify the settings. At other edges methods from the WebAdministration modules are (implicitly) used or own support functions are written. Because of this to me it was hard to find a way through all this cleaning up the code in the |
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 24 unresolved discussions (waiting on @mhendric)
a discussion (no related file):
Previously, mhendric (Mike Hendrickson) wrote…
Hey @tmeckel. Awesome work here so far. I only reviewed about half your changes, but have some general observations that will need to be addressed throughout:
Lots of new functions need comment based help.
Lots of calls to Write-Verbose or Write-Error need -Message parameter, and non parenthesis
Lots of new function parameter blocks needs to be updated to follow: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-block-at-top-of-function
A lot of the new strings should probably be created as localized strings instead.
There seems to be lots of similar code that joins similar paths together. Can any of this be consolidated into a helper function to avoid duplicate code?
To me cleaning up the code could/should be done, when all required (wrapper/mockable) functions has been created and the existing Unit tests are still successfully passed.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 445 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Write-Verbose ('Accepting self signed certificates from incoming hosts')Needs a -Message parameter on Write-Verbose. The parenthesis are not required.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 745 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Write-Verbose ("AcceptSelfSignedCertificates is enabled. Checking if module Selfsigned IIS module is configured for web site at [$webConfigFullPath].")Needs a -Message parameter on Write-Verbose. The parenthesis are not required.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 750 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Write-Verbose ("Module not enabled in web site. Current configuration does not match the desired state.")Needs a -Message parameter on Write-Verbose. The parenthesis are not required. Needs single quotes.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 755 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Write-Verbose ("Module present in web site. Current configuration match the desired state.")Needs a -Message parameter on Write-Verbose. The parenthesis are not required. Needs single quotes.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 760 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Write-Verbose ("Selfsigned module not installed in IIS. Current configuration does not match the desired state.")Needs a -Message parameter on Write-Verbose. The parenthesis are not required. Needs single quotes.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 784 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
function Get-OSVersionNew function needs comment based help.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 795 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
function Get-WebConfigModulesSettingNew function needs comment based help.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 827 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
# Allow this Website to enable/disable specific Auth Schemes by adding <location> tag in applicationhost.config function Update-LocationTagInApplicationHostConfigForAuthenticationNew function needs comment based help.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 838 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
[ValidateSet('anonymous', 'basic', 'windows')]Not positive, but should the first letter of each of these be capitalized?
No... It's the name of the IIS authentocation module. And the names are lowercase
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 853 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
function Get-IISServerManagerNew function needs comment based help.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 873 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
# Helper function to Test the specified Web.Config Modules Setting function Test-WebConfigModulesSetting {New function needs comment based help.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 876 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
paramFunction should have an output type of System.Boolean
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 904 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Write-Verbose ("Test-WebConfigModulesSetting: web.config file not found at [$WebConfigFullPath]")Needs a -Message parameter on Write-Verbose. The parenthesis are not required.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1015 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
function Get-IISAppCmd { Push-Location -Path "$env:windir\system32\inetsrv" $appCmd = Get-Command -Name '.\appcmd.exe' -CommandType 'Application' -ErrorAction:Stop Pop-Location $appCmd }Function needs comment based help. Also needs cmdletbinding and param statements.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1022 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
function Test-IISSelfSignedModuleInstalledFunction needs comment based help.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1024 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
[OutputType([bool])] param ( [switch]$Enable32BitAppOnWin64OutputType should be Boolean or System.Boolean. param style should follow https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-block-at-top-of-function
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1033 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
function Install-IISSelfSignedModuleNeeds comment based help.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1036 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
param ( [switch]$Enable32BitAppOnWin64Param style should follow https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#parameter-block-at-top-of-function
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1042 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Write-Verbose ("Install-IISSelfSignedModule: Providing $iisSelfSignedModuleAssemblyName to run in a 32 bit process")
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1042 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Needs -Message and no parens.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1050 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Write-Verbose ("Install-IISSelfSignedModule: module $iisSelfSignedModuleName already installed")Needs -Message and no parens.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1054 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Write-Verbose ("Install-IISSelfSignedModule: Installing module $iisSelfSignedModuleName")Needs -Message and no parens.
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1118 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Write-Error ("Website [$EndpointName] not found")Needs -Message and no parens.
Done.
|
Hey @tmeckel , just checking if you're ready for another code review on this or not. Let me know. Also, that's totally fine if you want to leave code cleanup for the module until later or a different PR (although newly added code, or modified code, should still follow style guidelines). |
There was a problem hiding this comment.
Hi @mhendric... just adjusted the code according to your first review. Lemme do some tests tomorrow in one the customers tests enviroments if the code works properly. If so the PR is ready for another review. About the code cleanup: as I said I already had a version for which I moved all helper functions into separate modules. But that broke the Unit tests because I could get the mock for 'Get-Website' running inside all new submodules. I'm still the opinion that this is the correct strategy, I mean movin helper code to sub-modules. But perhaps I need so help from you someone else to get that damn mock running :-D
Reviewable status: 0 of 2 files reviewed, 24 unresolved discussions (waiting on @mhendric)
|
Hey @mhendric ready for an additional review. As you can see I fixed another issue that prevents the successfull configuration of a PullServer when the IIS SelfSigned module has been already loaded by IIS and is thus locked in the filesystem. Well there is remaining risk that the configuration will fail, when the loaded module DLL differs from the one which is located in the IIS and the PullServer directory in Secondly, because I was working on the code sequence anyway, I (hopefully) fixed issue #528 :-D |
There was a problem hiding this comment.
Hey @tmeckel. Excellent work here. Still some work to be done, but this is getting a lot better.
Reviewable status: 0 of 2 files reviewed, 35 unresolved discussions (waiting on @mhendric and @tmeckel)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 445 at r1 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
Looks like this still needs to be done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 784 at r1 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 795 at r1 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 827 at r1 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 853 at r1 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 873 at r1 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1015 at r1 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
Can you move the function open bracket to the new line?
Also Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1022 at r1 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1033 at r1 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1036 at r1 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
This one still needs a bit of an update. Can you change the type to 'Switch' (uppercase) or 'System.Management.Automation.SwitchParameter'? Also, the type and the variable name should be on separate lines.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 907 at r3 (raw file):
# Name of the WebSite
Don't need this with comment based help
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 912 at r3 (raw file):
# Authentication Type
Don't need this with comment based help
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 944 at r3 (raw file):
Quoted 12 lines of code…
$iisInstallPath = (Get-ItemProperty -Path 'HKLM:\SOFTWARE\Microsoft\INetStp' -Name InstallPath).InstallPath if (-not $iisInstallPath) { throw ($LocalizedData.IISInstallationPathNotFound) } $assyPath = Join-Path -Path $iisInstallPath -ChildPath 'Microsoft.Web.Administration.dll' -Resolve -ErrorAction:SilentlyContinue if (-not $assyPath) { throw ($LocalizedData.IISWebAdministrationAssemblyNotFound) } $assy = [System.Reflection.Assembly]::LoadFrom($assyPath) return [System.Activator]::CreateInstance($assy.FullName, 'Microsoft.Web.Administration.ServerManager').Unwrap()
Can you add some line breaks in here between the variable assignments and the if blocks to make this easier to read?
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1022 at r3 (raw file):
Quoted 16 lines of code…
.SYNOPSIS Tests if a the currently configured path for a website is equal to a given path .PARAMETER EndpointName The endpoint name (website name) to test .PARAMETER PhysicalPath The full physical path to check .OUTPUTS System.Boolean. true if the current installation status is equal to the expected installation status
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1063 at r3 (raw file):
Quoted 21 lines of code…
.SYNOPSIS Test if a currently configured app setting is equal to a given value .PARAMETER WebConfigFullPath The full path to the web.config .PARAMETER AppSettingName The app setting name to check .PARAMETER ExpectedAppSettingValue The expected value .OUTPUTS System.Boolean. true if the current value is equal to the expected value
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1130 at r3 (raw file):
Quoted 17 lines of code…
.SYNOPSIS Helper function to Get the specified Web.Config App Setting .PARAMETER WebConfigFullPath The full path to the web.config .PARAMETER AppSettingName The app settings name to get the value for .OUTPUTS System.String. the current app settings value
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1184 at r3 (raw file):
IIS Selfsegned Certficate Module
Need to fix the misspelling of Self Signed
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1212 at r3 (raw file):
Quoted 16 lines of code…
.SYNOPSIS Tests if two files differ .PARAMETER SourceFilePath Path to the source file .PARAMETER DestinationFilePath Path to the destination file .OUTPUTS System.Boolean. true if the two files differ
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1314 at r3 (raw file):
Quoted 13 lines of code…
$destinationFolderPath = "$env:windir\System32\inetsrv" $destinationFilePath = Join-Path -Path $destinationFolderPath -ChildPath $iisSelfSignedModuleAssemblyName if (Test-FilesDiffer -SourceFilePath $sourceFilePath -DestinationFilePath $destinationFilePath) { # Might fail if the DLL has already been loaded by the IIS from a former PullServer Deployment Copy-Item -Path $sourceFilePath -Destination $destinationFolderPath -Force } else { Write-Verbose -Message "Install-IISSelfSignedModule: module $iisSelfSignedModuleName already installed at $destinationFilePath with the correct version" } Write-Verbose -Message "Install-IISSelfSignedModule: globally activating module $iisSelfSignedModuleName" & (Get-IISAppCmd) install module /name:$iisSelfSignedModuleName /image:$destinationFilePath /add:false /lock:false
Could use some line breaks in here between the if/else blocks and everything else to make this more readable.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1331 at r3 (raw file):
Quoted 12 lines of code…
.SYNOPSIS Enable the IISSelfSignedModule module for a specific website (endpoint) .PARAMETER EndpointName The endpoint (website) for which the module should be enabled .PARAMETER Enable32BitAppOnWin64 If set enable the module as a 32bit module
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1346 at r3 (raw file):
Quoted 5 lines of code…
param ( [Parameter(Mandatory)] [ValidateNotNullOrEmpty()] [string]$EndpointName, [switch]$Enable32BitAppOnWin64
Open parens for param should be on new line. [String] and [Switch] should be capitalized. Variable names should be on their own line. $Enable32BitAppOnWin64 needs a [Parameter()] block. Parameter should be : [Parameter(Mandatory = $true)]
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1365 at r3 (raw file):
Quoted 8 lines of code…
.SYNOPSIS Disable the IISSelfSignedModule module for a specific website (endpoint) .PARAMETER EndpointName The endpoint (website) for which the module should be disabled
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1379 at r3 (raw file):
Quoted 4 lines of code…
param ( [Parameter(Mandatory)] [ValidateNotNullOrEmpty()] [string]$EndpointName
Open parens for param should be on new line. [String] should be capitalized. Variable names should be on their own line. Parameter should be : [Parameter(Mandatory = $true)]
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1387 at r3 (raw file):
Quoted 28 lines of code…
.SYNOPSIS Tests if the IISSelfSignedModule module is enabled for a website (endpoint) .PARAMETER EndpointName The endpoint (website) for which the status should be checked .OUTPUTS System.Boolean. true if the module is enabled
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1405 at r3 (raw file):
[Parameter(Mandatory)] [ValidateNotNullOrEmpty()] [string]$EndpointName
Open parens for param should be on new line. [String] should be capitalized. Variable names should be on their own line.
Parameter should be : [Parameter(Mandatory = $true)]
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 182 at r3 (raw file):
Quoted 6 lines of code…
Mock -CommandName Test-WebConfigModulesSetting ` -ParameterFilter {$ModuleName -eq 'IISSelfSignedCertModule(32bit)'} ` -MockWith { param ($ExpectedInstallationStatus) Write-Verbose -Message 'Test-WebConfigAppSetting - IISSelfSignedCertModule'; $acceptSelfSignedCertificates -eq $ExpectedInstallationStatus }
If this code isn't necessary it should be removed.
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 472 at r3 (raw file):
$testArguments = 'if ($allowedArgs -notcontains $MyInvocation.Line.Trim()) {throw ''Mock test failed.''}'
I don't see $allowedArgs defined anywhere anymore. Will this actually do anything?
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 484 at r3 (raw file):
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-Command
Fix me :)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 498 at r3 (raw file):
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-Command
Fix me :)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 578 at r3 (raw file):
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-Command
Fix me :)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 637 at r3 (raw file):
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-Command
Fix me :)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 665 at r3 (raw file):
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-Command
Fix me :)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 754 at r3 (raw file):
$testArguments = 'if ($allowedArgs -notcontains $MyInvocation.Line.Trim()) {throw ''Mock test failed.''}'
I don't see $allowedArgs defined anywhere anymore. Will this actually do anything?
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 35 unresolved discussions (waiting on @mhendric and @tmeckel)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 445 at r1 (raw file):
Accepting self signed certificates from incoming hosts
Missed that damn thingy :-D
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 784 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done according to style guides
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 795 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done according to style guides.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 827 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done according to style guides.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 853 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done according to style guides
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 873 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done according to style guides
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1015 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Can you move the function open bracket to the new line?
Also Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1022 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1033 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Comment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1036 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
This one still needs a bit of an update. Can you change the type to 'Switch' (uppercase) or 'System.Management.Automation.SwitchParameter'? Also, the type and the variable name should be on separate lines.
Done
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 907 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
# Name of the WebSiteDon't need this with comment based help
Removed
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 912 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
# Authentication TypeDon't need this with comment based help
Removed
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 944 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
$iisInstallPath = (Get-ItemProperty -Path 'HKLM:\SOFTWARE\Microsoft\INetStp' -Name InstallPath).InstallPath if (-not $iisInstallPath) { throw ($LocalizedData.IISInstallationPathNotFound) } $assyPath = Join-Path -Path $iisInstallPath -ChildPath 'Microsoft.Web.Administration.dll' -Resolve -ErrorAction:SilentlyContinue if (-not $assyPath) { throw ($LocalizedData.IISWebAdministrationAssemblyNotFound) } $assy = [System.Reflection.Assembly]::LoadFrom($assyPath) return [System.Activator]::CreateInstance($assy.FullName, 'Microsoft.Web.Administration.ServerManager').Unwrap()Can you add some line breaks in here between the variable assignments and the if blocks to make this easier to read?
Added some line breaks
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1022 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
.SYNOPSIS Tests if a the currently configured path for a website is equal to a given path .PARAMETER EndpointName The endpoint name (website name) to test .PARAMETER PhysicalPath The full physical path to check .OUTPUTS System.Boolean. true if the current installation status is equal to the expected installation statusComment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done according to style guides
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1063 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
.SYNOPSIS Test if a currently configured app setting is equal to a given value .PARAMETER WebConfigFullPath The full path to the web.config .PARAMETER AppSettingName The app setting name to check .PARAMETER ExpectedAppSettingValue The expected value .OUTPUTS System.Boolean. true if the current value is equal to the expected valueComment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1130 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
.SYNOPSIS Helper function to Get the specified Web.Config App Setting .PARAMETER WebConfigFullPath The full path to the web.config .PARAMETER AppSettingName The app settings name to get the value for .OUTPUTS System.String. the current app settings valueComment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1184 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
IIS Selfsegned Certficate ModuleNeed to fix the misspelling of Self Signed
Corrected
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1212 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
.SYNOPSIS Tests if two files differ .PARAMETER SourceFilePath Path to the source file .PARAMETER DestinationFilePath Path to the destination file .OUTPUTS System.Boolean. true if the two files differComment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1314 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
$destinationFolderPath = "$env:windir\System32\inetsrv" $destinationFilePath = Join-Path -Path $destinationFolderPath -ChildPath $iisSelfSignedModuleAssemblyName if (Test-FilesDiffer -SourceFilePath $sourceFilePath -DestinationFilePath $destinationFilePath) { # Might fail if the DLL has already been loaded by the IIS from a former PullServer Deployment Copy-Item -Path $sourceFilePath -Destination $destinationFolderPath -Force } else { Write-Verbose -Message "Install-IISSelfSignedModule: module $iisSelfSignedModuleName already installed at $destinationFilePath with the correct version" } Write-Verbose -Message "Install-IISSelfSignedModule: globally activating module $iisSelfSignedModuleName" & (Get-IISAppCmd) install module /name:$iisSelfSignedModuleName /image:$destinationFilePath /add:false /lock:falseCould use some line breaks in here between the if/else blocks and everything else to make this more readable.
I added some line breaks here. Hope it's more readable now.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1331 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
.SYNOPSIS Enable the IISSelfSignedModule module for a specific website (endpoint) .PARAMETER EndpointName The endpoint (website) for which the module should be enabled .PARAMETER Enable32BitAppOnWin64 If set enable the module as a 32bit moduleComment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1346 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
param ( [Parameter(Mandatory)] [ValidateNotNullOrEmpty()] [string]$EndpointName, [switch]$Enable32BitAppOnWin64Open parens for param should be on new line. [String] and [Switch] should be capitalized. Variable names should be on their own line. $Enable32BitAppOnWin64 needs a [Parameter()] block. Parameter should be : [Parameter(Mandatory = $true)]
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1365 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
.SYNOPSIS Disable the IISSelfSignedModule module for a specific website (endpoint) .PARAMETER EndpointName The endpoint (website) for which the module should be disabledComment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1379 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
param ( [Parameter(Mandatory)] [ValidateNotNullOrEmpty()] [string]$EndpointNameOpen parens for param should be on new line. [String] should be capitalized. Variable names should be on their own line. Parameter should be : [Parameter(Mandatory = $true)]
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1387 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
.SYNOPSIS Tests if the IISSelfSignedModule module is enabled for a website (endpoint) .PARAMETER EndpointName The endpoint (website) for which the status should be checked .OUTPUTS System.Boolean. true if the module is enabledComment based help should look like the good examples in the DSC style guidelines. Specifically, headers should be indented by 4 chars, and the text underneath should be indented by 8. Also, we don't need a blank line between the header and the text (although we should have a blank line between header sections)
Done.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1405 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
[Parameter(Mandatory)] [ValidateNotNullOrEmpty()] [string]$EndpointNameOpen parens for param should be on new line. [String] should be capitalized. Variable names should be on their own line.
Parameter should be : [Parameter(Mandatory = $true)]
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r5.
Reviewable status: 1 of 2 files reviewed, 10 unresolved discussions (waiting on @mhendric and @tmeckel)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 519 at r5 (raw file):
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-Command
Needs to be fixed.
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 602 at r5 (raw file):
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-Command
Needs to be fixed.
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 10 unresolved discussions (waiting on @mhendric)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 182 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Mock -CommandName Test-WebConfigModulesSetting ` -ParameterFilter {$ModuleName -eq 'IISSelfSignedCertModule(32bit)'} ` -MockWith { param ($ExpectedInstallationStatus) Write-Verbose -Message 'Test-WebConfigAppSetting - IISSelfSignedCertModule'; $acceptSelfSignedCertificates -eq $ExpectedInstallationStatus }If this code isn't necessary it should be removed.
Removed the Mock … Tests are still passed
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 472 at r3 (raw file):
$allowedArgs
Re-inserted $allowedArgs because it has been inadvertently removed. Wondering why the tests still passed though!
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 484 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-CommandFix me :)
Replaced with the correct new mock function Get-IISAppCmd
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 498 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-CommandFix me :)
Corrected with new mock function
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 578 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-CommandFix me :)
Corrected with mock Get-IISAppCmd
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 637 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-CommandFix me :)
Corrected with new mock function
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 665 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-CommandFix me :)
Corrected with new mock function
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 754 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
$testArguments = 'if ($allowedArgs -notcontains $MyInvocation.Line.Trim()) {throw ''Mock test failed.''}'I don't see $allowedArgs defined anywhere anymore. Will this actually do anything?
Re-inserted $allowedArgs again because it has been deleted inadvertently
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 519 at r5 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-CommandNeeds to be fixed.
Corrected
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 602 at r5 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
##FIXME: Assert-MockCalled -Exactly -Times 1 -CommandName Get-CommandNeeds to be fixed.
Corrected with new mock Get-IISAppCmd
|
Hey @mhendric … ready for the next review :-D in addition I found and corrected an issue how the module detects the FQDN of a host. The old code relied on environment variables. I adopted the code from the uni tests which uses |
There was a problem hiding this comment.
Reviewed 1 of 2 files at r6.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @mhendric and @tmeckel)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 472 at r3 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
$allowedArgs
Re-inserted $allowedArgs because it has been inadvertently removed. Wondering why the tests still passed though!
To be honest, I'm still a bit confused as to what the $testArguments script block was trying to accomplish in the first place. Having spent a lot of time looking at this code, do you think this particular MockWith buys us much? The $allowedArgs variable triggers the PSUseDeclaredVarsMoreThanAssignments script analyzer rule since it's not actually used until RunTime. If these two vars aren't actually doing much (other than confusing us), maybe they shouldn't be here...
|
Hey @tmeckel , I just noticed that this fixes all but one of the script analyzer errors in the Meta tests. Up to you if you want to fix that here or not, but if not, we'll need another PR for it at some point. [00:01:09] Context MSFT_xDSCWebService.psm1 |
|
FYI, I opened #565 to track the PSSA issue above. Somehow I missed that file when I creates Issues for the rest of the problem files. |
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @mhendric)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 472 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
To be honest, I'm still a bit confused as to what the $testArguments script block was trying to accomplish in the first place. Having spent a lot of time looking at this code, do you think this particular MockWith buys us much? The $allowedArgs variable triggers the PSUseDeclaredVarsMoreThanAssignments script analyzer rule since it's not actually used until RunTime. If these two vars aren't actually doing much (other than confusing us), maybe they shouldn't be here...
After I changed the mocking and the assertions for it inside the unit tests I discovered that the method Get-IISAppCmdis never called in the currently implemented (unit) tests. You'll find Assert-MockCalled -Exactly -Times 0 -CommandName Get-IISAppCmd at every Itstatement. So I started to analyze the calling hierarchy inside *-TargetResource manually and after that I was very sure that the Get-IISAppCmdmust be called during the execution of the unit tests. So as you I'm a bit baffled now. And secondly, because Get-IISAppCmd is calling Get-Command internally I can revert back the changes to unit tests where I replaced the Get-Command mocking in favor of Get-IISAppCmd.
Although the implementation of MSFT_xDSCWebService is very robust even in the craziest machine setups (I even deployed and retracted a PullServer successfully on a machine where VisualStudio, IIS Express, SQLServer and SharePoint are installed simultaneously!) I would prefer to analyse this strange behavior with Get-IISAppCmd before we merge the PR.
Hey @mhendric I'll fix those issues on the current PR. |
|
Hey @mhendric I changed the implementation of the unit tests. Now this looks much more reasonable, although the syntax (implementation) of the mock for |
|
@mhendric I rebased the PR to the current origin/dev branch but still a hint about existing conflicts is displayed. If I should do another rebase lemme know. Secondly I fixed the PSSA issue #565. Hopefully we're finally on the finishing line for this PR! Oh .. do I have to adjust the CHANGELOG.md again because I fixed #565 and #528 as well in this PR? |
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @mhendric)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 472 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Looking better! I'm still wondering what this code is trying to guard against though. It seems that it was trying to prevent the calling of AppCmd using non-pre-approved strings. Is that really a meaningful test though, and is it a scenario we really need to guard against? I feel like I'm missing something, but it just seems to me to be an arbitrary test that isn't actually useful.
If we do figure out the reason why this code is helpful, I think we should clearly document it using comments. Also, rather than having the MockWith create a ScriptBlock from the $testArguments string, I think we may be better off just putting this and $allowedArgs directly in the MockWith block, and not bother converting from string to script block. But again, if we can't figure out what purpose this code is serving, maybe we should just get rid of it.
Done.
There was a problem hiding this comment.
Hey @tmeckel, sorry for the late response, been traveling all week. It probably would be good to update the CHANGELOG if you are now fixing other issues too. And your comments on Get-Command sound good. I've added one more comment on that below. Let me know what you think.
Reviewed 1 of 2 files at r7.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @mhendric and @tmeckel)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 472 at r3 (raw file):
Previously, tmeckel (Thomas Meckel) wrote…
Done.
Sorry, I think I was a bit unclear with that last paragraph. So instead of generating a script block from strings which are super hard to read and which throw off the Script Analyzer, I was suggesting just moving this code directly into the MockWith like this. Making the $allowedArgs more readable would be a bonus too. Same for the other Mock later in the code. What do you think?
Mock -CommandName Get-Command -ParameterFilter {$Name -eq '.\appcmd.exe'} -MockWith {
$allowedCallers = @(
([ScriptBlock]{('' -ne ((& (Get-IISAppCmd) list config -section:system.webServer/globalModules) -like "*$iisSelfSignedModuleName*"))}).ToString()
([ScriptBlock]{& (Get-IISAppCmd) install module /name:$iisSelfSignedModuleName /image:$destinationFilePath /add:false /lock:false}).ToString()
([ScriptBlock]{& (Get-IISAppCmd) add module /name:$iisSelfSignedModuleName /app.name:"$EndpointName/" $preConditionBitnessArgumentFor32BitInstall}).ToString()
([ScriptBlock]{& (Get-IISAppCmd) delete module /name:$iisSelfSignedModuleName /app.name:"$EndpointName/"}).ToString()
)
if ($allowedCallers -notcontains ($MyInvocation.Line.Trim() -replace '\s+', ' ')) {
throw "Mock test failed. Unexpected caller of Get-Command and appcmd.exe. [$line]"
}
}
|
@tmeckel , also note that after you rebase, your change log entry is going to be under the 8.5.0.0 section, and will need to be moved to the Unreleased section. Thanks. |
|
Hey @mhendric your proposal on restructuring the mock code for |
…b module in MSFT_xDSCWebService. Changed the unit tests accordingly
…CWebService because of issues with Pester Mocking * Updated Unit tests for MSFT_xDSCWebService
…e is still used instead of Test-IISSelfSignedModuleInstalled
destination folder although the module is locked (loaded) by IIS already * fixed issue 528
relied on environemnt variables * changed Unit tests according to review
* added verbose messages to Get-TargetResource to satisfy PSSA rules
…tion to CHANGELOG.md
|
Hey @mhendric I changed the |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r8.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mhendric and @tmeckel)
Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 472 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Sorry, I think I was a bit unclear with that last paragraph. So instead of generating a script block from strings which are super hard to read and which throw off the Script Analyzer, I was suggesting just moving this code directly into the MockWith like this. Making the $allowedArgs more readable would be a bonus too. Same for the other Mock later in the code. What do you think?
Mock -CommandName Get-Command -ParameterFilter {$Name -eq '.\appcmd.exe'} -MockWith { $allowedCallers = @( ([ScriptBlock]{('' -ne ((& (Get-IISAppCmd) list config -section:system.webServer/globalModules) -like "*$iisSelfSignedModuleName*"))}).ToString() ([ScriptBlock]{& (Get-IISAppCmd) install module /name:$iisSelfSignedModuleName /image:$destinationFilePath /add:false /lock:false}).ToString() ([ScriptBlock]{& (Get-IISAppCmd) add module /name:$iisSelfSignedModuleName /app.name:"$EndpointName/" $preConditionBitnessArgumentFor32BitInstall}).ToString() ([ScriptBlock]{& (Get-IISAppCmd) delete module /name:$iisSelfSignedModuleName /app.name:"$EndpointName/"}).ToString() ) if ($allowedCallers -notcontains ($MyInvocation.Line.Trim() -replace '\s+', ' ')) { throw "Mock test failed. Unexpected caller of Get-Command and appcmd.exe. [$line]" } }
Awesome. Thanks for adding detailed additional comments too!
Pull Request (PR) description
This Pull Request (PR) implements a fix for issue #460
This Pull Request (PR) fixes the following issues
-Fixes #460
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is