RTA: xSMTP #162
RTA: xSMTP #162
Conversation
|
Not to be a pest but would it be possible to call it SMTPServer or SMTPHost or something to clarify what its purpose is? Reviewed 7 of 7 files at r1. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 11 [r1] (raw file):
Going to need to add four spaces to all of these or @mbreakey3 will come scowl at you. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 91 [r1] (raw file):
~~I'm not sure I understand this ValidateSet what's this doing? ~~ Why are we limiting ourselves to the default SMTP? Couldn't we be managing multiple SMTP servers? DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 256 [r1] (raw file):
So just a thought here, could we possibly do a loop instead of each check individually? You could use the following code as a basis: https://github.com/PowerShell/xNetworking/blob/dev/DSCResources/MSFT_xFirewall/MSFT_xFirewall.psm1#L41 Then we could do something like: foreach ($parameter in $ParameterList)
{
if ($PSBoundParameters.ContainsKey($Parameter) -and $Result.$Parameter -ne $PSBoundParameter[$Parameter]
{
Write-Verbose -Message "Updated SMTPSetting {0} from {1} to {2}" -f $Parameter, $Result.$Parameter, $PSBoundParameter[$Parameter]
Set-SMTPSettings `
-Name $Name `
-Setting $Parameter
-Value $PSBoundParameter[$Parameter]
}
}
This would cover all of the setting/checking and break it down into a more "simple"* loop.
DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 257 [r1] (raw file):
Could we move this over four spaces? if (($PSBoundParameters.ContainsKey('AuthFlags') -and `
$Result.AuthFlags -ne $AuthFlags))
{
#code stuff
}Actually you can assume that I'd like to see this all the way down. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 272 [r1] (raw file):
Are we being proactive or does this actually fail if the folder doesn't exist? DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 717 [r1] (raw file):
Same comments for Set-TargetResource as for here as a way to tighten up the loop of things we can do. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1009 [r1] (raw file):
I think the style is to have this outside of the function? Honestly I'm not sure here but you need at least four more spaces. <#
.SYNOPSIS
#>DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1029 [r1] (raw file):
tab I think? DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1042 [r1] (raw file):
Think you got to many ss here. I think your variable should probably be $ExistingBindingsToCheck (note one less s) DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1050 [r1] (raw file):
Think we can just get away with doing: $ExistingToCheck = $ExistingToCheck | Sort-Object -Unique
$InputToCheck = $InputToCheck | Sort-Object -UniqueDSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1124 [r1] (raw file):
Missalinged comment. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1136 [r1] (raw file):
Tab? DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1143 [r1] (raw file):
Email is mandatory shouldn't this always be true? And if so why do we need it? DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1148 [r1] (raw file):
If our output is supposed to be a boolean why are we throwing here? Personal preference, test- cmdlets shouldn't throw only return true/false. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1183 [r1] (raw file):
Could you remove this space here please. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1193 [r1] (raw file):
Also do we need to terminate here? Could we just return false? DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 1212 [r1] (raw file):
I think if you have multiple ServerBindings wouldn't this exist the function before all of them have been tested? I think you want to move your return $true outside of the foreach loop. DSCResources/MSFT_xSMTP/MSFT_xSMTP.schema.mof, line 4 [r1] (raw file):
I'm not seeing $name in the schema, should this be $name and not $id? Tests/Integration/MSFT_xSMTP.config.ps1, line 45 [r1] (raw file):
Need a new line here. Tests/Integration/MSFT_xSMTP.config.psd1, line 5 [r1] (raw file):
Does tthis not work if we have it as: Name = 1That would cast it as an int instead of a string. Tests/Integration/MSFT_xSMTP.config.psd1, line 40 [r1] (raw file):
Need a new line here. Tests/Integration/MSFT_xSMTP.Integration.tests.ps1, line 28 [r1] (raw file):
Could we backup the web config before loading the config file please? Tests/Integration/MSFT_xSMTP.Integration.tests.ps1, line 47 [r1] (raw file):
tab? Tests/Integration/MSFT_xSMTP.Integration.tests.ps1, line 100 [r1] (raw file):
New line here Tests/Unit/MSFT_xSMTP.tests.ps1, line 14 [r1] (raw file):
Need some tabs here Tests/Unit/MSFT_xSMTP.tests.ps1, line 24 [r1] (raw file):
tabs? Tests/Unit/MSFT_xSMTP.tests.ps1, line 325 [r1] (raw file):
I'm not sure this is doing what you think it is. Because you are doing a return $false this would return out on the first check and be done, so it doesn't really matter that all of them are wrong it's only checking the first one. I'm not sure this is actually valuable. Tests/Unit/MSFT_xSMTP.tests.ps1, line 347 [r1] (raw file):
So consider this advice for all of these. I would consider adding a check for the verbose message to validate you are hitting the line you think you are. In your code you do a return $false the first time you run into something that's wrong, which imo is good practice no sense in checking the rest of it. This would be tested like this: $Result = Test-TargetResource -Name $MockParamaters.Name -AuthFlags $MockParamaters.AuthFlags -Verbose 4>&1
It 'should return False' {
$Result[0] | Should Be $false
}
It 'should return the correct message' {
$result[1] | Should Be $LocalizedData.VerboseTestTargetFalseAuthFlags
}Tests/Unit/MSFT_xSMTP.tests.ps1, line 817 [r1] (raw file):
Your spacing gets all of from about here down in the code. Your closing brace should line up with Mock and all functions below it should line up as well. Might want to take a pass through this. Tests/Unit/MSFT_xSMTP.tests.ps1, line 821 [r1] (raw file):
I would consider adding ParameterFilter to the mock, here we're not 100% sure we've called the right thing at the right time. This would look like. Mock Set-SMTPSettings {}
Mock -CommandName Set-SMTPSettings -ParameterFilter { $Setting -eq 'AuthFlags' -and $Value -eq $MockParameters.AuthFlags}
It 'should call the expected mocks' {
$Result = Set-TargetResource -Name $MockParamaters.Name -AuthFlags $MockParamaters.AuthFlags
Assert-MockCalled -CommandName Set-SMTPSettings -Exactly 0
Assert-MockCalled -CommandName Set-SMTPSettings -ParameterFilter { $Setting -eq 'AuthFlags' -and $Value -eq $MockParameters.AuthFlags} -Exactly 1
}This is not only validating that you called the right function with the right call but you didn't call it some other way. Comments from Reviewable |
|
You're the boss; whatever you want you can have Review status: all files reviewed at latest revision, 30 unresolved discussions, some commit checks failed. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 11 [r1] (raw file):
|
|
@tysonjhayes made some changes based on your review but I am not in position to test them right now sorry so expect them to not work and fail |
|
Review status: 1 of 7 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 272 [r1] (raw file):
|
|
Reviewed 5 of 6 files at r2, 1 of 1 files at r3. Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 272 [r1] (raw file):
|
|
Review status: 5 of 7 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 272 [r1] (raw file):
|
|
Reviewed 2 of 2 files at r4. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 359 [r4] (raw file):
You can remove the parameter and make a note saying we don't need to specify it. I'd do this before the loop. https://clan8blog.wordpress.com/2014/06/26/removing-a-parameter-from-psboundparameters/ Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed. DSCResources/MSFT_xSMTP/MSFT_xSMTP.psm1, line 272 [r1] (raw file):
|
|
Reviewed 4 of 4 files at r5. Comments from Reviewable |
|
@tysonjhayes I want to come back to this but I feel like this is going to go nowhere fast due to the issues I encountered |
kwirkykat
added
the
waiting for author response
|
@nzspambot are you still planning to work on this? or can we close this pr? |
|
@eshaparmar I would like to get back at looking at this but won't be anytime soon so yeah I will close this for now |
vors
removed
the
waiting for author response
|
As this is part of an open issue, I will reopen this I leave it as abandoned so someone else can continue the work. |
Ok here is a 95%(?) complete xSMTP resource
Things to do
add more unit tests for the helper functione: Added these testsThings which don't work
Things to note
Get-TargetResourcein the integration tests will take a 💩This change is