Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xSQLServerRSConfig: Virtual directory creation for SQL 2016 #575

Merged
merged 8 commits into from
Jun 15, 2017

Conversation

bozho
Copy link
Contributor

@bozho bozho commented May 24, 2017

Pull Request (PR) description
A fix for web portal application name change introduced in SQL Server 2016.

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

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 Reviewable

@bozho bozho force-pushed the issue-569-sql2016-virtual-dir-name branch from 2587476 to 1d356b1 Compare May 24, 2017 14:23
@codecov-io
Copy link

codecov-io commented May 24, 2017

Codecov Report

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

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #575   +/-   ##
===================================
+ Coverage    93%    94%   +1%     
===================================
  Files        28     28           
  Lines      2880   2883    +3     
===================================
+ Hits       2682   2737   +55     
+ Misses      198    146   -52

@bozho bozho force-pushed the issue-569-sql2016-virtual-dir-name branch 3 times, most recently from 7c00eda to aba4e8a Compare May 24, 2017 16:27
@johlju johlju added help wanted The issue is up for grabs for anyone in the community. needs review The pull request needs a code review. and removed help wanted The issue is up for grabs for anyone in the community. labels May 24, 2017
@bozho bozho force-pushed the issue-569-sql2016-virtual-dir-name branch from aba4e8a to 50ff653 Compare May 25, 2017 16:10
@johlju johlju force-pushed the issue-569-sql2016-virtual-dir-name branch from 81d5b06 to cd1692e Compare May 26, 2017 17:05
@johlju
Copy link
Member

johlju commented May 26, 2017

@bozho Tests added. I rebased this one becuase AppVeyor does not run test when there is merge conflicts.
Please pull changes for this branch to your local branch so you get my changes.

@johlju
Copy link
Member

johlju commented May 27, 2017

I did manually integration test of this code as well and this code works flawless in my lab also, for both SQL Server 2016 and SQL Server 2017, of what I can see.
Awesome work @bozho!! 😄

One thing though, it seems Reporting Services service need a restart to get the 'Reports' site working. 'ReportServer' site works without restart, but not 'Reports'. Have you seen this too? If so I can submit an issue for that so that can be added (optional restart parameter).

@johlju johlju 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 May 27, 2017
@johlju
Copy link
Member

johlju commented May 27, 2017

Waiting for PR #573 to be merged before label this one for review.

@bozho bozho force-pushed the issue-569-sql2016-virtual-dir-name branch from d15422f to 9bfc9fc Compare May 30, 2017 09:49
@bozho
Copy link
Contributor Author

bozho commented May 30, 2017

@johlju Rebased this on top of rebased #573. Didn't forget about your 2 commits :)

@johlju
Copy link
Member

johlju commented May 31, 2017

It seems like reporting services must be restarted for the Reports web site to work, because just installed again and Reports does not start without restarting the service.
I add an issue for that.

@bozho
Copy link
Contributor Author

bozho commented May 31, 2017

Ok, I can add the fix for that on this branch.

@bozho
Copy link
Contributor Author

bozho commented Jun 1, 2017

Hi, are you ok with me rebasing this on top of #569 after rebasing onto current dev?

@bozho
Copy link
Contributor Author

bozho commented Jun 1, 2017

... or should we just merge stuff? I know the history will look ugly until the patch goes into dev, but with rebasing we're risking overwriting each other's work...

@bozho bozho force-pushed the issue-569-sql2016-virtual-dir-name branch from 18da6aa to 5aa939b Compare June 2, 2017 10:18
@bozho
Copy link
Contributor Author

bozho commented Jun 2, 2017

Rebased. Phew!

@bozho bozho force-pushed the issue-569-sql2016-virtual-dir-name branch 2 times, most recently from 1a29064 to 7d6179b Compare June 5, 2017 09:37
@johlju
Copy link
Member

johlju commented Jun 6, 2017

@bozho Merged PR #573. Please rebase this PR against dev again.

@johlju
Copy link
Member

johlju commented Jun 8, 2017

Sorry about all the style comments. This is a resource that nobody has touch yet, so the style guidelines has not been enforced.
Really appreciate if you can fix all these comments. Let me know if you need any help.
I'm so grateful that you are making this resource so much better! 😄


Reviewed 1 of 2 files at r2, 1 of 1 files at r4.
Review status: 2 of 3 files reviewed at latest revision, 54 unresolved discussions.


CHANGELOG.md, line 15 at r4 (raw file):

    this problems for us.

  - Fixed virtual directory creation for SQL Server 2016.

Add "(issue #569)" so there is a reference to the issue number.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 5 at r4 (raw file):

# Load Common Code
Import-Module $currentPath\..\..\xSQLServerHelper.psm1 -Verbose:$false -ErrorAction Stop

Could you please change this row to this, so it is the same as in other resource.

Import-Module -Name (Join-Path -Path (Split-Path (Split-Path $PSScriptRoot -Parent) -Parent) `
                               -ChildPath 'xSQLServerHelper.psm1') `
                               -Force

After fixing this, please remove line 1-4 which is not needed (?).


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 7 at r4 (raw file):

Import-Module $currentPath\..\..\xSQLServerHelper.psm1 -Verbose:$false -ErrorAction Stop

function Get-TargetResource

Could you please add comment-based help to the Get-TargetResource function?

Example:
https://github.com/PowerShell/xSQLServer/blob/dev/DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1#L4-L13


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 13 at r4 (raw file):

    param
    (
        [parameter(Mandatory = $true)]

Change to 'Parameter' - Upper 'P'


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 17 at r4 (raw file):

        $InstanceName,

        [parameter(Mandatory = $true)]

Change to 'Parameter' - Upper 'P'


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 21 at r4 (raw file):

        $RSSQLServer,

        [parameter(Mandatory = $true)]

Change to 'Parameter' - Upper 'P'


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 26 at r4 (raw file):

    )

    if(Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\Instance Names\RS" -Name $InstanceName -ErrorAction SilentlyContinue)

Please add a blank space after each if-statement.

Also change to single quote ' around the string


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 28 at r4 (raw file):

    if(Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\Instance Names\RS" -Name $InstanceName -ErrorAction SilentlyContinue)
    {
        $InstanceKey = (Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\Instance Names\RS" -Name $InstanceName).$InstanceName

This has the same path as the one above, could we add the path to a variable, and use the variable in both places instead?


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 28 at r4 (raw file):

$InstanceKey

Please change to $instanceKey, or rather $instanceId to align with other resources. Lower-case 'i' in 'instance'.
Do this throughout.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 29 at r4 (raw file):

$SQLVersion

Please change to $sqlVersion. Lower-case 'sql'.
Do this throughout.

Also change to single quote ' around the strings that do not have variables in them.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 31 at r4 (raw file):

$RSConfig

Could you please change this to $reportingServicesConfiguration so we don't have abbreviations.
Do this throughout.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 32 at r4 (raw file):

        $RSConfig = Get-WmiObject -Class MSReportServer_ConfigurationSetting -Namespace "root\Microsoft\SQLServer\ReportServer\RS_$InstanceName\v$SQLVersion\Admin"
        if($RSConfig.DatabaseServerName.Contains("\"))

Please add a blank space after each if-statement.

Also change to single quote ' around the string


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 34 at r4 (raw file):

        if($RSConfig.DatabaseServerName.Contains("\"))
        {
            $RSSQLServer = $RSConfig.DatabaseServerName.Split("\")[0]

Please change to single quote ' around the string


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 35 at r4 (raw file):

        {
            $RSSQLServer = $RSConfig.DatabaseServerName.Split("\")[0]
            $RSSQLInstanceName = $RSConfig.DatabaseServerName.Split("\")[1]

Please change to single quote ' around the string


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 40 at r4 (raw file):

        {
            $RSSQLServer = $RSConfig.DatabaseServerName
            $RSSQLInstanceName = "MSSQLSERVER"

Please change to single quote ' around the string


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

            $RSSQLInstanceName = "MSSQLSERVER"
        }
        $IsInitialized = $RSConfig.IsInitialized

Please add a blank row before this row.


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

$IsInitialized

Please change to $isInitialized. Lower-case 'i' in 'is'.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 45 at r4 (raw file):

    }
    else
    {  

Two blank space on this row.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 59 at r4 (raw file):

}

Please remove extra blank line here


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 60 at r4 (raw file):

function Set-TargetResource

Could you please add comment-based help to the Set-TargetResource function?

Example:
https://github.com/PowerShell/xSQLServer/blob/dev/DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1#L61-L95


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 65 at r4 (raw file):

    param
    (
        [parameter(Mandatory = $true)]

Change to 'Parameter' - Upper 'P'


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 69 at r4 (raw file):

        $InstanceName,

        [parameter(Mandatory = $true)]

Change to 'Parameter' - Upper 'P'


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 73 at r4 (raw file):

        $RSSQLServer,

        [parameter(Mandatory = $true)]

Change to 'Parameter' - Upper 'P'


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 78 at r4 (raw file):

    )

    if(Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\Instance Names\RS" -Name $InstanceName -ErrorAction SilentlyContinue)

Please add a blank space after each if-statement.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 80 at r4 (raw file):

    if(Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\Instance Names\RS" -Name $InstanceName -ErrorAction SilentlyContinue)
    {
        # smart import of the SQL module

What do you mean by 'smart import'?


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

        Import-SQLPSModule

        $InstanceKey = (Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\Instance Names\RS" -Name $InstanceName).$InstanceName

This has the same path as the one above, could we add the path to a variable, and use the variable in both places instead?

Please change to single quote ' around the string


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

        Import-SQLPSModule

        $InstanceKey = (Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\Instance Names\RS" -Name $InstanceName).$InstanceName

Please change to $instanceKey, or rather $instanceId to align with other resources. Lower-case 'i' in 'instance'.
Do this throughout.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 84 at r4 (raw file):

[int]

Could we change to [System.Int32]?


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 84 at r4 (raw file):

$SQLVersion

Please change to $sqlVersion. Lower-case 'sql'.
Do this throughout.


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

        $InstanceKey = (Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\Instance Names\RS" -Name $InstanceName).$InstanceName
        $SQLVersion = [int]((Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\$InstanceKey\Setup" -Name "Version").Version).Split(".")[0]
        if($InstanceName -eq "MSSQLSERVER")

Please add a blank space after each if-statement.

And please add a blank row before this row.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 87 at r4 (raw file):

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

Please change to single quote ' around the string


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 88 at r4 (raw file):

        {
            $RSServiceName = "ReportServer"
            $RSVirtualDirectory = "ReportServer"

Please change to single quote ' around the string


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

            $RSServiceName = "ReportServer"
            $RSVirtualDirectory = "ReportServer"
            $RMVirtualDirectory = "Reports"

Please change to single quote ' around the string


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

            $RSVirtualDirectory = "ReportServer"
            $RMVirtualDirectory = "Reports"
            $RSDatabase = "ReportServer"

Please change to single quote ' around the string


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 99 at r4 (raw file):

            $RSDatabase = "ReportServer`$$InstanceName"
        }
        if($RSSQLInstanceName -eq "MSSQLSERVER")

Please add a blank space after each if-statement.

And please add a blank row before this row.

Please change to single quote ' around the string


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 101 at r4 (raw file):

$RSConnection

Could you please change this to $reportingServicesConnnection so we don't have abbreviations.
Do this throughout.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 107 at r4 (raw file):

            $RSConnection = "$RSSQLServer\$RSSQLInstanceName"
        }
        $Language = (Get-WMIObject -Class Win32_OperatingSystem -Namespace root/cimv2 -ErrorAction SilentlyContinue).OSLanguage

Please add a blank row before this row.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 107 at r4 (raw file):

$Language

Please change to $language. Lower-case 'l'.
Do this throughout.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 108 at r4 (raw file):

        }
        $Language = (Get-WMIObject -Class Win32_OperatingSystem -Namespace root/cimv2 -ErrorAction SilentlyContinue).OSLanguage
        $RSConfig = Get-WmiObject -Class MSReportServer_ConfigurationSetting -Namespace "root\Microsoft\SQLServer\ReportServer\RS_$InstanceName\v$SQLVersion\Admin"

Could you please change this to $reportingServicesConfiguration so we don't have abbreviations.
Do this throughout.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 109 at r4 (raw file):

        $Language = (Get-WMIObject -Class Win32_OperatingSystem -Namespace root/cimv2 -ErrorAction SilentlyContinue).OSLanguage
        $RSConfig = Get-WmiObject -Class MSReportServer_ConfigurationSetting -Namespace "root\Microsoft\SQLServer\ReportServer\RS_$InstanceName\v$SQLVersion\Admin"
        if($RSConfig.VirtualDirectoryReportServer -ne $RSVirtualDirectory)

Please add a blank space after each if-statement.

And please add a blank row before this row.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 111 at r4 (raw file):

        if($RSConfig.VirtualDirectoryReportServer -ne $RSVirtualDirectory)
        {
            $null = $RSConfig.SetVirtualDirectory("ReportServerWebService",$RSVirtualDirectory,$Language)

Please change to single quote ' around the string


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 112 at r4 (raw file):

        {
            $null = $RSConfig.SetVirtualDirectory("ReportServerWebService",$RSVirtualDirectory,$Language)
            $null = $RSConfig.ReserveURL("ReportServerWebService","http://+:80",$Language)

Please change to single quote ' around the string


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 114 at r4 (raw file):

            $null = $RSConfig.ReserveURL("ReportServerWebService","http://+:80",$Language)
        }
        if($RSConfig.VirtualDirectoryReportManager -ne $RMVirtualDirectory)

Please add a blank space after each if-statement.

And please add a blank row before this row.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 120 at r4 (raw file):

            $virtualDirectoryName = if ($SQLVersion -ge 13) { 'ReportServerWebApp' } else { 'ReportManager'}
            $null = $RSConfig.SetVirtualDirectory($virtualDirectoryName,$RMVirtualDirectory,$Language)
            $null = $RSConfig.ReserveURL($virtualDirectoryName,"http://+:80",$Language)

Please change to single quote ' around the string


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 122 at r4 (raw file):

            $null = $RSConfig.ReserveURL($virtualDirectoryName,"http://+:80",$Language)
        }
        $RSCreateScript = $RSConfig.GenerateDatabaseCreationScript($RSDatabase,$Language,$false)

Please add a blank row before this row.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 130 at r4 (raw file):

        Invoke-Sqlcmd -ServerInstance $RSConnection -Query $RSCreateScript.Script
        Invoke-Sqlcmd -ServerInstance $RSConnection -Query $RSRightsScript.Script
        $null = $RSConfig.SetDatabaseConnection($RSConnection,$RSDatabase,2,"","")

Please change to single quote ' around the string


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

    }

    if(!(Test-TargetResource @PSBoundParameters))

Please add a blank space after each if-statement.

And please add a blank row before this row.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 140 at r4 (raw file):

}

Please remove extra blank line here


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 141 at r4 (raw file):

function Test-TargetResource

Could you please add comment-based help to the Test-TargetResource function?

Example:
https://github.com/PowerShell/xSQLServer/blob/dev/DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1#L228-L268


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 147 at r4 (raw file):

    param
    (
        [parameter(Mandatory = $true)]

Change to 'Parameter' - Upper 'P'


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 151 at r4 (raw file):

        $InstanceName,

        [parameter(Mandatory = $true)]

Change to 'Parameter' - Upper 'P'


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 155 at r4 (raw file):

        $RSSQLServer,

        [parameter(Mandatory = $true)]

Change to 'Parameter' - Upper 'P'


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 161 at r4 (raw file):

    $result = (Get-TargetResource @PSBoundParameters).IsInitialized
    

Four blank spaces here.
Btw. If you are using VS Code you can turn on automatic removal of these spaces.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 164 at r4 (raw file):

    $result
}

Please remove extra blank line here


Comments from Reviewable

@johlju johlju 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 Jun 8, 2017
@bozho
Copy link
Contributor Author

bozho commented Jun 9, 2017

No worries, mate. You're code reviewing my stuff and writing tests :-)

@bozho
Copy link
Contributor Author

bozho commented Jun 9, 2017

Review status: 2 of 3 files reviewed at latest revision, 54 unresolved discussions.


CHANGELOG.md, line 15 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add "(issue #569)" so there is a reference to the issue number.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 5 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please change this row to this, so it is the same as in other resource.

Import-Module -Name (Join-Path -Path (Split-Path (Split-Path $PSScriptRoot -Parent) -Parent) `
                               -ChildPath 'xSQLServerHelper.psm1') `
                               -Force

After fixing this, please remove line 1-4 which is not needed (?).

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 7 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please add comment-based help to the Get-TargetResource function?

Example:
https://github.com/PowerShell/xSQLServer/blob/dev/DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1#L4-L13

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 13 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to 'Parameter' - Upper 'P'

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 17 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to 'Parameter' - Upper 'P'

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 21 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to 'Parameter' - Upper 'P'

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 26 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank space after each if-statement.

Also change to single quote ' around the string

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 28 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$InstanceKey

Please change to $instanceKey, or rather $instanceId to align with other resources. Lower-case 'i' in 'instance'.
Do this throughout.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 29 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$SQLVersion

Please change to $sqlVersion. Lower-case 'sql'.
Do this throughout.

Also change to single quote ' around the strings that do not have variables in them.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 31 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$RSConfig

Could you please change this to $reportingServicesConfiguration so we don't have abbreviations.
Do this throughout.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 32 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank space after each if-statement.

Also change to single quote ' around the string

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 34 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 35 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 40 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

$IsInitialized

Please change to $isInitialized. Lower-case 'i' in 'is'.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 45 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Two blank space on this row.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 65 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to 'Parameter' - Upper 'P'

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 69 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to 'Parameter' - Upper 'P'

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 73 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to 'Parameter' - Upper 'P'

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 78 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank space after each if-statement.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 80 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

What do you mean by 'smart import'?

Import-SQLPSModule cmdlet will import SQLPS (SQL 2012/14) or SqlServer module (SQL 2016), and if importing SQLPS, change directory back to the original one, since SQLPS changes the current directory to SQLSERVER:\ on import.


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

Previously, johlju (Johan Ljunggren) wrote…

Please change to $instanceKey, or rather $instanceId to align with other resources. Lower-case 'i' in 'instance'.
Do this throughout.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 84 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[int]

Could we change to [System.Int32]?

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 84 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$SQLVersion

Please change to $sqlVersion. Lower-case 'sql'.
Do this throughout.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 87 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 88 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 101 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$RSConnection

Could you please change this to $reportingServicesConnnection so we don't have abbreviations.
Do this throughout.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 107 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$Language

Please change to $language. Lower-case 'l'.
Do this throughout.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 108 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please change this to $reportingServicesConfiguration so we don't have abbreviations.
Do this throughout.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 111 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 112 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 120 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 130 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please change to single quote ' around the string

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank space after each if-statement.

And please add a blank row before this row.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 147 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to 'Parameter' - Upper 'P'

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 151 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to 'Parameter' - Upper 'P'

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 155 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to 'Parameter' - Upper 'P'

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 161 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Four blank spaces here.
Btw. If you are using VS Code you can turn on automatic removal of these spaces.

Done.


Comments from Reviewable

@bozho
Copy link
Contributor Author

bozho commented Jun 9, 2017

Review status: 2 of 3 files reviewed at latest revision, 54 unresolved discussions.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 28 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This has the same path as the one above, could we add the path to a variable, and use the variable in both places instead?

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank row before this row.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 59 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please remove extra blank line here

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 60 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please add comment-based help to the Set-TargetResource function?

Example:
https://github.com/PowerShell/xSQLServer/blob/dev/DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1#L61-L95

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

This has the same path as the one above, could we add the path to a variable, and use the variable in both places instead?

Please change to single quote ' around the string

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank space after each if-statement.

And please add a blank row before this row.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 99 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank space after each if-statement.

And please add a blank row before this row.

Please change to single quote ' around the string

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 107 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank row before this row.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 109 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank space after each if-statement.

And please add a blank row before this row.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 114 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank space after each if-statement.

And please add a blank row before this row.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 122 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a blank row before this row.

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 140 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please remove extra blank line here

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 141 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please add comment-based help to the Test-TargetResource function?

Example:
https://github.com/PowerShell/xSQLServer/blob/dev/DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1#L228-L268

Done.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 164 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please remove extra blank line here

Done.


Comments from Reviewable

@johlju johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Jun 9, 2017
@johlju
Copy link
Member

johlju commented Jun 9, 2017

Thanks for the changes! And I'm happy to help where I can :)

A little more comments, just style again. I also missed some rows in the previous review.


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


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 80 at r4 (raw file):

Previously, bozho (Marko Bozikovic) wrote…

Import-SQLPSModule cmdlet will import SQLPS (SQL 2012/14) or SqlServer module (SQL 2016), and if importing SQLPS, change directory back to the original one, since SQLPS changes the current directory to SQLSERVER:\ on import.

Ah, then I suggest that you could add that in a comment block instead of the present comment? :)

<#
    Import-SQLPSModule cmdlet will import SQLPS (SQL 2012/14) or SqlServer module (SQL 2016), 
    and if importing SQLPS, change directory back to the original one, since SQLPS changes the 
    current directory to SQLSERVER:\ on import.
#>

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

                               -Force

Could you remove two of the blank rows here so only one remains?


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

$RSServiceName

I missed these in previous review, saw them as parameters, but they weren't.
Could you change this to $reportingServicesServiceName?


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

$RSVirtualDirectory

I would like to change this to something else (not abbreviated). Do you have any suggestions?
Maybe $reportingServicesReportServerVirtualDirectoryName or $reportServerVirtualDirectoryName?


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

$RMVirtualDirectory

I would like to change this to something else (not abbreviated). Do you have any suggestions?
Maybe $reportingServicesReportsVirtualDirectoryName or $reportsVirtualDirectoryName?


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

$RSDatabase

Could you change this to $reportingServicesDatabaseName?


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

        else
        {
            $RSServiceName = "ReportServer`$$InstanceName"

Same comment as above for this line and the three rows below.


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

        if ( $RSSQLInstanceName -eq 'MSSQLSERVER' )
        {
            $reportingServicesConnnection = "$RSSQLServer"

Should be needed to have quotes around this variable?


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

        if ( $reportingServicesConfiguration.VirtualDirectoryReportManager -ne $RMVirtualDirectory )
        {
            # SSRS Web Portal application name changed in SQL Server 2016

This two comments need to be a comment block.

<#
    Text ....
#>

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

            # SSRS Web Portal application name changed in SQL Server 2016
            # https://docs.microsoft.com/en-us/sql/reporting-services/breaking-changes-in-sql-server-reporting-services-in-sql-server-2016
            $virtualDirectoryName = if ($sqlVersion -ge 13) { 'ReportServerWebApp' } else { 'ReportManager'}

Could you please put the braces on separate lines - and could we do it like below which would be easier to read. But the way it is done is okay also as long as the braces are on a separate line.

if ($sqlVersion -ge 13)
{
    $virtualDirectoryName = 'ReportServerWebApp'
}
else
{
    $virtualDirectoryName = 'ReportManager'
}

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

$RSCreateScript

Could you change this to $reportingServicesDatabaseScript? Or do you have another suggestion?


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

$RSSvcAccountUsername

Could you change this to $reportingServicesServiceAccountUserName? Or do you have another suggestion?


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

$RSRightsScript

Could you change this to $reportingServicesDatabaseRightsScript? Or do you have another suggestion?


Comments from Reviewable

@johlju johlju 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 Jun 9, 2017
@bozho
Copy link
Contributor Author

bozho commented Jun 14, 2017

Review status: 2 of 3 files reviewed at latest revision, 13 unresolved discussions.


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

Previously, johlju (Johan Ljunggren) wrote…

$RSServiceName

I missed these in previous review, saw them as parameters, but they weren't.
Could you change this to $reportingServicesServiceName?

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

$RSVirtualDirectory

I would like to change this to something else (not abbreviated). Do you have any suggestions?
Maybe $reportingServicesReportServerVirtualDirectoryName or $reportServerVirtualDirectoryName?

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

$RMVirtualDirectory

I would like to change this to something else (not abbreviated). Do you have any suggestions?
Maybe $reportingServicesReportsVirtualDirectoryName or $reportsVirtualDirectoryName?

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

$RSDatabase

Could you change this to $reportingServicesDatabaseName?

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Same comment as above for this line and the three rows below.

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Should be needed to have quotes around this variable?

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

$RSCreateScript

Could you change this to $reportingServicesDatabaseScript? Or do you have another suggestion?

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

$RSSvcAccountUsername

Could you change this to $reportingServicesServiceAccountUserName? Or do you have another suggestion?

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

$RSRightsScript

Could you change this to $reportingServicesDatabaseRightsScript? Or do you have another suggestion?

Done.


Comments from Reviewable

@bozho
Copy link
Contributor Author

bozho commented Jun 14, 2017

Review status: 2 of 3 files reviewed at latest revision, 13 unresolved discussions.


DSCResources/MSFT_xSQLServerRSConfig/MSFT_xSQLServerRSConfig.psm1, line 80 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ah, then I suggest that you could add that in a comment block instead of the present comment? :)

<#
    Import-SQLPSModule cmdlet will import SQLPS (SQL 2012/14) or SqlServer module (SQL 2016), 
    and if importing SQLPS, change directory back to the original one, since SQLPS changes the 
    current directory to SQLSERVER:\ on import.
#>

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Could you remove two of the blank rows here so only one remains?

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

This two comments need to be a comment block.

<#
    Text ....
#>

Done.


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

Previously, johlju (Johan Ljunggren) wrote…

Could you please put the braces on separate lines - and could we do it like below which would be easier to read. But the way it is done is okay also as long as the braces are on a separate line.

if ($sqlVersion -ge 13)
{
    $virtualDirectoryName = 'ReportServerWebApp'
}
else
{
    $virtualDirectoryName = 'ReportManager'
}

Done.


Comments from Reviewable

@bozho
Copy link
Contributor Author

bozho commented Jun 14, 2017

Done. Sorry for the delay, got distracted by priorities at work :)

@bozho bozho force-pushed the issue-569-sql2016-virtual-dir-name branch from 3adff1c to 008c15d Compare June 14, 2017 10:57
@bozho
Copy link
Contributor Author

bozho commented Jun 14, 2017

Force pushed a newline change.

@johlju
Copy link
Member

johlju commented Jun 15, 2017

No worries! :) The code looks good now. I have to improve the tests a little bit, so the comments below, I can fix them. I don't know why I didn't do that at the beginning, feels like I cheated. 😄


Reviewed 1 of 1 files at r3, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 24 at r6 (raw file):

function Invoke-TestSetup {
    #Add-Type -Path (Join-Path -Path (Join-Path -Path (Join-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'Tests') -ChildPath 'Unit') -ChildPath 'Stubs') -ChildPath 'SqlPowerShellSqlExecutionException.cs')

I missed to remove this comment.


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 163 at r6 (raw file):

'Should return the the state as initialized'

Should be: 'Should throw the correct error message'


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 206 at r6 (raw file):

                    It 'Should configure Reporting Service without throwing an error' {
                        { Set-TargetResource @testParameters } | Should Not Throw

There should be some Assert-MockCalled here, and we need to verify that the methods are actually called.


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 209 at r6 (raw file):

'When there is no Reporting Services instance'

'When there is no Reporting Services instance after Set-TargetResource has been called'


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 215 at r6 (raw file):

'Should return the the state as initialized'

Should be: 'Should throw the correct error message'


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 249 at r6 (raw file):

                    It 'Should configure Reporting Service without throwing an error' {
                        { Set-TargetResource @testParameters } | Should Not Throw

There should be some Assert-MockCalled here, and we need to verify that the methods are actually called.There should be some Assert-MockCalled here, and we need to verify that the methods are actually called.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Jun 15, 2017

Review status: 2 of 3 files reviewed at latest revision, 6 unresolved discussions.


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 24 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I missed to remove this comment.

Done


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 163 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'Should return the the state as initialized'

Should be: 'Should throw the correct error message'

Done


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 206 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

There should be some Assert-MockCalled here, and we need to verify that the methods are actually called.

Done


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 209 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'When there is no Reporting Services instance'

'When there is no Reporting Services instance after Set-TargetResource has been called'

Done


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 215 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'Should return the the state as initialized'

Should be: 'Should throw the correct error message'

Done


Tests/Unit/MSFT_xSQLServerRSConfig.Tests.ps1, line 249 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

There should be some Assert-MockCalled here, and we need to verify that the methods are actually called.There should be some Assert-MockCalled here, and we need to verify that the methods are actually called.

Done


Comments from Reviewable

@johlju johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Jun 15, 2017
@johlju
Copy link
Member

johlju commented Jun 15, 2017

:lgtm:


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


Comments from Reviewable

@johlju johlju removed the needs review The pull request needs a code review. label Jun 15, 2017
@johlju johlju merged commit 31bc58d into dsccommunity:dev Jun 15, 2017
@bozho bozho deleted the issue-569-sql2016-virtual-dir-name branch June 16, 2017 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants