BREAKING CHANGE: FTP

[t3mi]

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

• [x] Added an entry under the Unreleased section of the change log in the README.md. Entry should say what was changed, and how that affects users (if applicable).
• [x] Resource documentation added/updated in README.md.
• [x] Resource parameter descriptions added/updated in README.md, schema.mof and comment-based help.
• [x] Comment-based help added/updated.
• [x] Localization strings added/updated in all localization files as appropriate.
• [x] Examples appropriately added/updated.
• [x] Unit tests added/updated. See DSC Resource Testing Guidelines.
• [x] Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• [x] New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[msftclas]

All CLA requirements met.

[gaelcolas]

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? https://ci.appveyor.com/project/PowerShell/xwebadministration/builds/24663455?fullLog=true#L826

[regedit32]

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 FTP. At some point we will rename older resources and bring them up to HQRM standards in the process, but this will make it easier if we follow this practice with all new resources moving forward.

[t3mi]

@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 [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUsingConvertToSecureStringWithPlainText", "")] could be considered as well?

@regedit32 ok.

[codecov-io]

Codecov Report

Merging #425 into dev will increase coverage by 1.15%. The diff coverage is 96.86%.

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

Impacted Files	Coverage Δ
DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1	96.39% <100%> (-0.73%)	:arrow_down:
...ces/MSFT_xWebApplication/MSFT_xWebApplication.psm1	97.87% <94.73%> (+7.12%)	:arrow_up:
DSCResources/MSFT_FTP/MSFT_FTP.psm1	95.71% <95.71%> (ø)
DSCResources/Helper.psm1	97.2% <98.22%> (+7.2%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3ed63bc...f501470. Read the comment docs.

[regedit32]

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.

[t3mi]

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

[regedit32]

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.

[t3mi]

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.

[danielboth]

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

[regedit32]

Hopefully I will get back to this over the weekend to provide feedback. Anyone is welcome to help review in the meantime.

[danielboth]

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.

[johlju]

@t3mi do you have time to work on this so we can get this over the finish line?

[t3mi]

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

[danielboth]

Hi @t3mi, any updates from you on this one? Would be great to finish it.

[t3mi]

@danielboth yes, I'm working on this.

[danielboth]

Let me check-in here as I'm wondering what progress has been made.

[markatdxb]

Hello, just looking at the thread, has the FTP been already implemented ?

[davetapley]

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.

[johlju]

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