WebAdministrationDsc icon indicating copy to clipboard operation
WebAdministrationDsc copied to clipboard

Attempt to fix MSFT_xIisModule

Open scottmckenzie opened this issue 7 years ago • 11 comments

  • Changes to MSFT_xIisModule
    • Fix verb parsing.
    • Get fastCgi setting from webServer config.
    • Set handler setting on website.
    • Fix various typos.

Should fix #305 and #323


This change is Reviewable

scottmckenzie avatar May 14 '18 11:05 scottmckenzie

CLA assistant check
All CLA requirements met.

msftclas avatar May 14 '18 11:05 msftclas

Codecov Report

Merging #354 into dev will not change coverage. The diff coverage is 0%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #354   +/-   ##
===================================
  Coverage    89%    89%           
===================================
  Files        14     14           
  Lines      2210   2210           
===================================
  Hits       1983   1983           
  Misses      227    227

codecov-io avatar May 14 '18 11:05 codecov-io

Reviewed 1 of 1 files at r1. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


a discussion (no related file): Could you possible please add unit test(s) for these changes?


a discussion (no related file): Could you please add an descriptive entry, for each fix, to the change log under the the Unreleased section in the file README.md? If an entry resolves an issue please reference the issue.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 79 at r1 (raw file):

iis:\

Why not use Get-IisSitePath here as well? Maybe without the -SiteName parameter if it can only be found on "root level"?


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 163 at r1 (raw file):

$resourceTests.EndPointSetup

This should be $resourceStatus.EndPointSetup?


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 398 at r1 (raw file):

                    ModulePresent = $modulePresent
                    ModuleConfigured = $moduleConfigured
                    EndPointSetup = $resourceStatus.EndPointSetup

This should not be needed? See previous comment.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 407 at r1 (raw file):

                    ModulePresent = $modulePresent
                    ModuleConfigured = $moduleConfigured
                    EndPointSetup = $resourceStatus.EndPointSetup

This should not be needed? See previous comment.


Comments from Reviewable

johlju avatar May 14 '18 12:05 johlju

@scottmckenzie Could you please go into Reviewable and write 'Done' on the review comments that are resolved (or comment on them if they need to be discussed). After you replied to all review comments (they are saved as drafts), then press the big Publish button at the top of the page. That will send all review comments as one big comment back to this PR.

You find Reviewable in the big purple button in the PR description.

johlju avatar May 15 '18 08:05 johlju

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 79 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
iis:\

Why not use Get-IisSitePath here as well? Maybe without the -SiteName parameter if it can only be found on "root level"?

Done.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 163 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$resourceTests.EndPointSetup

This should be $resourceStatus.EndPointSetup?

Done.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 398 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should not be needed? See previous comment.

Done.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 407 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should not be needed? See previous comment.

Done.


Comments from Reviewable

scottmckenzie avatar May 15 '18 11:05 scottmckenzie

The changes looks good, although it would be great if you could add a unit test for testing these changes (the last review comment). Unfortunately there are no existing unit test for this resource. There is a template to use here; https://github.com/PowerShell/DscResources/blob/master/Tests.Template/unit_template.ps1

Let me know if you need any help.


Reviewed 2 of 2 files at r2. Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

johlju avatar May 16 '18 08:05 johlju

Attempting to write some unit tests...

scottmckenzie avatar May 17 '18 08:05 scottmckenzie

@scottmckenzie Awesome! If you get stuck let me know and I might help with pointers. 🙂

johlju avatar May 18 '18 10:05 johlju

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

stale[bot] avatar Jun 18 '18 23:06 stale[bot]