xSQLServerRSConfig: Virtual directories and URL reservations

[bozho]

Pull Request (PR) description
Added support for customising SSRS virtual server names.

This Pull Request (PR) fixes the following issues:
Fixes #570

Task list:

• Change details added to Unreleased section of CHANGELOG.md?
• Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
• Examples appropriately updated?
• New/changed code adheres to Style Guidelines?
• Unit and (optional) Integration tests created/updated where possible?

---

This change is 

[codecov-io]

Codecov Report

Merging #638 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##            dev   #638    +/-   ##
====================================
+ Coverage    96%    96%   +<1%     
====================================
  Files        31     31            
  Lines      3314   3410    +96     
====================================
+ Hits       3197   3293    +96     
  Misses      117    117

[johlju]

@bozho Could you be so kind to add at an example, or several examples if it is necessary, to show how the resource should be used?

Do you want to give it a shot to add code coverage in the unit test for this change? 😄

[johlju]

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 57 unresolved discussions, some commit checks failed.

---

README.md, line 755 at r2 (raw file):

* **[String] RSSQLServer** _(Required)_: Name of the SQL Server to host the Reporting Service database.
* **[String] RSSQLInstanceName** _(Required)_: Name of the SQL Server instance to host the Reporting Service database.
* **[String] ReportServerVirtualDir** _(Write)_: Report Server Web Service virtual directory. Optional.

Please align this description with the schema.mof. So they have the same description.

---

README.md, line 756 at r2 (raw file):

* **[String] RSSQLInstanceName** _(Required)_: Name of the SQL Server instance to host the Reporting Service database.
* **[String] ReportServerVirtualDir** _(Write)_: Report Server Web Service virtual directory. Optional.
* **[String] ReportsVirtualDir** _(Write)_: Report Manager virtual directory. Optional.

Please align this description with the schema.mof. So they have the same description.

---

README.md, line 757 at r2 (raw file):

* **[String] ReportServerVirtualDir** _(Write)_: Report Server Web Service virtual directory. Optional.
* **[String] ReportsVirtualDir** _(Write)_: Report Manager virtual directory. Optional.
* **[String[]] ReportServerReservedUrl** _(Write)_: Report Server URL reservations. Optional. If not specified, "http://+:80" URL reservation will be used.

Please align this description with the schema.mof. So they have the same description.

---

README.md, line 758 at r2 (raw file):

* **[String] ReportsVirtualDir** _(Write)_: Report Manager virtual directory. Optional.
* **[String[]] ReportServerReservedUrl** _(Write)_: Report Server URL reservations. Optional. If not specified, "http://+:80" URL reservation will be used.
* **[String[]] ReportsReservedUrl** _(Write)_: Report Manager URL reservations. Optional. If not specified, "http://+:80" URL reservation will be used.

Please align this description with the schema.mof. So they have the same description.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 38 at r2 (raw file):

        [parameter()]
        [System.String]
        $ReportServerVirtualDir,

This parameter is not used in the Get-TargetResource, so it can be removed. Only required to have all non-mandatory parameters in the Get-TargetResource function.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 42 at r2 (raw file):

        [parameter()]
        [System.String]
        $ReportsVirtualDir,

This parameter is not used in the Get-TargetResource, so it can be removed. Only required to have all non-mandatory parameters in the Get-TargetResource function.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 46 at r2 (raw file):

        [parameter()]
        [System.String[]]
        $ReportServerReservedUrl,

This parameter is not used in the Get-TargetResource, so it can be removed. Only required to have all non-mandatory parameters in the Get-TargetResource function.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 50 at r2 (raw file):

        [parameter()]
        [System.String[]]
        $ReportsReservedUrl

This parameter is not used in the Get-TargetResource, so it can be removed. Only required to have all non-mandatory parameters in the Get-TargetResource function.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 89 at r2 (raw file):

$ReportServerVirtualDir

When parameter is removed this should be changed to $reportServerVirtualDirectory. And all other occurrences.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 90 at r2 (raw file):

$ReportsVirtualDir

When parameter is removed this should be changed to $reportsVirtualDirectory. And all other occurrences.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 94 at r2 (raw file):

$ReportServerReservedUrl

When parameter is removed this should be changed to $reportServerReservedUrl. And all other occurrences.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 95 at r2 (raw file):

$ReportsReservedUrl

When parameter is removed this should be changed to $reportsReservedUrl. And all other occurrences.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 97 at r2 (raw file):

for ( $i = 0; $i -lt $reservedUrls.Application.Count; ++$i )

Doesn't foreach ($reservedUrl in $reservedUrls) work here?
If not, this is okay as is.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 162 at r2 (raw file):

        [parameter()]
        [System.String]
        $ReportServerVirtualDir,

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 166 at r2 (raw file):

        [parameter()]
        [System.String]
        $ReportsVirtualDir,

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 170 at r2 (raw file):

        [parameter()]
        [System.String[]]
        $ReportServerReservedUrl,

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 174 at r2 (raw file):

        [parameter()]
        [System.String[]]
        $ReportsReservedUrl

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 177 at r2 (raw file):

    )

    $instanceNamesRegistryKey = 'HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\Instance Names\RS'

Question. This code seems similar to the code in Get-TargetResource. Isn't it possible to call Get-TargetResource and remove some of this code?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 186 at r2 (raw file):

        if ( $InstanceName -eq 'MSSQLSERVER' )
        {
            $reportingServicesServiceName = "ReportServer"

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 187 at r2 (raw file):

        {
            $reportingServicesServiceName = "ReportServer"
            if([string]::IsNullOrEmpty($ReportServerVirtualDir)) { $ReportServerVirtualDir = "ReportServer" }

Please use if ( space after if-statement.
Please use single quotes.
And please move braces on separate lines

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 187 at r2 (raw file):

if([string]::IsNullOrEmpty($ReportServerVirtualDir))

Should be enough with if (-not $ReportServerVirtualDir)?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 188 at r2 (raw file):

            $reportingServicesServiceName = "ReportServer"
            if([string]::IsNullOrEmpty($ReportServerVirtualDir)) { $ReportServerVirtualDir = "ReportServer" }
            if([string]::IsNullOrEmpty($ReportsVirtualDir)) { $ReportsVirtualDir = "Reports" }

Please use if ( space after if-statement.
Please use single quotes.
And please move braces on separate lines

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 188 at r2 (raw file):

if([string]::IsNullOrEmpty($ReportsVirtualDir))

Should be enough with if (-not $ReportsVirtualDir)?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 189 at r2 (raw file):

            if([string]::IsNullOrEmpty($ReportServerVirtualDir)) { $ReportServerVirtualDir = "ReportServer" }
            if([string]::IsNullOrEmpty($ReportsVirtualDir)) { $ReportsVirtualDir = "Reports" }
            $reportingServicesDatabaseName = "ReportServer"

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 194 at r2 (raw file):

        {
            $reportingServicesServiceName = "ReportServer`$$InstanceName"
            if([string]::IsNullOrEmpty($ReportServerVirtualDir)) { $ReportServerVirtualDir = "ReportServer_$InstanceName" }

Please use if ( space after if-statement.
Please use single quotes.
And please move braces on separate lines

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 194 at r2 (raw file):

if([string]::IsNullOrEmpty($ReportServerVirtualDir))

Should be enough with if (-not $ReportServerVirtualDir)?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 195 at r2 (raw file):

            $reportingServicesServiceName = "ReportServer`$$InstanceName"
            if([string]::IsNullOrEmpty($ReportServerVirtualDir)) { $ReportServerVirtualDir = "ReportServer_$InstanceName" }
            if([string]::IsNullOrEmpty($ReportsVirtualDir)) { $ReportsVirtualDir = "Reports_$InstanceName" }

Please use if ( space after if-statement.
Please use single quotes.
And please move braces on separate lines

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 195 at r2 (raw file):

if([string]::IsNullOrEmpty($ReportsVirtualDir))

Should be enough with if (-not $ReportsVirtualDir)?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 224 at r2 (raw file):

        }

        if(!$reportingServicesConfiguration.IsInitialized)

Please use if ( space after if-statement.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 228 at r2 (raw file):

            New-VerboseMessage -Message "Initializing Reporting Services on $RSSQLServer\$RSSQLInstanceName."

            if ( $ReportServerReservedUrl -eq $null )

Should be enough with if (-not $ReportServerReservedUrl)?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 230 at r2 (raw file):

            if ( $ReportServerReservedUrl -eq $null )
            {
                $ReportServerReservedUrl = @("http://+:80")

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 233 at r2 (raw file):

            }
            
            if ( $ReportsReservedUrl -eq $null )

Should be enough with if (-not $ReportsReservedUrl)?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 235 at r2 (raw file):

            if ( $ReportsReservedUrl -eq $null )
            {
                $ReportsReservedUrl = @("http://+:80")

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 241 at r2 (raw file):

            {
                New-VerboseMessage -Message "Setting report server virtual directory on $RSSQLServer\$RSSQLInstanceName to $ReportServerVirtualDir."
                $null = $reportingServicesConfiguration.SetVirtualDirectory("ReportServerWebService",$ReportServerVirtualDir,$language)

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 244 at r2 (raw file):

                $ReportServerReservedUrl | ForEach-Object {
                    New-VerboseMessage -Message "Adding report server URL reservation on $RSSQLServer\$RSSQLInstanceName`: $_."
                    $null = $reportingServicesConfiguration.ReserveURL("ReportServerWebService",$_,$language)

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 257 at r2 (raw file):

                }
            }
            $reportingServicesDatabaseScript = $reportingServicesConfiguration.GenerateDatabaseCreationScript($reportingServicesDatabaseName,$language,$false)

Please add a blank line before this one.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 272 at r2 (raw file):

            Invoke-Sqlcmd -ServerInstance $reportingServicesConnnection -Query $reportingServicesDatabaseRightsScript.Script

            $null = $reportingServicesConfiguration.SetDatabaseConnection($reportingServicesConnnection,$reportingServicesDatabaseName,2,"","")

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 287 at r2 (raw file):

                    change the virtual directory and re-add URL reservations
                #>
                $currentConfig.ReportServerReservedUrl | ForEach-Object { $null = $reportingServicesConfiguration.RemoveURL("ReportServerWebService",$_,$language) }

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 288 at r2 (raw file):

                #>
                $currentConfig.ReportServerReservedUrl | ForEach-Object { $null = $reportingServicesConfiguration.RemoveURL("ReportServerWebService",$_,$language) }
                $reportingServicesConfiguration.SetVirtualDirectory("ReportServerWebService",$ReportServerVirtualDir,$language)

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 289 at r2 (raw file):

                $currentConfig.ReportServerReservedUrl | ForEach-Object { $null = $reportingServicesConfiguration.RemoveURL("ReportServerWebService",$_,$language) }
                $reportingServicesConfiguration.SetVirtualDirectory("ReportServerWebService",$ReportServerVirtualDir,$language)
                $currentConfig.ReportServerReservedUrl | ForEach-Object { $null = $reportingServicesConfiguration.ReserveURL("ReportServerWebService",$_,$language) }

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 300 at r2 (raw file):

                    change the virtual directory and re-add URL reservations
                #>
                $currentConfig.ReportsReservedUrl | ForEach-Object { $null = $reportingServicesConfiguration.RemoveURL($reportsApplicationName,$_,$language) }

Won't this remove the URL added above?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 305 at r2 (raw file):

            }

            if ( ($ReportServerReservedUrl -ne $null) -and ((Compare-Object -ReferenceObject $currentConfig.ReportServerReservedUrl -DifferenceObject $ReportServerReservedUrl) -ne $null) )

Could you please move the Compare-Object bore the if and assign it to a variable and use the variable in the evaluation instead? To shorten the row.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 308 at r2 (raw file):

            {
                $currentConfig.ReportServerReservedUrl | ForEach-Object {
                    $null = $reportingServicesConfiguration.RemoveURL("ReportServerWebService",$_,$language)

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 313 at r2 (raw file):

                $ReportServerReservedUrl | ForEach-Object {
                    New-VerboseMessage -Message "Adding report server URL reservation on $RSSQLServer\$RSSQLInstanceName`: $_."
                    $null = $reportingServicesConfiguration.ReserveURL("ReportServerWebService",$_,$language)

Please use single quotes.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 317 at r2 (raw file):

            }

            if ( ($ReportsReservedUrl -ne $null) -and ((Compare-Object -ReferenceObject $currentConfig.ReportsReservedUrl -DifferenceObject $ReportsReservedUrl) -ne $null) )

Could you please move the Compare-Object bore the if and assign it to a variable and use the variable in the evaluation instead? To shorten the row.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 370 at r2 (raw file):

        [parameter()]
        [System.String]
        $ReportServerVirtualDir,

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 374 at r2 (raw file):

        [parameter()]
        [System.String]
        $ReportsVirtualDir,

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 378 at r2 (raw file):

        [parameter()]
        [System.String[]]
        $ReportServerReservedUrl,

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 382 at r2 (raw file):

        [parameter()]
        [System.String[]]
        $ReportsReservedUrl

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 389 at r2 (raw file):

!

Please use -not instead

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 395 at r2 (raw file):

if ( ![string]::IsNullOrEmpty($ReportServerVirtualDir) -and ...

Shouldn't it be enough with if ( $ReportServerVirtualDir -and ...?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 401 at r2 (raw file):

if ( ![string]::IsNullOrEmpty($ReportsVirtualDir) -and ...

Shouldn't it be enough with if ( $ReportsVirtualDir-and ...?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 407 at r2 (raw file):

$ReportServerReservedUrl -ne $null
Please use $null first in the evaluation $null -ne $ReportServerReservedUrl.

Please fix all of these
https://ci.appveyor.com/project/PowerShell/xsqlserver/build/5.0.1292.0#L532

MSFT_xSQLServerRSConfig.psm1 (Line 228): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 233): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 305): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 305): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 317): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 317): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 407): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 407): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 413): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 413): $null should be on the left side of equality comparisons.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 407 at r2 (raw file):

    }

    if ( ($ReportServerReservedUrl -ne $null) -and ((Compare-Object -ReferenceObject $currentConfig.ReportServerReservedUrl -DifferenceObject $ReportServerReservedUrl) -ne $null) )

Could you please move the Compare-Object bore the if and assign it to a variable and use the variable in the evaluation instead? To shorten the row.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 413 at r2 (raw file):

    }

    if ( ($ReportsReservedUrl -ne $null) -and ((Compare-Object -ReferenceObject $currentConfig.ReportsReservedUrl -DifferenceObject $ReportsReservedUrl) -ne $null) )

Could you please move the Compare-Object bore the if and assign it to a variable and use the variable in the evaluation instead? To shorten the row.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.schema.mof, line 7 at r2 (raw file):

ReportServerVirtualDir

Could we change to 'ReportServerVirtualDirectory'?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.schema.mof, line 8 at r2 (raw file):

ReportsVirtualDir

Could we change to 'ReportsVirtualDirectory'?

---

Comments from Reviewable

[bozho]

Review status: all files reviewed at latest revision, 57 unresolved discussions, some commit checks failed.

---

README.md, line 755 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please align this description with the schema.mof. So they have the same description.

Done.

---

README.md, line 756 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please align this description with the schema.mof. So they have the same description.

Done.

---

README.md, line 757 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please align this description with the schema.mof. So they have the same description.

Done.

---

README.md, line 758 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please align this description with the schema.mof. So they have the same description.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 38 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This parameter is not used in the Get-TargetResource, so it can be removed. Only required to have all non-mandatory parameters in the Get-TargetResource function.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 42 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This parameter is not used in the Get-TargetResource, so it can be removed. Only required to have all non-mandatory parameters in the Get-TargetResource function.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 46 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This parameter is not used in the Get-TargetResource, so it can be removed. Only required to have all non-mandatory parameters in the Get-TargetResource function.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 50 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This parameter is not used in the Get-TargetResource, so it can be removed. Only required to have all non-mandatory parameters in the Get-TargetResource function.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 89 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$ReportServerVirtualDir

When parameter is removed this should be changed to $reportServerVirtualDirectory. And all other occurrences.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 90 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$ReportsVirtualDir

When parameter is removed this should be changed to $reportsVirtualDirectory. And all other occurrences.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 94 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$ReportServerReservedUrl

When parameter is removed this should be changed to $reportServerReservedUrl. And all other occurrences.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 95 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$ReportsReservedUrl

When parameter is removed this should be changed to $reportsReservedUrl. And all other occurrences.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 97 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

for ( $i = 0; $i -lt $reservedUrls.Application.Count; ++$i )

Doesn't foreach ($reservedUrl in $reservedUrls) work here?
If not, this is okay as is.

No, since $reservedUrls has application names and url strings in two separate array properties (.Application and .UrlString)

I could've been clever and zip the two arrays and then enumerate that, but the properties are only used here and for(...) is simple to understand and follow.

I try to avoid "clever" code when a simple solution will do :-)

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 162 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 166 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 170 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 174 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 177 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Question. This code seems similar to the code in Get-TargetResource. Isn't it possible to call Get-TargetResource and remove some of this code?

Yes, looking at it, we could encapsulate getting $reportingServicesConfiguration into a helper method (e.g. Get-RSConfiguration, or something similar)

It might be useful for the method to return both the configuration and the $reportsApplicationName value (which is based off the SQL Server version)

What do you think?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 187 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if([string]::IsNullOrEmpty($ReportServerVirtualDir))

Should be enough with if (-not $ReportServerVirtualDir)?

Eh, C# habits... I wasn't even aware that -not returns $false for both empty strings and $null strings in PS :-)

I find it a bit annoying that PowerShell sometimes implicitly converts $null strings to empty ones. I'm aware that in shell scripting it's usually safe to treat them the same, but since we interface with .NET, I'd prefer if we treated the two values differently, and make it explicit when we'll accept either (like in this case)

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 189 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 224 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use if ( space after if-statement.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 228 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should be enough with if (-not $ReportServerReservedUrl)?

I prefer using explicit comparison against $null (C# habits), but if you prefer using -not, I'm ok with that.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 230 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 233 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should be enough with if (-not $ReportsReservedUrl)?

I prefer using explicit comparison against $null (C# habits), but if you prefer using -not, I'm ok with that.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 235 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 241 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 244 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 257 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank line before this one.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 272 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 287 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 288 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 289 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 300 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Won't this remove the URL added above?

You mean the above if() block, at 279? If so, then no, those are $currentConfig.ReportServerReservedUrl URLs

We are handling $currentConfig.ReportsReservedUrl URLs here.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 305 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please move the Compare-Object bore the if and assign it to a variable and use the variable in the evaluation instead? To shorten the row.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 308 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 313 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 317 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please move the Compare-Object bore the if and assign it to a variable and use the variable in the evaluation instead? To shorten the row.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 370 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 374 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 378 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 382 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add this to the comment-based help. Please use at least the same description as in the README.md (more text is okay too).

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 389 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

!

Please use -not instead

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 407 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$ReportServerReservedUrl -ne $null
Please use $null first in the evaluation $null -ne $ReportServerReservedUrl.

Please fix all of these
https://ci.appveyor.com/project/PowerShell/xsqlserver/build/5.0.1292.0#L532

MSFT_xSQLServerRSConfig.psm1 (Line 228): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 233): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 305): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 305): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 317): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 317): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 407): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 407): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 413): $null should be on the left side of equality comparisons.
MSFT_xSQLServerRSConfig.psm1 (Line 413): $null should be on the left side of equality comparisons.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 407 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please move the Compare-Object bore the if and assign it to a variable and use the variable in the evaluation instead? To shorten the row.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 413 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please move the Compare-Object bore the if and assign it to a variable and use the variable in the evaluation instead? To shorten the row.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.schema.mof, line 7 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

ReportServerVirtualDir

Could we change to 'ReportServerVirtualDirectory'?

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.schema.mof, line 8 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

ReportsVirtualDir

Could we change to 'ReportsVirtualDirectory'?

Done.

---

Comments from Reviewable

[bozho]

Review status: all files reviewed at latest revision, 57 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 186 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use single quotes.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 187 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use if ( space after if-statement.
Please use single quotes.
And please move braces on separate lines

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 188 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use if ( space after if-statement.
Please use single quotes.
And please move braces on separate lines

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 194 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use if ( space after if-statement.
Please use single quotes.
And please move braces on separate lines

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 195 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use if ( space after if-statement.
Please use single quotes.
And please move braces on separate lines

Done.

---

Comments from Reviewable

[bozho]

@johlju I've addressed most of the CR comments (sorry for giving you a lot of work - the style is quite different from what I use, it takes a while to adjust :-)

I'll add some examples.

I've never done code coverage tests, can you point me to some tutorials, please?

[johlju]

Reviewed 1 of 2 files at r3, 1 of 3 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 177 at r2 (raw file):

Previously, bozho (Marko Bozikovic) wrote…

Yes, looking at it, we could encapsulate getting $reportingServicesConfiguration into a helper method (e.g. Get-RSConfiguration, or something similar)

It might be useful for the method to return both the configuration and the $reportsApplicationName value (which is based off the SQL Server version)

What do you think?

If that would remove duplicate code I'm all for it. If Get-TargetResource is gonna return more properties in the hash table, then remember to add them as Read properties in the schema.mof.
Do you thin it's doable without to much work?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 188 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if([string]::IsNullOrEmpty($ReportsVirtualDir))

Should be enough with if (-not $ReportsVirtualDir)?

Missed writing Done on this one, so can't Acknowledge. I guess you wanted to answer the same thing as the previous comment. :)

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 194 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if([string]::IsNullOrEmpty($ReportServerVirtualDir))

Should be enough with if (-not $ReportServerVirtualDir)?

Same as comment above

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 195 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if([string]::IsNullOrEmpty($ReportsVirtualDir))

Should be enough with if (-not $ReportsVirtualDir)?

Same as comment above

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 300 at r2 (raw file):

Previously, bozho (Marko Bozikovic) wrote…

You mean the above if() block, at 279? If so, then no, those are $currentConfig.ReportServerReservedUrl URLs

We are handling $currentConfig.ReportsReservedUrl URLs here.

My bad. Read this one wrong. All good.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 395 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if ( ![string]::IsNullOrEmpty($ReportServerVirtualDir) -and ...

Shouldn't it be enough with if ( $ReportServerVirtualDir -and ...?

Same comment as above

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 401 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if ( ![string]::IsNullOrEmpty($ReportsVirtualDir) -and ...

Shouldn't it be enough with if ( $ReportsVirtualDir-and ...?

Same comment as above

---

Comments from Reviewable

[johlju]

Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

README.md, line 757 at r5 (raw file):

"http://+:80"

Use single quotes here as well.

---

README.md, line 758 at r5 (raw file):

"http://+:80"

Use single quotes here as well.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 134 at r5 (raw file):

"http://+:80"

Use single quotes here as well.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 137 at r5 (raw file):

"http://+:80"

Use single quotes here as well.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 372 at r5 (raw file):

"http://+:80"

Use single quotes here as well.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 375 at r5 (raw file):

"http://+:80"

Use single quotes here as well.

---

Comments from Reviewable

[bozho]

Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

README.md, line 757 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"http://+:80"

Use single quotes here as well.

Done.

---

README.md, line 758 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"http://+:80"

Use single quotes here as well.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 177 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If that would remove duplicate code I'm all for it. If Get-TargetResource is gonna return more properties in the hash table, then remember to add them as Read properties in the schema.mof.
Do you thin it's doable without to much work?

Shouldn't be too hard.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 188 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missed writing Done on this one, so can't Acknowledge. I guess you wanted to answer the same thing as the previous comment. :)

Yes :)

Which style would you prefer here?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 194 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same as comment above

Yup

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 195 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same as comment above

Yup

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 395 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same comment as above

Yup. Not really sure what I need to write here during the review :)

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 401 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same comment as above

Yup

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 134 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"http://+:80"

Use single quotes here as well.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 137 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"http://+:80"

Use single quotes here as well.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 372 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"http://+:80"

Use single quotes here as well.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 375 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"http://+:80"

Use single quotes here as well.

Done.

---

Comments from Reviewable

[johlju]

@bozho No worries about the style, I know it can take a while to adjust 😄

I did not see any examples? Did you miss add them to a commit?

In regards to Pester tests, you only need to change and add to the existing tests here; https://github.com/PowerShell/xSQLServer/blob/dev/Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1

I think the best resource for learning all aspects of Pester is this book The Pester Book by Adam Bertram. And you can always reference the tests in this repo, since those cover many variants of tests. I learned by looking at others test code, the book did not exist then 😉. Also Mariah and Katie (in the PowerShell team) was kind to point out mistakes and how I could improve the tests.
You can do this by trial and error if you like. 😄 Give it a shot and I'm here to help and give pointers if you get stuck. 😄

[johlju]

Reviewed 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 177 at r2 (raw file):

Previously, bozho (Marko Bozikovic) wrote…

Shouldn't be too hard.

Waiting for this commit then 😄

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 188 at r2 (raw file):

Previously, bozho (Marko Bozikovic) wrote…

Yes :)

Which style would you prefer here?

You did right not answering on these, since you waited for an answer from me on the first review comment about this. It was my bad not answering. I just accepted that you want to use IsNullOrEmpty on the first comment, and never thought that you waited for an answer. :)

Agin my bad. You did correctly.

Leave this as is. That's all good! 😄

Now you need to write Done though 😉

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 395 at r2 (raw file):

Previously, bozho (Marko Bozikovic) wrote…

Yup. Not really sure what I need to write here during the review :)

You did right not answering on these, since you waited for an answer from me on the first review comment about this. It was my bad not answering. I just accepted that you want to use IsNullOrEmpty on the first comment, and never thought that you waited for an answer. :)

Agin my bad. You did correctly.

Leave this as is. That's all good! 😄

Now you need to write Done though 😉

---

Comments from Reviewable

[bozho]

@johlju I didn't add any examples yet, I thought I'd do that once we finish reviewing the code.

I've added that helper method, I just need to quickly rebuild my test VM to check I haven't done and messed something up. I'll update the Reviewable comments when I've done that.

[bozho]

Review status: 3 of 4 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 177 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Waiting for this commit then 😄

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 188 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You did right not answering on these, since you waited for an answer from me on the first review comment about this. It was my bad not answering. I just accepted that you want to use IsNullOrEmpty on the first comment, and never thought that you waited for an answer. :)

Agin my bad. You did correctly.

Leave this as is. That's all good! 😄

Now you need to write Done though 😉

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 395 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

You did right not answering on these, since you waited for an answer from me on the first review comment about this. It was my bad not answering. I just accepted that you want to use IsNullOrEmpty on the first comment, and never thought that you waited for an answer. :)

Agin my bad. You did correctly.

Leave this as is. That's all good! 😄

Now you need to write Done though 😉

Done.

---

Comments from Reviewable

[johlju]

You got the code much cleaner! Great work. A few more comments.

---

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 83 at r7 (raw file):

    }

    $returnValue = @{

Change this to and th next row can be removed.

return @{
   ...
}

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 200 at r7 (raw file):

        }

        $language = (Get-WMIObject -Class Win32_OperatingSystem -Namespace root/cimv2 -ErrorAction SilentlyContinue).OSLanguage

A potential problem with 'SilentlyContinue'? if this fails, $language will contain null which will break the function call using this? Add a check here if it is null, and throw an error?

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 239 at r7 (raw file):

            # Determine RS service account
            $reportingServicesServiceAccountUserName = (Get-WmiObject -Class Win32_Service | Where-Object {$_.Name -eq $reportingServicesServiceName}).StartName

Add -FilterScript to Where-Object.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 258 at r7 (raw file):

            $currentConfig = Get-TargetResource @PSBoundParameters

            if ( ![string]::IsNullOrEmpty($ReportServerVirtualDirectory) -and ($ReportServerVirtualDirectory -ne $currentConfig.ReportServerVirtualDirectory) )

Please change ! to -not.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 266 at r7 (raw file):

                    change the virtual directory and re-add URL reservations
                #>
                $currentConfig.ReportServerReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.RemoveURL('ReportServerWebService',$_,$language) }

Please move the code in the script block to a separate row.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 268 at r7 (raw file):

                $currentConfig.ReportServerReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.RemoveURL('ReportServerWebService',$_,$language) }
                $reportingServicesData.Configuration.SetVirtualDirectory('ReportServerWebService',$ReportServerVirtualDirectory,$language)
                $currentConfig.ReportServerReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.ReserveURL('ReportServerWebService',$_,$language) }

Please move the code in the script block to a separate row.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 271 at r7 (raw file):

            }

            if ( ![string]::IsNullOrEmpty($ReportsVirtualDirectory) -and ($ReportsVirtualDirectory -ne $currentConfig.ReportsVirtualDirectory) )

Please change ! to -not.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 279 at r7 (raw file):

                    change the virtual directory and re-add URL reservations
                #>
                $currentConfig.ReportsReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.RemoveURL($reportingServicesData.ReportsApplicationName,$_,$language) }

Please move the code in the script block to a separate row.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 281 at r7 (raw file):

                $currentConfig.ReportsReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.RemoveURL($reportingServicesData.ReportsApplicationName,$_,$language) }
                $reportingServicesData.Configuration.SetVirtualDirectory($reportingServicesData.ReportsApplicationName,$ReportsVirtualDirectory,$language)
                $currentConfig.ReportsReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.ReserveURL($reportingServicesData.ReportsApplicationName,$_,$language) }

Please move the code in the script block to a separate row.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 312 at r7 (raw file):

    }

    if ( !(Test-TargetResource @PSBoundParameters) )

Please change ! to -not.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 388 at r7 (raw file):

    }

    if ( ![string]::IsNullOrEmpty($ReportServerVirtualDirectory) -and ($ReportServerVirtualDirectory -ne $currentConfig.ReportServerVirtualDirectory) )

Please change ! to -not.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 394 at r7 (raw file):

    }

    if ( ![string]::IsNullOrEmpty($ReportsVirtualDirectory) -and ($ReportsVirtualDirectory -ne $currentConfig.ReportsVirtualDirectory) )

Please change ! to -not.

---

Comments from Reviewable

[johlju]

Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 220 at r7 (raw file):

                New-VerboseMessage -Message "Setting report server virtual directory on $RSSQLServer\$RSSQLInstanceName to $ReportServerVirtualDirectory."
                $null = $reportingServicesData.Configuration.SetVirtualDirectory('ReportServerWebService',$ReportServerVirtualDirectory,$language)
                $ReportServerReservedUrl | ForEach-Object {

Please add -Process to the ForEach-Object cmdlet.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 230 at r7 (raw file):

                New-VerboseMessage -Message "Setting reports virtual directory on $RSSQLServer\$RSSQLInstanceName to $ReportServerVirtualDirectory."
                $null = $reportingServicesData.Configuration.SetVirtualDirectory($reportingServicesData.ReportsApplicationName,$ReportsVirtualDirectory,$language)
                $ReportsReservedUrl | ForEach-Object {

Please add -Process to the ForEach-Object cmdlet.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 266 at r7 (raw file):

                    change the virtual directory and re-add URL reservations
                #>
                $currentConfig.ReportServerReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.RemoveURL('ReportServerWebService',$_,$language) }

Please add -Process to the ForEach-Object cmdlet.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 268 at r7 (raw file):

                $currentConfig.ReportServerReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.RemoveURL('ReportServerWebService',$_,$language) }
                $reportingServicesData.Configuration.SetVirtualDirectory('ReportServerWebService',$ReportServerVirtualDirectory,$language)
                $currentConfig.ReportServerReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.ReserveURL('ReportServerWebService',$_,$language) }

Please add -Process to the ForEach-Object cmdlet.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 279 at r7 (raw file):

                    change the virtual directory and re-add URL reservations
                #>
                $currentConfig.ReportsReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.RemoveURL($reportingServicesData.ReportsApplicationName,$_,$language) }

Please add -Process to the ForEach-Object cmdlet.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 281 at r7 (raw file):

                $currentConfig.ReportsReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.RemoveURL($reportingServicesData.ReportsApplicationName,$_,$language) }
                $reportingServicesData.Configuration.SetVirtualDirectory($reportingServicesData.ReportsApplicationName,$ReportsVirtualDirectory,$language)
                $currentConfig.ReportsReservedUrl | ForEach-Object { $null = $reportingServicesData.Configuration.ReserveURL($reportingServicesData.ReportsApplicationName,$_,$language) }

Please add -Process to the ForEach-Object cmdlet.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 287 at r7 (raw file):

            if ( ($null -ne $ReportServerReservedUrl) -and ($null -ne $reportServerReservedUrlDifference) )
            {
                $currentConfig.ReportServerReservedUrl | ForEach-Object {

Please add -Process to the ForEach-Object cmdlet.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 291 at r7 (raw file):

                }

                $ReportServerReservedUrl | ForEach-Object {

Please add -Process to the ForEach-Object cmdlet.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 300 at r7 (raw file):

            if ( ($null -ne $ReportsReservedUrl) -and ($null -ne $reportsReservedUrlDifference) )
            {
                $currentConfig.ReportsReservedUrl | ForEach-Object {

Please add -Process to the ForEach-Object cmdlet.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 304 at r7 (raw file):

                }

                $ReportsReservedUrl | ForEach-Object {

Please add -Process to the ForEach-Object cmdlet.

---

Comments from Reviewable

[bozho]

Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 220 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add -Process to the ForEach-Object cmdlet.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 230 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add -Process to the ForEach-Object cmdlet.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 239 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add -FilterScript to Where-Object.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 258 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change ! to -not.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 266 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add -Process to the ForEach-Object cmdlet.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 268 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add -Process to the ForEach-Object cmdlet.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 271 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change ! to -not.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 279 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add -Process to the ForEach-Object cmdlet.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 281 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add -Process to the ForEach-Object cmdlet.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 287 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add -Process to the ForEach-Object cmdlet.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 291 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add -Process to the ForEach-Object cmdlet.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 300 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add -Process to the ForEach-Object cmdlet.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 304 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add -Process to the ForEach-Object cmdlet.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 312 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change ! to -not.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 388 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change ! to -not.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 394 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change ! to -not.

Done.

---

Comments from Reviewable

[bozho]

Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 83 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change this to and th next row can be removed.

return @{
   ...
}

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 200 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

A potential problem with 'SilentlyContinue'? if this fails, $language will contain null which will break the function call using this? Add a check here if it is null, and throw an error?

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 266 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please move the code in the script block to a separate row.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 268 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please move the code in the script block to a separate row.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 279 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please move the code in the script block to a separate row.

Done.

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 281 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please move the code in the script block to a separate row.

Done.

---

Comments from Reviewable

[johlju]

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

---

DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 198 at r8 (raw file):

        }

        $wmiOS = Get-WMIObject -Class Win32_OperatingSystem -Namespace root/cimv2 -ErrorAction SilentlyContinue

Please change to $wmiOperatingSystem
(let's keep out abbreviations)

---

Comments from Reviewable

[johlju]

Reviewed 4 of 4 files at r18.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[johlju]

I did a rebase on this one to get all the changes I did in other PR's. Code looks okay. I will look at fixing the unit tests as soon as possible now.
I will also merge the integration test PR later, then this PR can be tests against those tests as well, I maybe even extend the integration tests.

But all good now, so far. As I said, I will look at the unit tests as soon as possible.

[johlju]

---

Reviewed 1 of 1 files at r19, 2 of 2 files at r20.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[johlju]

@bozho Really awesome work on this one! Hope you continue to approve on the Reporting Services resources. 😄

[bozho]

Thank you very much for your help with reviews and tests, too!