BREAKING CHANGE: FTP #425
BREAKING CHANGE: FTP #425
Conversation
… README, moved shared resources to helper.psm1 from xWebsite,xWebApplication and xFTP, Updated unit tests for said resources to reflect that movement
|
|
|
Thanks a lot for this contribution. I don't know if you've noticed but some tests aren't passing. Any chance you could try fixing them? |
|
Also, before we get too far in the review, we don't need to keep the 'x' prefix in the name. Similar to the latest resource added to this module, WebApplicationHandler, we can start naming resources without any prefixes, so perhaps just |
|
@gaelcolas thanks. I'm using clear text password in Get function to construct credential object. Is switching to every property check the only route or suppressing failed rule using @regedit32 ok. |
Codecov Report
@@ Coverage Diff @@
## dev #425 +/- ##
==========================================
+ Coverage 90.77% 91.92% +1.15%
==========================================
Files 17 18 +1
Lines 2438 2835 +397
==========================================
+ Hits 2213 2606 +393
- Misses 225 229 +4
Continue to review full report at Codecov.
|
|
Hi @t3mi , nice work on this! Can we separate the changes into two PRs? One for the new FTP resource and another for the changes to xWebsite and xWebApplication? They are both big changes in their own right and would be good to have a separate issue opened for the changes to xWebsite and xWebApplication and describe what we're addressing there. It's a large PR and will be easier to review if we can keep the changes scoped a bit more. |
|
@regedit32 thanks. I would prefer to not split as all the changes to xWebsite and xWebApplication are from shared functions which were moved to Helper module. The only piece which could be submitted as different PR for is change to default values for AuthenticationInformation but its very small. |
|
Oh I see, you consolidated reusable functions for this new resource. Thanks for that! Can you describe the breaking change a little more? If we're going to include a breaking change, I want to make sure we're clear on the where/why for tracking purposes. |
|
Sure. If AuthenticationInfo parameter was not specified for resources xWebsite or xWebApplication current values will be compared with default values (all available authentication options set to $false). So if any authentication option was enabled outside of DSC it will be reverted to $false unless AuthenticationInfo was set. The purpose for such change is that both resources had initial check and set of AuthenticationInfo to default values if none was provided but it wasn't working because condition had $PSBoundParameters.ContainsKey() as one of the checks. |
|
Nice work @t3mi, had a quick glance at the code, can't wait for this to be merged in! @regedit32 / @kwirkykat can we get this one in for the next DSC resource kit release please?! |
|
Hopefully I will get back to this over the weekend to provide feedback. Anyone is welcome to help review in the meantime. |
regedit32
added
breaking change
There was a problem hiding this comment.
Reviewed 4 of 14 files at r2, 11 of 11 files at r3.
Reviewable status: all files reviewed, 46 unresolved discussions (waiting on @t3mi)
DSCResources/Helper.psm1, line 374 at r3 (raw file):
$cimNamespace = 'root/microsoft/Windows/DesiredStateConfiguration' switch($IisType)
Can you put a space between switch and the open parentheses?
DSCResources/Helper.psm1, line 449 at r3 (raw file):
) $Location = "${Site}"
non-parameter Location should use camelCase
DSCResources/Helper.psm1, line 453 at r3 (raw file):
if ($IisType -eq 'Application') { $Location = "${Site}/${Application}"
If this is just to get to the right location on the IIS drive, why a forward slash?
Maybe even better: Join-Path -Path $Site -ChildPath $Application, and then repeat that with the IIS;\Sites logic
DSCResources/Helper.psm1, line 513 at r3 (raw file):
) $Properties = ($AuthenticationInfo.CimInstanceProperties | Where-Object {$null -ne $_.Value}).Name | Sort-Object
non-parameter Properties should use camelCase
DSCResources/Helper.psm1, line 567 at r3 (raw file):
) $Location = "${Site}"
non-parameter Location should use camelCase
DSCResources/Helper.psm1, line 571 at r3 (raw file):
if ($IisType -eq 'Application') { $Location = "${Site}/${Application}"
Same comment about slash direction and can be improved with Join-Path
DSCResources/Helper.psm1, line 635 at r3 (raw file):
) $Properties = ($AuthenticationInfo.CimInstanceProperties | Where-Object {$null -ne $_.Value}).Name | Sort-Object
non-parameter Properties should use camelCase
DSCResources/Helper.psm1, line 691 at r3 (raw file):
) $CurrentLogFlags = switch($FtpSite.IsPresent)
non-parameter CurrentLogFlags should use camelCase, same for ProposedLogFlags
Add a space after switch
DSCResources/Helper.psm1, line 780 at r3 (raw file):
) $WebSiteASP = (Get-WebConfiguration `
non-parameter WebsiteASP should use camelCase,
Same for ExistingObject, ProposedObject and ErrorMessage
DSCResources/Helper.psm1, line 792 at r3 (raw file):
}) if(-not $ExistingObject)
Add space after if
DSCResources/Helper.psm1, line 797 at r3 (raw file):
} if(-not (Compare-Object -ReferenceObject $ExistingObject `
Add space after if
DSCResources/Helper.psm1, line 801 at r3 (raw file):
-Property name)) { if(Compare-Object -ReferenceObject $ExistingObject `
Add space after if
DSCResources/Helper.psm1, line 852 at r3 (raw file):
) $Website = Get-Website | Where-Object -FilterScript {$_.Name -eq $Name}
non-parameter Website should use camelCase
Same for ReferenceObject, ExcludeStopeed, OtherWebsiteFilter, DifferenceObject, CompareSplat and Result
DSCResources/Helper.psm1, line 1077 at r3 (raw file):
} $bindingInformation = $IPAddressString, `
Backticks are also not required after a comma ( , )
DSCResources/Helper.psm1, line 1201 at r3 (raw file):
{ <# WebAdministration can throw the following exception if there are non-standard
Excellent additional information! :)
DSCResources/Helper.psm1, line 1371 at r3 (raw file):
try { $IsValid = [UInt16]$InputString -ne 0
non-parameters should use camelCase
Examples/Sample_xFTP_NewFTPSite.ps1, line 37 at r3 (raw file):
PhysicalPath = $FTPSitePath State = 'Started' AuthorizationInfo = @(
This example misses AuthenticationInfo, which would be nice to add, so it's clear that Authentication needs to be configured next to Authorization.
Tests/Unit/Helper.Tests.ps1, line 1097 at r3 (raw file):
It 'Should call expected mocks' { Assert-MockCalled -CommandName Get-WebConfigurationProperty -Exactly ($testCase.IisType -ne 'Ftp')
($testCase.IisType -ne 'Ftp') is not returning a number here, so I had to test what this is doing. It works since $true casts to [int] 1, but that's not very clear from this code. It would already be better if you cast it to int yourself so: [int]($testCase.IisType -ne 'Ftp')
Other than that, I feel it would be even better to just write -Times 0 or -Times 1, what's the benefit of doing it like this?
Tests/Unit/Helper.Tests.ps1, line 1198 at r3 (raw file):
) foreach($testCase in $testCases)
Space after foreach
Tests/Unit/Helper.Tests.ps1, line 1232 at r3 (raw file):
} foreach($truthyTestCase in $truthyTestCases)
Space after foreach
Tests/Unit/Helper.Tests.ps1, line 2016 at r3 (raw file):
Mock -CommandName Clear-WebConfiguration -Verifiable foreach($testCase in $testCases)
Add a space between foreach and (
Tests/Unit/Helper.Tests.ps1, line 2234 at r3 (raw file):
) foreach($testCase in $testCases)
Add a space after foreach
Tests/Unit/MSFT_xWebApplication.Tests.ps1, line 4 at r3 (raw file):
$script:DSCResourceName = 'MSFT_xWebApplication' $script:DSCHelperModuleName = 'Helper'
Same generic comment about camelCase variable names applies for this file as well
Tests/Unit/MSFT_xWebsite.Tests.ps1, line 4 at r3 (raw file):
$script:DSCResourceName = 'MSFT_xWebsite' $script:DSCHelperModuleName = 'Helper'
Same generic comment about camelCase variable names applies for this file as well
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 111 at r3 (raw file):
Specifies username for access to physical path if required. .PARAMETER PhysicalPathAccessPass
This password is implemented as a plaintext string. Doing that means users of this resource are encouraged to put their passwords in plaintext in the configuration. I would rather see that PhysicalPathAccessAccount and PhysicalPathAccessPass get combined in a pscredential object. That way we can at least deliver it to the LCM in a secure way.
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 353 at r3 (raw file):
} | ForEach-Object -Begin { $NewftpSiteSplat = @{}
non-parameter NewftpSiteSplat should use camelCase
Same for: LocalizedData
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 369 at r3 (raw file):
# Set default port to FTP:21 else it will be HTTP:80 if(-not(($PSBoundParameters.ContainsKey('BindingIfo'))))
BindingIfo is misspelled, should be BindingInfo
Also, please add a space between if and the open parentheses
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 374 at r3 (raw file):
} if ([bool]([System.Uri]$PhysicalPath).IsUnc)
[bool] is not required here, IsUnc is a boolean by default.
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 411 at r3 (raw file):
# Update physical path access username if required if ($PSBoundParameters.ContainsKey('PhysicalPathAccessAccount') -and `
A backtick behind -and is not required, code will work fine without it. I won't make this statement everywhere, but please clean it everywhere, just makes it so much more readable.
It's the same with -or as well.
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 647 at r3 (raw file):
if ($PSBoundParameters.ContainsKey('LogTruncateSize')) { Write-Verbose -Message ($LocalizedData.WarningLogPeriod -f $Name)
In general, other resources return an error to the user when an unsupported configuration has been setup. This way of silently handling such an unsupported configuration might lead to unexpected results for the user of the resource. I would prefer to just return an error if the user specified both LogPeriod and LogTruncateSize.
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 1006 at r3 (raw file):
Assert-Module $InDesiredState = $true
non-parameter InDesiredState should use camelCase
Same for: LocalizedData
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 1233 at r3 (raw file):
if ($PSBoundParameters.ContainsKey('LoglocalTimeRollover') -and ` ($LoglocalTimeRollover -ne ` ([System.Convert]::ToBoolean($ftpSite.ftpServer.logFile.localTimeRollover))))
Is there some version specific thing that you need to convert it to boolean? (As my test system returns this as boolean itself, W2016).
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 1300 at r3 (raw file):
) $CurrentDirectoryBrowseflags = (Get-Website -Name $Site).ftpServer.directoryBrowse.showFlags `
non-parameter CurrentDirectoryBrowseflags should use camelCase
Same for ProposedDirectoryBrowseflags
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 1362 at r3 (raw file):
} $existingFtpAuthorizationInfo = $CurrentAuthorizationCollection | `
Same comment as with -and and -or, after a pipe | there is not backtick required to continue on the next line.
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 1366 at r3 (raw file):
Select-Object accessType,users,roles,permissions $existingObject=$()
Can you put some spaces around the equal sign?
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 1369 at r3 (raw file):
$existingObject += foreach($existingAuthorization in $existingFtpAuthorizationInfo) { $currentObject = New-Object -TypeName PSObject `
This is pure preference, but in general I would prefer this:
[pscustomobject]@{
accessType = $existingAuthorization.accessType
users = $existingAuthorization.users
roles = $existingAuthorization.roles
permissions = $existingAuthorization.permissions
}
Helps you with keeping the code nicely formatted and doesn't require backticks.
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 1419 at r3 (raw file):
) $Store = $SslInfo.CertificateStoreName
non-parameter Store should use camelCase
Same for, Hash and Properties
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 1422 at r3 (raw file):
$Hash = $SslInfo.CertificateThumbprint if($null -ne $Hash -and -not(Test-Path -Path Cert:\LocalMachine\${Store}\${Hash}))
Add a space between if and open parentheses
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 1454 at r3 (raw file):
-Property $Properties if($null -ne $compare)
Add a space between if and open parentheses
DSCResources/MSFT_FTP/MSFT_FTP.psm1, line 1672 at r3 (raw file):
} foreach ($Authorization in $AuthorizationInfo)
non-parameter Authorization should use camelCase
Tests/Integration/MSFT_FTP.config.psd1, line 15 at r3 (raw file):
AuthenticationInfoAnonymous = $false AuthenticationInfoBasic = $true AuthorizationInfoAccessType1 = 'Allow'
The names of the variables are a bit cryptic here, why not just:
AuthorizationInfoAccessTypeAllow
AuthorizationInforAccessTypeDeny
(and the same for User and Permissions)
That will also make the integration tests more readable on what happens!
Tests/Unit/MSFT_FTP.tests.ps1, line 4 at r3 (raw file):
$script:DSCResourceName = 'MSFT_FTP' $script:DSCHelperModuleName = 'Helper'
In general, since this script does not accept parameters, all parameters should be in camelCase
Tests/Unit/MSFT_FTP.tests.ps1, line 1196 at r3 (raw file):
Set-TargetResource @MockParameters It 'Should call all the mocks' {
In general, I like the way the tests for Test-TargetResource are structured way better than these tests. If this test now fails, that tells just "something failed", but we don't know what or where.
I think it's much more beneficial to future contributors to split the tests so each individual parameter is tested if it sets successfully, leaving the end-to-end test to the integration tests.
I did have a look at the other tests in this module as well, and see they follow a similar model, so I don't think this is something to decline this PR on, I do think it's valuable to get this discussion started, so would also love to hear your opinion.
Tests/Unit/MSFT_FTP.tests.ps1, line 1197 at r3 (raw file):
It 'Should call all the mocks' { Assert-MockCalled -CommandName Set-ItemProperty -Exactly 18
Use named parameters for function and cmdlet calls rather than positional parameters, so all these should read:
Assert-MockCalled -CommandName Set-ItemProperty -Exactly -Times 18
This is due to Exactly being a switch parameter.
Tests/Unit/MSFT_FTP.tests.ps1, line 1328 at r3 (raw file):
} Describe "how $DSCResourceName\Compare-DirectoryBrowseFlags responds" {
Since DirectoryBrowseFlags accepts multiple flags, can you add a test that covers that scenario?
Tests/Unit/MSFT_FTP.tests.ps1, line 1763 at r3 (raw file):
It 'Should call Get-WebConfiguration 4 times' { Assert-MockCalled -CommandName Get-WebConfiguration -Exactly 1
This test does not match what the It block tells it will do
|
Hi @t3mi, @regedit32, Ok so I went for a full review of this resource this weekend. Not only reviewing the code, but also testing it on a test system. One learning from that one that, even if the IIS server is not setup as a FTP server (missing the Web-Ftp-Server role), this resource will apply the configuration like everything should work. That would be a nice check to add. Another discussion point is the setup of username / password as plaintext strings, I've added a comment in reviewable, so we can follow up on that one. I have to say, awesome work on all the refactoring @t3mi. Also, the FTP resource went through all the tests I did on my test system properly as well. Feels production ready and well setup with full coverage for all the FTP properties! On the review I did, I spent a good number of hours on this, but I went through the tests a bit less rigorous as I did through the code implementing the FTP resource. Mainly because reviewable just has a hard time perfoming properly on these large diffs, so apologies if I missed something there. For all comments around syntax, I took most of these from https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md, except for the backticks part where we should possible add that backticks should only be used when required. I hope we can agree on that. |
|
@t3mi do you have time to work on this so we can get this over the finish line? |
|
@johlju I'll be able to finalize all my PRs once I'm back from vacation in the mid of September. @danielboth thanks for spending your time to review this! |
|
Hi @t3mi, any updates from you on this one? Would be great to finish it. |
|
@danielboth yes, I'm working on this. |
There was a problem hiding this comment.
Let me know if you need any more help @t3mi, thanks!
Reviewable status: all files reviewed, 46 unresolved discussions (waiting on @t3mi)
|
Let me check-in here as I'm wondering what progress has been made. |
|
Hello, just looking at the thread, has the FTP been already implemented ? |
|
Paging @t3mi and @danielboth, looks like both @markatdxb and myself are interested in this. I'm fairly new to all this (I discovered it via Ansible), but I'm happy to help if I can. |
|
@davetapley I think this PR has been abandoned since there has been no commits since the review. The best option would be to send a PR to @t3mi's working branch, but that means @t3mi need to be around to merge it thpugh. Other option is to fork the @t3mi's working branch to continue the work by sending in a new PR as per https://dsccommunity.org/guidelines/contributing/#how-to-continue-working-on-a-pull-request-pr-when-an-author-contributor-is-unable-to-complete-it. |
Pull Request (PR) description
This is continued work from PR#202 started by @nzspambot. All the latest changes were integrated, tests and some function logic were optimized and fixed. Please review.
This Pull Request (PR) fixes the following issues
Adds new resource FTP for managing FTP sites. Fixes #81
BREAKING CHANGES: For xWebsite and xWebApplication if AuthenticationInformation was not specified Default (all $false) is assumed.
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is