RTA: xFTP #202

nzspambot · 2016-07-28T05:50:14Z

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

… README, moved shared resources to helper.psm1 from xWebsite,xWebApplication and xFTP, Updated unit tests for said resources to reflect that movement

mbreakey3 · 2016-08-12T00:00:12Z

@nzspambot - Any updates on this?

nzspambot · 2016-08-17T22:45:56Z

@mbreakey3 I was hoping/waiting for a review/feed back from someone

eshaparmar · 2016-09-21T18:34:40Z

DSCResources/Helper.psm1

+    {
+        {($_ -eq 'Website') -or ($_ -eq 'Ftp')}
+        {
+            $prop = Get-WebConfigurationProperty `


Can this be null. null check? error handling?

eshaparmar · 2016-09-21T18:35:23Z

DSCResources/Helper.psm1

+
+        Application
+        {
+            $prop = Get-WebConfigurationProperty `


Can this be null. null check? error handling?

eshaparmar · 2016-09-21T18:36:41Z

DSCResources/Helper.psm1

+            foreach ($type in @('Anonymous', 'Basic', 'Digest', 'Windows'))
+            {
+
+            $expected = $AuthenticationInfo.CimInstanceProperties[$type].Value


Can CimInstanceProperties be $null?

eshaparmar · 2016-09-21T18:40:41Z

DSCResources/Helper.psm1

+            foreach ($type in @('Anonymous', 'Basic', 'Digest', 'Windows'))
+            {
+
+            $expected = $AuthenticationInfo.CimInstanceProperties[$type].Value


Most of the code is common within switch cases. You should be able to condense this code by moving some common stuff outside

eshaparmar · 2016-09-21T18:45:41Z

DSCResources/Helper.psm1

+
+    )
+
+    $CurrentLogFlags = (Get-Website -Name $Name).logfile.logExtFileFlags -split ',' | Sort-Object


(Get-Website -Name $Name) - can this be $null or throw an exception?

eshaparmar · 2016-09-21T20:49:29Z

DSCResources/MSFT_xFTP/MSFT_xFTP.psm1

+        [Microsoft.Management.Infrastructure.CimInstance[]] $AuthorizationInfo
+    )
+
+    $currentFtpAuthorization = (get-webconfiguration '/system.ftpServer/security/authorization' `


Lots of common code shared across Authorizarion.Users and Authorization.Roles. Should be able to move that to a separate function

eshaparmar · 2016-09-21T20:54:54Z

DSCResources/MSFT_xFTP/MSFT_xFTP.psm1

+            CertificateHash
+            {
+                $correctValue = 'serverCertHash'
+                Set-ItemProperty `


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

eshaparmar · 2016-09-21T21:17:35Z

Tests/Integration/MSFT_xFTP.Integration.Tests.ps1

@@ -0,0 +1,169 @@
+$script:DSCModuleName   = 'xWebAdministration'


Can you add a description of the scenario this Int test is testing?

eshaparmar · 2016-09-21T21:21:46Z

Tests/Unit/MSFT_xFTP.test.ps1

@@ -0,0 +1,2257 @@
+


Add a description as in uber tests that this test script is testing

eshaparmar · 2016-09-21T21:26:22Z

Tests/Unit/MSFT_xFTP.test.ps1

+                }
+            }
+
+            Context 'Check PhysicalPath is different' {


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

nzspambot · 2017-01-18T02:46:24Z

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…

Can this be null. null check? error handling?

no

DSCResources/Helper.psm1, line 360 at r1 (raw file):

Previously, eshaparmar (Esha Parmar) wrote…

Can this be null. null check? error handling?

No

DSCResources/Helper.psm1, line 408 at r1 (raw file):

Previously, eshaparmar (Esha Parmar) wrote…

Can CimInstanceProperties be $null?

No

DSCResources/Helper.psm1, line 425 at r1 (raw file):

Previously, eshaparmar (Esha Parmar) wrote…

Most of the code is common within switch cases. You should be able to condense this code by moving some common stuff outside

This is not possible.

DSCResources/Helper.psm1, line 488 at r1 (raw file):

Previously, eshaparmar (Esha Parmar) wrote…

(Get-Website -Name $Name) - can this be $null or throw an exception?

No

DSCResources/Helper.psm1, line 804 at r1 (raw file):

Previously, eshaparmar (Esha Parmar) wrote…

here by using an inverted 'if' can reduce the lines of code even further

What does this even mean?

DSCResources/MSFT_xFTP/MSFT_xFTP.psm1, line 956 at r1 (raw file):

Previously, eshaparmar (Esha Parmar) wrote…

Lots of common code shared across Authorizarion.Users and Authorization.Roles. Should be able to move that to a separate function

Not possible

DSCResources/MSFT_xFTP/MSFT_xFTP.psm1, line 1237 at r1 (raw file):

Previously, eshaparmar (Esha Parmar) wrote…

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

Not possible

Tests/Integration/MSFT_xFTP.Integration.Tests.ps1, line 1 at r1 (raw file):

Previously, eshaparmar (Esha Parmar) wrote…

Can you add a description of the scenario this Int test is testing?

What does this even mean?

Tests/Unit/MSFT_xFTP.test.ps1, line 1 at r1 (raw file):

Previously, eshaparmar (Esha Parmar) wrote…

Add a description as in uber tests that this test script is testing

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…

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

Different: not the same as another or each other ie if the path is different return $false

Comments from Reviewable

nzspambot · 2017-01-18T02:49:32Z

@eshaparmar help me understand you comments please. most of them make zero sense to me.

thanks

nzspambot · 2017-01-19T00:18:15Z

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…

no

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…

No

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…

No

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 not possible.

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…

No

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…

Not possible

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…

Not possible

This may be unclear but I needed to do some transforms. either its this way or a lot of if blocks

Comments from Reviewable

johlju · 2018-04-25T13:46:51Z

@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!

t3mi · 2019-04-25T15:09:24Z

@nzspambot
I hope you don't mind if I continue where you left on this.

James Baker added 2 commits July 28, 2016 14:37

Added xFTP resource with Unit/Integration tests and examples, updated…

af3613c

… README, moved shared resources to helper.psm1 from xWebsite,xWebApplication and xFTP, Updated unit tests for said resources to reflect that movement

Merge from upstream/dev

e70743d

msftclas added the cla-already-signed label Jul 28, 2016

James Baker added 3 commits July 29, 2016 09:02

Moved LocalizedData message

e037338

I have failed at copy/paste

436daa7

Formatting pass on files

ca7bb29

mbreakey3 assigned narrieta Aug 1, 2016

mbreakey3 unassigned narrieta Aug 11, 2016

mbreakey3 added the needs review The pull request needs a code review. label Aug 18, 2016

eshaparmar self-assigned this Sep 20, 2016

eshaparmar suggested changes Sep 21, 2016

View reviewed changes

eshaparmar added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Sep 21, 2016

Renamed unit test and slightly formatting fixes

0b55f10

rosberglinhares mentioned this pull request May 27, 2017

MSFT_xWebsite support for FTP websites #81

Open

nzspambot closed this Jun 25, 2017

joeyaiello removed the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Jun 25, 2017

johlju reopened this Apr 25, 2018

johlju added the abandoned The pull request has been abandoned. label Apr 25, 2018

johlju unassigned eshaparmar Apr 25, 2018

nzspambot closed this Feb 19, 2019

SteveL-MSFT added this to Abandoned in powershell/dscresources May 14, 2019

t3mi mentioned this pull request May 20, 2019

BREAKING CHANGE: FTP #425

Open

9 tasks

SteveL-MSFT removed this from Abandoned in powershell/dscresources Nov 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTA: xFTP #202

RTA: xFTP #202

nzspambot commented Jul 28, 2016 •

edited by KarolKaczmarek

Loading

mbreakey3 commented Aug 12, 2016

nzspambot commented Aug 17, 2016

eshaparmar Sep 21, 2016

eshaparmar Sep 21, 2016

eshaparmar Sep 21, 2016

eshaparmar Sep 21, 2016 •

edited

Loading

eshaparmar Sep 21, 2016

eshaparmar Sep 21, 2016

eshaparmar Sep 21, 2016

eshaparmar Sep 21, 2016

eshaparmar Sep 21, 2016

eshaparmar Sep 21, 2016 •

edited

Loading

nzspambot commented Jan 18, 2017

nzspambot commented Jan 18, 2017

nzspambot commented Jan 19, 2017

johlju commented Apr 25, 2018

t3mi commented Apr 25, 2019


		)

		$CurrentLogFlags = (Get-Website -Name $Name).logfile.logExtFileFlags -split ',' \| Sort-Object

		@@ -0,0 +1,169 @@
		$script:DSCModuleName = 'xWebAdministration'

RTA: xFTP #202

RTA: xFTP #202

Conversation

nzspambot commented Jul 28, 2016 • edited by KarolKaczmarek Loading

mbreakey3 commented Aug 12, 2016

nzspambot commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eshaparmar Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eshaparmar Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

nzspambot commented Jan 18, 2017

nzspambot commented Jan 18, 2017

nzspambot commented Jan 19, 2017

johlju commented Apr 25, 2018

t3mi commented Apr 25, 2019

nzspambot commented Jul 28, 2016 •

edited by KarolKaczmarek

Loading

eshaparmar Sep 21, 2016 •

edited

Loading

eshaparmar Sep 21, 2016 •

edited

Loading