BREAKING CHANGE: Introducing native ReverseDSC support for the module #442
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #442 +/- ##
==========================================
+ Coverage 90.85% 95.42% +4.56%
==========================================
Files 17 18 +1
Lines 2461 2796 +335
==========================================
+ Hits 2236 2668 +432
+ Misses 225 128 -97
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Nik, this is great work and I look forward to seeing this capability added to the module. Please open an issue for the work so there is more context for the changes being introduced and there can be discussion if needed. It's a large PR, so I didn't have time to review it all at once, but wanted to provide some initial feedback in the meantime. Some comments are style related and I didn't want to leave the same style comment on every single instance on this review, but please go through again and see if you can correct those throughout.
Also, let's add tests for the new functions as well. There would be great value in asserting the new Export-TargetResource functions return the expected configurations given mocked data for the websites. LEt me know if you need help with the test pattern for these function.
Can you add some documentation and/or examples that show how this is used?
Reviewable status: 0 of 22 files reviewed, 9 unresolved discussions (waiting on @NikCharlebois)
DSCResources/MSFT_WebApplicationHandler/MSFT_WebApplicationHandler.psm1, line 385 at r3 (raw file):
[CmdletBinding()] [OutputType([System.String])]
These attributes won't work without a param (). These attributes should go above an empty param ()
DSCResources/MSFT_WebApplicationHandler/MSFT_WebApplicationHandler.psm1, line 392 at r3 (raw file):
$DSCConfigContent = ""
Per the style guidelines, variable names use camelCase, so this would be $dscConfigContent. Same applies throughout
DSCResources/MSFT_WebApplicationHandler/MSFT_WebApplicationHandler.psm1, line 404 at r3 (raw file):
$Script:DSCConfigContent += " WebApplicationHandler " + (New-Guid).ToString() + "`r`n {`r`n" $Script:DSCConfigContent += Get-DSCBlock -Params $results -ModulePath $PSScriptRoot $Script:DSCConfigContent += " }`r`n"
The $stringBuilder = [System.Text.StringBuilder]::new() .NET method would be more efficient in these functions. It would be easier to read because the .AppendLine method would allow us to create new lines without the r`n carriage return, new line syntax on each line.
DSCResources/MSFT_xIIsHandler/MSFT_xIisHandler.psm1, line 918 at r3 (raw file):
} function Export-TargetResource
This function appears to be missing logic that would export the current configuration. Is this one not ready or supported?
DSCResources/MSFT_xIisFeatureDelegation/MSFT_xIisFeatureDelegation.psm1, line 161 at r3 (raw file):
"Continue"
Per the style guidelines, single quotes should always be used to delimit string literals wherever possible. Double quoted string literals may only be used when it contains ($) expressions that need to be evaluated. Please double check the other strings throughout are wrapped in single quotes whenever possible.
DSCResources/MSFT_xIisFeatureDelegation/MSFT_xIisFeatureDelegation.psm1, line 171 at r3 (raw file):
function Get-IISFeatureDelegation { param
All cmdlets should be [CmdletBinding()] and include the OutputType attribute when the function returns something.
DSCResources/MSFT_xIisModule/MSFT_xIisModule.schema.mof, line 2 at r3 (raw file):
Quoted 12 lines of code…
[ClassVersion("1.0.0"), FriendlyName("xIisModule")] class MSFT_xIisModule : OMI_BaseResource { [Key, Description("The path to the module, usually a dll, to be added to IIS.")] String Path; [Required, Description("The logical name of the module to add to IIS.")] String Name; [Required, Description("The allowed request Path example: *.php")] String RequestPath; [Required, Description("The supported verbs for the module.")] String Verb[]; [Write, Description("The IIS Site to register the module.")] String SiteName; [Write, Description("Should the module be present or absent."), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure; [Write, Description("The type of the module."), ValueMap{"FastCgiModule"}, Values{"FastCgiModule"}] String ModuleType; [Read, Description("The End Point is setup. Such as a Fast Cgi endpoint.")] Boolean EndPointSetup; };
This is a major change to this resource and breaking change. Can we get a issue open for this, so we can reference in this PR?
DSCResources/MSFT_xSSLSettings/MSFT_xSSLSettings.psm1, line 177 at r3 (raw file):
} function Export-TargetResource
This one also appears to not return the current configuration. Is this intentional?
DSCResources/MSFT_xWebApplication/MSFT_xWebApplication.psm1, line 518 at r3 (raw file):
$i = 1 foreach($website in $webSites)
Per the style guidelines, there should be a single space after the keyword foreach (
regedit32
added
breaking change
regedit32
changed the title
There was a problem hiding this comment.
Reviewable status: 0 of 23 files reviewed, 10 unresolved discussions (waiting on @NikCharlebois)
a discussion (no related file):
I haven't had time to review yet, but as @regedit32 said we need tests and documentation for this change, and also update the change log with entries what this PR changes and adds.
Great work!
|
Linked to #443 |
There was a problem hiding this comment.
Reviewable status: 0 of 36 files reviewed, 10 unresolved discussions (waiting on @regedit32)
a discussion (no related file):
Previously, johlju (Johan Ljunggren) wrote…
I haven't had time to review yet, but as @regedit32 said we need tests and documentation for this change, and also update the change log with entries what this PR changes and adds.
Great work!
Done.
DSCResources/MSFT_WebApplicationHandler/MSFT_WebApplicationHandler.psm1, line 385 at r3 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
[CmdletBinding()] [OutputType([System.String])]These attributes won't work without a
param (). These attributes should go above an emptyparam ()
Done.
DSCResources/MSFT_WebApplicationHandler/MSFT_WebApplicationHandler.psm1, line 392 at r3 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
$DSCConfigContent = ""Per the style guidelines, variable names use camelCase, so this would be
$dscConfigContent. Same applies throughout
Done.
DSCResources/MSFT_WebApplicationHandler/MSFT_WebApplicationHandler.psm1, line 404 at r3 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
$Script:DSCConfigContent += " WebApplicationHandler " + (New-Guid).ToString() + "`r`n {`r`n" $Script:DSCConfigContent += Get-DSCBlock -Params $results -ModulePath $PSScriptRoot $Script:DSCConfigContent += " }`r`n"The
$stringBuilder = [System.Text.StringBuilder]::new().NET method would be more efficient in these functions. It would be easier to read because the.AppendLinemethod would allow us to create new lines without ther`ncarriage return, new line syntax on each line.
Done.
DSCResources/MSFT_xIIsHandler/MSFT_xIisHandler.psm1, line 918 at r3 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
This function appears to be missing logic that would export the current configuration. Is this one not ready or supported?
Done.
DSCResources/MSFT_xIisFeatureDelegation/MSFT_xIisFeatureDelegation.psm1, line 161 at r3 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
"Continue"Per the style guidelines, single quotes should always be used to delimit string literals wherever possible. Double quoted string literals may only be used when it contains ($) expressions that need to be evaluated. Please double check the other strings throughout are wrapped in single quotes whenever possible.
Done.
DSCResources/MSFT_xIisFeatureDelegation/MSFT_xIisFeatureDelegation.psm1, line 171 at r3 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
All cmdlets should be
[CmdletBinding()]and include theOutputTypeattribute when the function returns something.
Done.
DSCResources/MSFT_xIisModule/MSFT_xIisModule.schema.mof, line 2 at r3 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
[ClassVersion("1.0.0"), FriendlyName("xIisModule")] class MSFT_xIisModule : OMI_BaseResource { [Key, Description("The path to the module, usually a dll, to be added to IIS.")] String Path; [Required, Description("The logical name of the module to add to IIS.")] String Name; [Required, Description("The allowed request Path example: *.php")] String RequestPath; [Required, Description("The supported verbs for the module.")] String Verb[]; [Write, Description("The IIS Site to register the module.")] String SiteName; [Write, Description("Should the module be present or absent."), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure; [Write, Description("The type of the module."), ValueMap{"FastCgiModule"}, Values{"FastCgiModule"}] String ModuleType; [Read, Description("The End Point is setup. Such as a Fast Cgi endpoint.")] Boolean EndPointSetup; };This is a major change to this resource and breaking change. Can we get a issue open for this, so we can reference in this PR?
Done.
DSCResources/MSFT_xSSLSettings/MSFT_xSSLSettings.psm1, line 177 at r3 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
This one also appears to not return the current configuration. Is this intentional?
Done.
DSCResources/MSFT_xWebApplication/MSFT_xWebApplication.psm1, line 518 at r3 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
Per the style guidelines, there should be a single space after the keyword
foreach (
Done.
|
@regedit32 & @johlju Sorry I had to spam you with all these commits. Existing Resources were not even at par with the code Cov requirements in their previous status. I believe all is now ready for a second round of reviews. Cheers! |
| It 'Should Export all instances' { | ||
| Export-TargetResource | ||
| } |
There was a problem hiding this comment.
Thanks for adding the test, we can improve on those a bit. Every Pester test should have some type of assertion after a command or variable to assert the expected result. For a function that returns an output, the best way to test it is to invoke it and then compare the output to ensure it returns as expected.
For this test, based on the mock Get-WebConfigurationProperty return $mockCompliantHandler, what would we expect the output string for Export-TargetResource to look like? We can store that string in the test as $webApplicationHandlerConfiguration and assert the Export-TargetResource returns it.
For example:
Mock -CommandName New-Guid -MockWith { [GUID]::Empty }
$webApplicationHandlerConfiguration = @"
WebApplicationHandler 00000000-0000-0000-0000-000000000000
{
Path = 'MACHINE/WEBROOT/APPHOST'
Name = 'ATest-WebHandler'
PhysicalHandlerPath = '*'
Verb = '*'
Modules = 'IsapiModule'
RequireAccess = 'None'
ScriptProcessor = "C:\Windows\Microsoft.NET\Framework64\v4.0.30319\aspnet_isapi.dll"
ResourceType = 'Unspecified'
AllowPathInfo = $false
ResponseBufferLimit = 0
Type = $null
PreCondition = $null
Location = 'Default Web Site/TestDir'
}
@"
It 'Should return expected configuration' {
Export-TargetResource | Should -Be $webApplicationHandlerConfiguration
}Let's do that for all the Export-TargetResource tests. At least one, but we could probably come up with multiple scenarios for some of these. Like for xWebsite, what if zero websites return or multiple. Will the Export-TargetResource return as expected? Let me know if you need any help with that.
There was a problem hiding this comment.
Reviewed 3 of 31 files at r5, 2 of 8 files at r7.
Reviewable status: 5 of 37 files reviewed, 10 unresolved discussions (waiting on @NikCharlebois and @regedit32)
xWebAdministration.psd1, line 24 at r7 (raw file):
# Adds dependency to ReverseDSC RequiredModules = @(@{ModuleName = "ReverseDSC"; RequiredVersion = "1.9.4.7"; })
What is the dependency here and how does the added functionality interact with ReverseDsc? Since the Export-TargetResource functionality is an optional feature, I'm not sure we should enforce the module dependency here. Perhaps it would be better to assert the module is present whenever Export-TargetResource is ran? Thoughts?
DSCResources/MSFT_xWebApplication/MSFT_xWebApplication.psm1, line 524 at r7 (raw file):
if($webApplications)
Should be one space after the keyword
DSCResources/MSFT_xWebApplication/MSFT_xWebApplication.psm1, line 540 at r7 (raw file):
#$params.WebAppPool = $webapplication.applicationpool #$params.PhysicalPath = $webapplication.PhysicalPath
Let's remove the dead code if it is no longer needed
DSCResources/MSFT_xWebAppPool/MSFT_xWebAppPool.psm1, line 980 at r7 (raw file):
function Export-TargetResource
For all the Export-TargetResource functions, let's include comment based help. Since there are no parameters, it can just be a .DESCRIPTION or .SYNOPSIS
DSCResources/MSFT_xWebAppPool/MSFT_xWebAppPool.psm1, line 994 at r7 (raw file):
foreach($appPool in $appPools)
Should be one space after the keyword
DSCResources/MSFT_xWebAppPool/MSFT_xWebAppPool.psm1, line 1004 at r7 (raw file):
if($appPool.ProcessModel -eq 'SpecificUser')
Should be one space after the keyword
DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 1208 at r7 (raw file):
$AuthenticationTypes
variable should be camelCase
Modules/ReverseDSCCollector.psm1, line 18 at r7 (raw file):
$ResourcesPath
variables should be camelCase (per Style guidelines). Can you also correct the other variables throughout this file? The only pascal names should be $Path since it is a parameter and $PSScriptRoot since it is an automatic variable.
|
There are a lot of duplicated code introduced with this change, I also want to look over this to see that it actually the correct way of doing this. Please continue resolving comments on this change, but I will add an 'on hold' label for the time-being so we have chance to discuss this. I have offline conversation with @NikCharlebois about this, but I have not have time to get back to that yet. |
johlju
added
the
on hold
Pull Request (PR) description
Native ReverseDSC Support
This Pull Request (PR) fixes the following issues
N/A
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is