xSQLServerServiceAccount: New Resource Added

[nabrond]

Pull Request (PR) description
Added new resource xSQLServerServiceAccount with examples and localization information.

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

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 #795 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##            dev   #795    +/-   ##
====================================
+ Coverage    96%    96%   +<1%     
====================================
  Files        30     31     +1     
  Lines      3259   3310    +51     
====================================
+ Hits       3142   3193    +51     
  Misses      117    117

[johlju]

@nabrond Awesome work! I will try to review the PR this weekend!

[johlju]

I will review the tests tomorrow!

---

Reviewed 4 of 8 files at r2, 3 of 4 files at r3.
Review status: 7 of 8 files reviewed at latest revision, 37 unresolved discussions.

---

CHANGELOG.md, line 88 at r3 (raw file):

  - Fixed PS Script Analyzer errors ([issue #729](https://github.com/PowerShell/xSQLServer/issues/729))
- Added new resource (issue #706)
  - xSQLServerServiceAccount

Could you please move this row to the previous row?

'- Added new resource xSQLServerServiceAccount (issue #706)'

---

README.md, line 155 at r3 (raw file):

* [**xWaitForAvailabilityGroup**](#xwaitforavailabilitygroup) resource to wait until
  availability group is created on primary server.
* [**xSQLServerServiceAccount**](#xsqlserverserviceaccount) Manage the service account

Please move this row so it's listed in alphabetical order.

---

README.md, line 1217 at r3 (raw file):

Manage the service account for SQL Server services.

#### Requirements

Please add requirements (see other resources). If you feel there are no requirements, then maybe we should write 'None.' here.

---

README.md, line 1222 at r3 (raw file):

SQLInstance

Should be 'SQLInstanceName'

---

README.md, line 1223 at r3 (raw file):

SQLInstance

Should be 'SQLInstanceName'

And full stop '.' at the end of the sentence.

---

README.md, line 1224 at r3 (raw file):

"

Got a double-quote here.

---

README.md, line 1224 at r3 (raw file):

SqlServerIntegrationService

Should be 'IntegrationServices'. Throughout the PR.

Looking at name of components here:
https://docs.microsoft.com/en-us/sql/sql-server/editions-and-components-of-sql-server-2016

---

README.md, line 1224 at r3 (raw file):

AnalysisServer

Should be 'AnalysisServices'. Throughout the PR.

---

README.md, line 1224 at r3 (raw file):

SqlServer

Should be 'DatabaseEngine'. Throughout the PR.

---

README.md, line 1224 at r3 (raw file):

SqlAgent

Should be 'SQLServerAgent'. Throughout the PR.

---

README.md, line 1225 at r3 (raw file):

ReportServer

Should be 'ReportingServices'. Throughout the PR.

---

README.md, line 1225 at r3 (raw file):

SqlBrowser

Should be 'SQLServerBrowser'? Throughout the PR.

https://docs.microsoft.com/en-us/sql/database-engine/configure-windows/sql-server-browser-service-database-engine-and-ssas

---

README.md, line 1225 at r3 (raw file):

NotificationServer

Should be 'NotificationServices'. Throughout the PR.

---

README.md, line 1231 at r3 (raw file):

  automatically restarted.
* **`[Boolean]` Force** (Write): Forces the service account to be updated.
  Useful for password changes.

Please add something that will cause that 'Set-TargetResource' will run on each consecutive run.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 10 at r3 (raw file):

LoadWithPartialName

We can't use LoadWithPartialName anymore. That will cause problems where, in certain scenarios, the wrong assembly can be loaded (see issue #649).

Please use Import-SQLPSModule if possible (in the *-TargetResource functions). Or at least use Register-SqlWmiManagement so when we change the behaviour of that helper function to support all scenarios (for example when using SqlServer module).
But I think it should be enough to use Import-SQLPSModule when I get my working branch for issue #649 merged (which I hope will be soon! I got the needed changes in common tests merged yesterday).

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 11 at r3 (raw file):

New-VerboseMessage -Message $script:localizedData.LoadingAssemblies
$null = [System.Reflection.Assembly]::LoadWithPartialName('Microsoft.SqlServer.SqlWmiManagement')
$null = [System.Reflection.Assembly]::LoadWithPartialName('Microsoft.SqlServer.WmiEnum')

We can't use LoadWithPartialName anymore. Please use Import-SQLPSModule if possible (in the *-TargetResource functions). See above comment.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 15 at r3 (raw file):

<#
    .SYNOPSIS
    Gets the service account for the specified instance

These lines should be indented one step. Throughout the comment-based help.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 18 at r3 (raw file):

    .PARAMETER SQLServer
    Host name of the SQL Server to manage

And full stop '.' at the end of the sentence.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 25 at r3 (raw file):

    .PARAMETER ServiceType
    Type of service to be managed. Must be one of the following:
    SqlServer, SqlAgent, Search, SqlServerIntegrationService, AnalysisServer, ReportServer, SqlBrowser, NotificationServer

And full stop '.' at the end of the sentence (I think)?

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 38 at r3 (raw file):

[String]

Could we use [System.String]? Throughout all parameters.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 39 at r3 (raw file):

= $env:COMPUTERNAME

Since this is mandatory, this should not be set. You most likely got a PSSA warning in the common tests as well. Throughout on all mandatory parameters that has default values.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 51 at r3 (raw file):

[PSCredential]

Could we use [System.Management.Automation.PSCredential]? Throughout.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 72 at r3 (raw file):

Where-Object {

Add -FilterScript. And please move the content of the filter script on the next line.

$serviceObject = $managedComputer.Services | Where-Object -FilterScript { 
    ($_.Type -eq $ServiceType) -and ($_.Name -imatch $serviceNamePattern)
}

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 95 at r3 (raw file):

    .PARAMETER SQLServer
    Host name of the SQL Server to manage

And full stop '.' at the end of the sentence.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 102 at r3 (raw file):

    .PARAMETER ServiceType
    Type of service to be managed. Must be one of the following:
    SqlServer, SqlAgent, Search, SqlServerIntegrationService, AnalysisServer, ReportServer, SqlBrowser, NotificationServer

And full stop '.' at the end of the sentence (I think)?

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 108 at r3 (raw file):

    .PARAMETER RestartService
    Determines whether the service is automatically restarted

And full stop '.' at the end of the sentence.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 120 at r3 (raw file):

[Boolean]

Couyld we use [System.Boolean]?

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 140 at r3 (raw file):

        $ServiceAccount,

        [Boolean]

Could we use [System.Boolean]? Throughout all parameters.

Also, please add [Parameter()] to all non.mandatory parameters. Throughout.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 165 at r3 (raw file):

    .PARAMETER SQLServer
    Host name of the SQL Server to manage

And full stop '.' at the end of the sentence.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 172 at r3 (raw file):

    .PARAMETER ServiceType
    Type of service to be managed. Must be one of the following:
    SqlServer, SqlAgent, Search, SqlServerIntegrationService, AnalysisServer, ReportServer, SqlBrowser, NotificationServer

And full stop '.' at the end of the sentence (I think)?

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 178 at r3 (raw file):

    .PARAMETER RestartService
    Determines whether the service is automatically restarted

And full stop '.' at the end of the sentence.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 184 at r3 (raw file):

    .EXAMPLE
    Set-TargetResource -SQLServer $env:COMPUTERNAME -SQLInstaneName MSSQLSERVER -ServiceType SqlServer -ServiceAccount $account

Maybe add an example for the Get-TargetResource as well? No other resources are using examples for the *-TargetResource functions (i think), but since you added them, lets be consequent in all functions here :)

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 215 at r3 (raw file):

    )

    $verboseMessage = $script:localizedData.ConnectingToWmi -f $SQLServer

Line 215-239 seems duplicate to the Get-TargetResource function. Could this be added to a helper function in this resource module instead of duplicating code?

---

DSCResources/MSFT_xSQLServerServiceAccount/en-US/MSFT_xSQLServerServiceAccount.strings.psd1, line 5 at r3 (raw file):

Force specified, skipping tests.

Could you make this a bit more descriptive - telling maybe that Test-TargetResource will always return $false (or something).

---

DSCResources/MSFT_xSQLServerServiceAccount/en-US/MSFT_xSQLServerServiceAccount.strings.psd1, line 6 at r3 (raw file):

Current service account is '{0}'.

Maybe add the service name in this string as well?

---

DSCResources/MSFT_xSQLServerServiceAccount/en-US/MSFT_xSQLServerServiceAccount.strings.psd1, line 8 at r3 (raw file):

Setting service account to '{0}'.

Maybe add the service name in this string as well?

---

Examples/Resources/xSQLServerServiceAccount/2-ConfigureServiceAccount-VirtualAccount.ps1, line 5 at r3 (raw file):

    This example shows how to ensure the SQL Server service
    on TestServer\DSC is running under a virtual account.
    Restart the service after updating.

Please explain what the Force does as well.

---

Comments from Reviewable

[nabrond]

The values for ServiceType were taken from the ManagedServiceType enumeration. We would need to maintain a mapping table to be able to use other values. Do you see value in this?

---

Review status: 2 of 8 files reviewed at latest revision, 38 unresolved discussions.

---

CHANGELOG.md, line 88 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please move this row to the previous row?

'- Added new resource xSQLServerServiceAccount (issue #706)'

Done.

---

README.md, line 155 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please move this row so it's listed in alphabetical order.

Done.

---

README.md, line 1217 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add requirements (see other resources). If you feel there are no requirements, then maybe we should write 'None.' here.

Done.

---

README.md, line 1222 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SQLInstance

Should be 'SQLInstanceName'

Done.

---

README.md, line 1223 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SQLInstance

Should be 'SQLInstanceName'

And full stop '.' at the end of the sentence.

Done.

---

README.md, line 1224 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"

Got a double-quote here.

Done.

---

README.md, line 1224 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SqlServerIntegrationService

Should be 'IntegrationServices'. Throughout the PR.

Looking at name of components here:
https://docs.microsoft.com/en-us/sql/sql-server/editions-and-components-of-sql-server-2016

See comment from PR.

---

README.md, line 1224 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

AnalysisServer

Should be 'AnalysisServices'. Throughout the PR.

See comment from PR.

---

README.md, line 1224 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SqlServer

Should be 'DatabaseEngine'. Throughout the PR.

See comment from PR.

---

README.md, line 1224 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SqlAgent

Should be 'SQLServerAgent'. Throughout the PR.

See comment from PR.

---

README.md, line 1225 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

ReportServer

Should be 'ReportingServices'. Throughout the PR.

See comment from PR.

---

README.md, line 1225 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SqlBrowser

Should be 'SQLServerBrowser'? Throughout the PR.

https://docs.microsoft.com/en-us/sql/database-engine/configure-windows/sql-server-browser-service-database-engine-and-ssas

See comment from PR.

---

README.md, line 1225 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

NotificationServer

Should be 'NotificationServices'. Throughout the PR.

See comment from PR.

---

README.md, line 1231 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add something that will cause that 'Set-TargetResource' will run on each consecutive run.

Good catch. Forgot about that from the other discussion! Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 10 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

LoadWithPartialName

We can't use LoadWithPartialName anymore. That will cause problems where, in certain scenarios, the wrong assembly can be loaded (see issue #649).

Please use Import-SQLPSModule if possible (in the *-TargetResource functions). Or at least use Register-SqlWmiManagement so when we change the behaviour of that helper function to support all scenarios (for example when using SqlServer module).
But I think it should be enough to use Import-SQLPSModule when I get my working branch for issue #649 merged (which I hope will be soon! I got the needed changes in common tests merged yesterday).

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 11 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We can't use LoadWithPartialName anymore. Please use Import-SQLPSModule if possible (in the *-TargetResource functions). See above comment.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 15 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

These lines should be indented one step. Throughout the comment-based help.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 18 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

And full stop '.' at the end of the sentence.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 25 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

And full stop '.' at the end of the sentence (I think)?

It seems weird to have a full stop on this line, but I added it anyway. Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 38 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[String]

Could we use [System.String]? Throughout all parameters.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 39 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

= $env:COMPUTERNAME

Since this is mandatory, this should not be set. You most likely got a PSSA warning in the common tests as well. Throughout on all mandatory parameters that has default values.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 51 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[PSCredential]

Could we use [System.Management.Automation.PSCredential]? Throughout.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 72 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Where-Object {

Add -FilterScript. And please move the content of the filter script on the next line.

$serviceObject = $managedComputer.Services | Where-Object -FilterScript { 
    ($_.Type -eq $ServiceType) -and ($_.Name -imatch $serviceNamePattern)
}

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 95 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

And full stop '.' at the end of the sentence.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 102 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

And full stop '.' at the end of the sentence (I think)?

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 108 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

And full stop '.' at the end of the sentence.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 120 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[Boolean]

Couyld we use [System.Boolean]?

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 140 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we use [System.Boolean]? Throughout all parameters.

Also, please add [Parameter()] to all non.mandatory parameters. Throughout.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 165 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

And full stop '.' at the end of the sentence.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 172 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

And full stop '.' at the end of the sentence (I think)?

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 178 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

And full stop '.' at the end of the sentence.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 184 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe add an example for the Get-TargetResource as well? No other resources are using examples for the *-TargetResource functions (i think), but since you added them, lets be consequent in all functions here :)

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 215 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Line 215-239 seems duplicate to the Get-TargetResource function. Could this be added to a helper function in this resource module instead of duplicating code?

Moved code to new function Get-ServiceObject. Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/en-US/MSFT_xSQLServerServiceAccount.strings.psd1, line 5 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Force specified, skipping tests.

Could you make this a bit more descriptive - telling maybe that Test-TargetResource will always return $false (or something).

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/en-US/MSFT_xSQLServerServiceAccount.strings.psd1, line 6 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Current service account is '{0}'.

Maybe add the service name in this string as well?

Service name is not available at this point, added SQL Server and SQL Instance Name to the message. Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/en-US/MSFT_xSQLServerServiceAccount.strings.psd1, line 8 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Setting service account to '{0}'.

Maybe add the service name in this string as well?

Done.

---

Examples/Resources/xSQLServerServiceAccount/2-ConfigureServiceAccount-VirtualAccount.ps1, line 5 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please explain what the Force does as well.

Done.

---

Comments from Reviewable

[nabrond]

Review status: 2 of 8 files reviewed at latest revision, 37 unresolved discussions.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 10 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

This change broke the Appveyor build for this push xSQLServer 6.0.1553. I am going to roll this back for now.

---

Comments from Reviewable

[johlju]

ManagedServiceType

This is a tricky one. Seems that somene realized "the mistake" and added the correct name as the description on each property value (because it would be a major breaking change to fix the property values). 😄
So either we continue using the "wrong name" or change it to the correct name here. When seeing this, I would personally want to change this to the correct names, even if that means adding a switch to do the correct mapping. Because this is the only time we can do this right without making a breaking change down the road.

I let you decide this one.

---

Reviewed 1 of 6 files at r4.
Review status: 3 of 8 files reviewed at latest revision, 31 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[johlju]

Reviewed 3 of 6 files at r4.
Review status: 6 of 8 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 10 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

This change broke the Appveyor build for this push xSQLServer 6.0.1553. I am going to roll this back for now.

Ah. That will be fixed when I send in the PR for issue #649! I was hoping to get that done this weekend, but thin I won't have time. Gonna try to do that tomorrow.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 294 at r4 (raw file):

    return $serviceObject

Remove blank line here.

---

Comments from Reviewable

[nabrond]

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

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 294 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Remove blank line here.

Done.

---

Comments from Reviewable

[johlju]

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

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 10 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ah. That will be fixed when I send in the PR for issue #649! I was hoping to get that done this weekend, but thin I won't have time. Gonna try to do that tomorrow.

When thinking about this. This has nothing to do with the issue #649. In the unit test you should mock Import-SQLPSModule and you should not get the problem you encountered.

---

Comments from Reviewable

[nabrond]

Review status: 2 of 8 files reviewed at latest revision, 9 unresolved discussions.

---

README.md, line 1224 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

See comment from PR.

Done.

---

README.md, line 1224 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

See comment from PR.

Done.

---

README.md, line 1224 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

See comment from PR.

Done.

---

README.md, line 1224 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

See comment from PR.

Done.

---

README.md, line 1225 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

See comment from PR.

Done.

---

README.md, line 1225 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

See comment from PR.

Done.

---

README.md, line 1225 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

See comment from PR.

Done.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 10 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

When thinking about this. This has nothing to do with the issue #649. In the unit test you should mock Import-SQLPSModule and you should not get the problem you encountered.

Done.

---

Comments from Reviewable

[nabrond]

Review status: 2 of 8 files reviewed at latest revision, 9 unresolved discussions.

---

DSCResources/MSFT_xSQLServerServiceAccount/MSFT_xSQLServerServiceAccount.psm1, line 11 at r3 (raw file):

Previously, nabrond (Brandon) wrote…

Done.

I removed this code, added the Import-SQLPSModule call which should load the module and all required assemblies. Tests pass locally, but I am hitting an issue where Test-xDscResource is failing for this module. It seems to be trying to load the SQLPS or SqlServer module and ignoring the Mock. @johlju can you look at this and let me know what I am missing?

---

Comments from Reviewable

[johlju]

Let me merge the fix that runs the integration tests in order and see if that fixes the problem.

[johlju]

@nabrond Please rebase and see if the recently merged PR fixes your issue.

[johlju]

@nabrond Can't see the problem with Test-xDscResource that you mention. But can see it fails on that you are using a SQL type which you need to mock using SMO.cs. You can create another .cs file if it is more appropriate.

RuntimeException: Unable to find type [Microsoft.SqlServer.Management.Smo.Wmi.ManagedServiceType].
        at ConvertTo-ManagedServiceType, 

https://ci.appveyor.com/project/PowerShell/xsqlserver/build/6.0.1566.0#L4489

See example here:

https://github.com/PowerShell/xSQLServer/blob/1a2d2693ee5caf5760badc221b39180213d868bd/Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1#L20

[nabrond]

Seems the issue with Test-xDSCResource was that I was calling Import-SQLPSModule at the top of the script. I moved the use of this function into the two helper functions which return SMO objects. This caused the error you cited above. I created a stub for the enumeration, added the Add-Type line and everything seems to be working now. Done.

---

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

---

Comments from Reviewable

[johlju]

It seems this PR don't see new changes in dev. I'm gonna close and reopen this PR to see if it gets resolved.

[johlju]

@nabrond This PR behaves strangely, see changelog.md and appveyor.yml. It seems it stuck in an older version of the dev branch and wants to merge changes you haven't made. Tried to close and reopen this issue to resolve it, but that didn't help. I haven't seen this before.
If you already rebase, then that won't help. So could I ask you to close and reopen a new PR? Sorry for the trouble.

[nabrond]

Review status: 7 of 9 files reviewed at latest revision, 9 unresolved discussions.

---

Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1, line 25 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

`

Got a back tick at the end of the row here.

Done.

---

Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1, line 54 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[String]

[System.String]

Here and throughout.

Done.

---

Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1, line 66 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$Pass

Please change to $Password

Originally did this to clear some PSSA warnings. Done.

---

Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1, line 122 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Creates a new ManagedComputer object for a default instance that thows an exception
when attempting to set the service account

Please change to comment block (also typo in throws)

<#
    Creates a new ManagedComputer object for a default instance that throws an exception
    when attempting to set the service account.
#>

Done.

---

Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1, line 137 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This value script seems to repeat. Could we add it to a variable maybe?

Done.

---

Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1, line 144 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$Pass

Change to $Password

Done.

---

Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1, line 229 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

These are an array of hash tables. So they need to align to the style guideline too.

$testCases = @(
    @{
        ServiceType = 'DatabaseEngine'
        ExpectedType = 'SqlServer' 
    }
    
    @{
        ServiceType = 'SQLServerAgent'
        ExpectedType = 'SqlAgent'
    }
    ...
)
</blockquote></details>

Done.

---

*[Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1, line 568 at r9](https://reviewable.io:443/reviews/powershell/xsqlserver/795#-Ku5Qe0jpd_x6FQz6B_R:-Ku5lEEyO751QPyEU1AQ:b-896fix) ([raw file](https://github.com/powershell/xsqlserver/blob/e9c8110a749e224023985e6bb390caef06a541ca/Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1#L568)):*
<details><summary><i>Previously, johlju (Johan Ljunggren) wrote…</i></summary><blockquote>

> SericeNotFound

Typo 'ServiceNotFound'
</blockquote></details>

Done.

---

*[Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1, line 652 at r9](https://reviewable.io:443/reviews/powershell/xsqlserver/795#-Ku5R_K4ty262U3Y9MKy:-Ku5lZK1NwKvJWJQg-eU:bs7wumb) ([raw file](https://github.com/powershell/xsqlserver/blob/e9c8110a749e224023985e6bb390caef06a541ca/Tests/Unit/MSFT_xSQLServerServiceAccount.Tests.ps1#L652)):*
<details><summary><i>Previously, johlju (Johan Ljunggren) wrote…</i></summary><blockquote>

> SericeNotFound

Typo 'ServiceNotFound'

</blockquote></details>

Done.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/powershell/xsqlserver/795)*
<!-- Sent from Reviewable.io -->

[johlju]

---

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

---

Comments from Reviewable

[johlju]

@nabrond Awesome work on this one!