RTA: xIisRemoteManagement

[nzspambot]

Hi

As mentioned here is a IIS RemoteManagement resource with:

1. Unit tests
2. Int tests
3. example
4. updated README

thanks

---

This change is 

[kwirkykat]

@nzspambot Re-running the tests. Test failure seems out of place for just a README change.

[nzspambot]

Yeah I think it was due to something being locked by appveyor

[kwirkykat]

@nzspambot Looks like there is a missing newline now (sorry common tests were just updated to include .md files).

Then there is a very odd failure further down:
[-] Disabling IISRemoteManagment 1.22s
Expected: {False}
But was: {False}
at line: 88 in C:\projects\xwebadministration\Tests\Integration\MSFT_xIISRemoteManagement.Integration.Tests.ps1
88: $webMgmtService | Should Be 'False'

No idea what's going on there. Maybe an issue with the new version of Pester that was released yesterday?

[nzspambot]

@kwirkykat ok let me fix the NL this weekend

Re the Pester failure, yeah you're very right there; that is rather odd 😕 I'll kick off a new build and see what happens I guess

[nzspambot]

@kwirkykat good guess; yes Pester 4.0.2 causes it to fail 😕 unsure as to why at this stage

[nzspambot]

@kwirkykat fixed but 14 other tests now fail with the same issue

This is clearly a pester issue so I'm unsure how we/you/I should handle this at this stage.

[kwirkykat]

@nzspambot The changes from string --> boolean are correct though. It's an issue with how those tests were originally written. Pester 4 is just more accurate with types apparently than Pester 3.

Could prolly fix the tests pretty easily by just replacing 'False'/'false' with $false and 'True'/'true' with $true in Notepad++ or an IDE. Do you want to do it here or would you like me to open another PR to fix the other tests?

[nzspambot]

@kwirkykat seems to be a bit more than a $true/$false issue

I've opened a issue in the Pester repo ( pester/Pester#702 )

eg:

        Expected: {Handler with name 'StaticFile' already exist}
        But was:  {Handler with name 'StaticFile' already exist}
        at line: 81 in C:\projects\xwebadministration\Tests\Unit\MSFT_xIISHandler.tests.ps1
        81:                     $result[0] | Should Be ($LocalizedData.HandlerExists -f $Name)
      [+] Should return False 8ms
    Context Handler is Present and Ensure = Absent
      [+] Should return False 103ms
      [+] Should not return a verbose message 14ms
    Context Handler is Present and Ensure = Present
      [-] Should return the correct verbose message 108ms
        Expected: {Handler with name 'StaticFile' is not present as requested}
        But was:  {Handler with name 'StaticFile' is not present as requested}
        at line: 110 in C:\projects\xwebadministration\Tests\Unit\MSFT_xIISHandler.tests.ps1
        110:                     $result[0] | Should Be ($LocalizedData.HandlerNotPresent -f $Name)
      [+] Should return False 7ms
  Describing MSFT_xIISHandler\Set-TargetResource
    Context Ensure = Present and Handler is NOT present
      [+] Should add the handler 167ms
      [-] Should call the right Verbose Message 11ms
        Expected: {Adding handler 'StaticFile'}
        But was:  {Adding handler 'StaticFile'}
        at line: 136 in C:\projects\xwebadministration\Tests\Unit\MSFT_xIISHandler.tests.ps1
        136:                     $message | Should Be ($LocalizedData.AddingHandler -f $mockName)
    Context Ensure = Absent and Handler IS present
      [+] Should add the handler 137ms
      [-] Should call the right Verbose Message 12ms
        Expected: {Removing handler 'StaticFile'}
        But was:  {Removing handler 'StaticFile'}
        at line: 153 in C:\projects\xwebadministration\Tests\Unit\MSFT_xIISHandler.tests.ps1
        153:                     $message | Should Be ($LocalizedData.RemovingHandler -f $mockName)```

[kwirkykat]

@nzspambot Ah. Didn't see those. Yeah all we can do is open an issue for Pester then and see what they say 😕

[nzspambot]

@kwirkykat the pester issues have been fixed by @mbreakey3 upstream so this now passes

[tysonjhayes]

Reviewed 5 of 7 files at r1, 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

DSCResources/MSFT_xIisRemoteManagement/MSFT_xIisRemoteManagement.psm1, line 50 at r4 (raw file):

    if ((Get-WindowsFeature -Name Web-Server).Installed -ne $true)
    { 
        $errorMessage = $LocalizedData.ErrorWebServerStateFailure -f $_.Exception.Message

Why aren't we just passing this into the parameter instead of assigning to a variable?

---

DSCResources/MSFT_xIisRemoteManagement/MSFT_xIisRemoteManagement.psm1, line 85 at r4 (raw file):

    }

    Write-Verbose -Message ($LocalizedData.VerboseGetTargetResult)

You should put this above the return line.

---

DSCResources/MSFT_xIisRemoteManagement/MSFT_xIisRemoteManagement.psm1, line 126 at r4 (raw file):

            Write-Verbose -Message ($LocalizedData.VerboseSetTargetInstallingRemoteManagement)
            Import-Module -Name $module
            Install-WindowsFeature -Name $windowsFeature

This function doesn't work (the function exists but it errors.... How obnoxious.) on clients, and I believe is called IIS-ManagementService. Currently our current code base supports both client and server and I'm not sure I'm willing to break that at this point.

If we wrote a helper function to detect the OS (server or client) we could easily switch which install/uninstall we use to compensate for this.

---

DSCResources/MSFT_xIisRemoteManagement/MSFT_xIisRemoteManagement.psm1, line 136 at r4 (raw file):

    }

    if ($getState.State -ne $State)

I'd flip these to disable the service before uninstalling it. In theory you could run into an instance where you've uninstalled the feature/service before you've attempted to stop it resulting in an error.

---

DSCResources/MSFT_xIisRemoteManagement/MSFT_xIisRemoteManagement.psm1, line 193 at r4 (raw file):

    {
        Write-Verbose -Message ($LocalizedData.VerboseTestTargetState)
        Return $false

return should be lowercase.

---

DSCResources/MSFT_xIisRemoteManagement/MSFT_xIisRemoteManagement.schema.mof, line 5 at r4 (raw file):

class MSFT_xIisRemoteManagement : OMI_BaseResource
{
    [Key, ValueMap{"Started","Stopped"}, Values{"Started","Stopped"}] String State;

Could we add descriptions here please? In the future I'd like to use the autodoc which pulls descriptions from here.

---

Examples/Sample_xIISRemoteManagement.ps1, line 1 at r4 (raw file):

{

not sure why we have curly braces here, could we remove these?

Also could you add a synopsis at the top explaining what this is showing (it's obvious I know but if we pull in the auto document writer it uses that info).

---

Tests/Integration/MSFT_xIISRemoteManagement.Integration.Tests.ps1, line 51 at r4 (raw file):

            Invoke-Expression -Command "$($script:DSCResourceName)_Enabled -OutputPath `$TestDrive"
            Start-DscConfiguration -Path $TestDrive -ComputerName localhost -Wait -Verbose -Force
            

nit: Could you remove all of the blank spaces from this file please?

---

Tests/Unit/MSFT_xIISRemoteManagement.tests.ps1, line 1 at r4 (raw file):

$global:DSCModuleName = 'xWebAdministration'

We shouldn't be using global variables. Typically I just redeclare the script variable in the inmodulescope brace to get the writing to work out.

---

Tests/Unit/MSFT_xIISRemoteManagement.tests.ps1, line 61 at r4 (raw file):

            }
        }
        

Could you also remove all of the blanks from this file too?

---

Tests/Unit/MSFT_xIISRemoteManagement.tests.ps1, line 85 at r4 (raw file):

               
                It -name 'should call Get-WindowsFeature twice' -test {
                    Assert-MockCalled -CommandName Get-WindowsFeature -Exactly -Times 2

Why should it be calling this twice? Your code shows it calling it once. One time for get-windowsfeature and one time for get-service.

---

Tests/Unit/MSFT_xIISRemoteManagement.tests.ps1, line 129 at r4 (raw file):

            }
            
            Context -Name 'All Settings are incorrect' -Fixture {

I'm not sure this test is really needed, it's really hitting the same code path as the next test which is checking the state.

---

Comments from Reviewable

[johlju]

Reopen this PR as it is part of an open issue. I will label this as abandoned for the time being so someone else can continue to work on this.