-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RTA: xFTP #202
RTA: xFTP #202
Conversation
… README, moved shared resources to helper.psm1 from xWebsite,xWebApplication and xFTP, Updated unit tests for said resources to reflect that movement
@nzspambot - Any updates on this? |
@mbreakey3 I was hoping/waiting for a review/feed back from someone |
{ | ||
{($_ -eq 'Website') -or ($_ -eq 'Ftp')} | ||
{ | ||
$prop = Get-WebConfigurationProperty ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be null. null check? error handling?
|
||
Application | ||
{ | ||
$prop = Get-WebConfigurationProperty ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be null. null check? error handling?
foreach ($type in @('Anonymous', 'Basic', 'Digest', 'Windows')) | ||
{ | ||
|
||
$expected = $AuthenticationInfo.CimInstanceProperties[$type].Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can CimInstanceProperties be $null?
foreach ($type in @('Anonymous', 'Basic', 'Digest', 'Windows')) | ||
{ | ||
|
||
$expected = $AuthenticationInfo.CimInstanceProperties[$type].Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the code is common within switch cases. You should be able to condense this code by moving some common stuff outside
|
||
) | ||
|
||
$CurrentLogFlags = (Get-Website -Name $Name).logfile.logExtFileFlags -split ',' | Sort-Object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Get-Website -Name $Name) - can this be $null or throw an exception?
[Microsoft.Management.Infrastructure.CimInstance[]] $AuthorizationInfo | ||
) | ||
|
||
$currentFtpAuthorization = (get-webconfiguration '/system.ftpServer/security/authorization' ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of common code shared across Authorizarion.Users and Authorization.Roles. Should be able to move that to a separate function
CertificateHash | ||
{ | ||
$correctValue = 'serverCertHash' | ||
Set-ItemProperty ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set-ItemProperty `
-
-Path "IIS:\Sites\$Name" `
-
-Name "ftpServer.security.ssl.$correctValue" `
-
-Value ($SslInfo.CimInstanceProperties | `
-
Where-Object {$_.Name -eq $value}).Value
Set-ItemProperty is getting repeated for every switch case. Please move this to the end, out of switch. Switch only sets the $correctValue
@@ -0,0 +1,169 @@ | |||
$script:DSCModuleName = 'xWebAdministration' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a description of the scenario this Int test is testing?
@@ -0,0 +1,2257 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a description as in uber tests that this test script is testing
} | ||
} | ||
|
||
Context 'Check PhysicalPath is different' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what the word 'different' means wrt these tests. i would change it to something more concrete.
Same comment for the other context descriptions following this one
Review status: 0 of 13 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed. DSCResources/Helper.psm1, line 350 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
no DSCResources/Helper.psm1, line 360 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
No DSCResources/Helper.psm1, line 408 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
No DSCResources/Helper.psm1, line 425 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
This is not possible. DSCResources/Helper.psm1, line 488 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
No DSCResources/Helper.psm1, line 804 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
What does this even mean? DSCResources/MSFT_xFTP/MSFT_xFTP.psm1, line 956 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
Not possible DSCResources/MSFT_xFTP/MSFT_xFTP.psm1, line 1237 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
Not possible Tests/Integration/MSFT_xFTP.Integration.Tests.ps1, line 1 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
What does this even mean? Tests/Unit/MSFT_xFTP.test.ps1, line 1 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
This follows the style of the other int tests Tests/Unit/MSFT_xFTP.test.ps1, line 357 at r1 (raw file): Previously, eshaparmar (Esha Parmar) wrote…
Different: not the same as another or each other ie if the path is different return $false Comments from Reviewable |
@eshaparmar help me understand you comments please. most of them make zero sense to me. thanks |
Review status: 0 of 13 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed. DSCResources/Helper.psm1, line 350 at r1 (raw file): Previously, nzspambot (James Baker) wrote…
as there is always authentication on sites. This will always return something. This should be clear as Get-DefaultAuthenticationInfo exists DSCResources/Helper.psm1, line 360 at r1 (raw file): Previously, nzspambot (James Baker) wrote…
as there is always authentication on sites. This will always return something. This should be clear as Get-DefaultAuthenticationInfo exists DSCResources/Helper.psm1, line 408 at r1 (raw file): Previously, nzspambot (James Baker) wrote…
as there is always authentication on sites. This will always return something. This should be clear as Get-DefaultAuthenticationInfo exists DSCResources/Helper.psm1, line 425 at r1 (raw file): Previously, nzspambot (James Baker) wrote…
This is due to the fact website requires 4 types of auth, application requires 4 types of auth and and application and ftp requires 2 types of auth. Again this is clear in the code. You do need to trace the function however. DSCResources/Helper.psm1, line 488 at r1 (raw file): Previously, nzspambot (James Baker) wrote…
Get-website does not throw when it does not exist. since it is a compare who cares if it is $null? that means it needs to be set DSCResources/MSFT_xFTP/MSFT_xFTP.psm1, line 956 at r1 (raw file): Previously, nzspambot (James Baker) wrote…
Due to the fact that if you're doing a user you only use 3/4 objects but you still need to provide the 4th. As you can see I'm compare of different propertys. DSCResources/MSFT_xFTP/MSFT_xFTP.psm1, line 1237 at r1 (raw file): Previously, nzspambot (James Baker) wrote…
This may be unclear but I needed to do some transforms. either its this way or a lot of if blocks Comments from Reviewable |
@nzspambot Impressive work on this PR! I reopen this PR since it is part of an open issue. If you still want to work on this PR I can do a review from scratch on this PR, I will label this as abandoned until I hear back from you, if not I hope someone will continue your work. Thanks for your contribution! |
@nzspambot |
So this took longer than expected but here is a draft of xFTP
Added what I thought was the basic stuff for a New FTP site, did unit and int tests and an example
I've also moved some stuff around into helper.psm1 as it is now shared across upto 3 resources.
Is this ok? is Helper where this should be?
Take this at this point as draft and subject to change; after if you are happy where this is at the moment
thanks
This change is