Common Module - Improve Connect-SQL and Invoke-Query #1356

kungfoome · 2019-05-03T16:59:13Z

Non-breaking changes

Task list

Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Resource Testing Guidelines.
Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

kungfoome · 2019-05-03T17:10:09Z

@johlju may need help on unit tests here, not sure how to emulate the 'Microsoft.SqlServer.Management.Smo.Server' to test that out.

johlju · 2019-05-03T17:30:26Z

In the phone now so can’t give you a good example, but there are stubs for the SMO object model written I C#. See here how to load them.

https://github.com/PowerShell/SqlServerDsc/blob/a7add04577baa0b9150e1991bc65cadc0fdbf707/Tests/Unit/MSFT_SqlAGListener.Tests.ps1#L43

You can extend the stubs with other properties and methods as needed. They are only minimal of what is needed by the current unit tests.

codecov-io · 2019-05-03T17:45:47Z

Codecov Report

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

@@          Coverage Diff          @@
##            dev   #1356    +/-   ##
=====================================
+ Coverage    98%     98%   +<1%     
=====================================
  Files        36      36            
  Lines      5215    5225    +10     
=====================================
+ Hits       5145    5155    +10     
  Misses       70      70

kungfoome · 2019-05-04T04:55:42Z

@johlju Ready for review. I put verbose back in for now, until i can further investigate this issue.

kungfoome · 2019-05-04T08:41:43Z

@johlju Finally got the test cases to work. Ready for review.

johlju

Reviewed 1 of 4 files at r1, 3 of 3 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @kungfu71186)

Modules/DscResource.Common/DscResource.Common.psm1, line 1430 at r3 (raw file):

Quoted 5 lines of code…

        [Alias("ServerName")]
        [Parameter()]
        [ValidateNotNullOrEmpty()]
        [System.String]
        $SQLServer = $env:COMPUTERNAME,

Maybe we should use parameter sets so that we can have specific mandatory parameters in a parameterset? That way we could keep mandatory = $true in the respectively parameterset
See example https://github.com/PowerShell/SqlServerDsc/blob/85f2d104d2aec5c60062b827820727e745920098/Modules/DscResource.Common/DscResource.Common.psm1#L2071

Modules/DscResource.Common/DscResource.Common.psm1, line 1441 at r3 (raw file):

$Database = 'master',

Why are we defaulting to this database? If the user (resource) want to execute a query in the context of 'master', then the user (resource) should provide that? This could potentially be a problem with permissions, depending what user is executing the resource, or credentials provided in the resource.

I suggest this keeps being mandatory. Would that work?

Modules/DscResource.Common/DscResource.Common.psm1, line 1460 at r3 (raw file):

$SqlManagementObject

I think this should be either $SqlServerObject or $ServerObject. ManagementObject sounds like something else. 🤔

Modules/DscResource.Common/DscResource.Common.psm1, line 1468 at r3 (raw file):

if (-not $PSBoundParameters.ContainsKey('SqlManagementObject'))

See above, maybe we could check what parameter set is used instead? 🤔

Tests/Unit/DscResource.Common.Tests.ps1, line 1505 at r3 (raw file):

Context 'Pass in an SMO Server Object'

We should also have a test that passes in the value from the pipeline.

kungfoome

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @johlju and @kungfu71186)

Modules/DscResource.Common/DscResource.Common.psm1, line 1430 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

        [Alias("ServerName")]
        [Parameter()]
        [ValidateNotNullOrEmpty()]
        [System.String]
        $SQLServer = $env:COMPUTERNAME,
Maybe we should use parameter sets so that we can have specific mandatory parameters in a parameterset? That way we could keep mandatory = $true in the respectively parameterset
See example https://github.com/PowerShell/SqlServerDsc/blob/85f2d104d2aec5c60062b827820727e745920098/Modules/DscResource.Common/DscResource.Common.psm1#L2071

sounds good

Modules/DscResource.Common/DscResource.Common.psm1, line 1441 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$Database = 'master',
Why are we defaulting to this database? If the user (resource) want to execute a query in the context of 'master', then the user (resource) should provide that? This could potentially be a problem with permissions, depending what user is executing the resource, or credentials provided in the resource.

I suggest this keeps being mandatory. Would that work?

don't matter to me, i think i saw it specified like this somewhere else, so i was just making it more consistent.

Modules/DscResource.Common/DscResource.Common.psm1, line 1460 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$SqlManagementObject
I think this should be either $SqlServerObject or $ServerObject. ManagementObject sounds like something else. 🤔

Doesn't matter to me, although that is what SMO is https://docs.microsoft.com/en-us/sql/relational-databases/server-management-objects-smo/sql-server-management-objects-smo-programming-guide?view=sql-server-2017

Tests/Unit/DscResource.Common.Tests.ps1, line 1505 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Context 'Pass in an SMO Server Object'
We should also have a test that passes in the value from the pipeline.

dang, i thought i put that in there. Maybe i took it out when i was trying to get it to work right. I'll put it back in.

kungfoome

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @johlju and @kungfu71186)

Modules/DscResource.Common/DscResource.Common.psm1, line 1430 at r3 (raw file):

Previously, kungfu71186 wrote…

sounds good

Actually, I just realized, it's not actually mandatory. You don't need credentials at all. There's actually 3 options, Windows, SQL, Integrated. If the credentials are not specified, it uses Integrated. Credentials are only mandatory when using Windows and SQL.

kungfoome · 2019-05-12T11:30:25Z

@johlju

I rewrote the connect-sql to make it a bit more clear.

Found a few issues in test i think:
https://github.com/kungfu71186/SqlServerDsc/blob/85f2d104d2aec5c60062b827820727e745920098/Tests/Unit/DscResource.Common.Tests.ps1#L2342
Not sure how it's going to use windows credentials here, since no credentials are specified, so it actually uses integrated security.

https://github.com/kungfu71186/SqlServerDsc/blob/85f2d104d2aec5c60062b827820727e745920098/Tests/Unit/DscResource.Common.Tests.ps1#L2438
No idea what this test is trying to do. Not sure how you get an instance back that you didn't specify. But in any case, I updated that test to check if you can't connect. Otherwise, let me know what that unit test was trying to test for.

The rewrite adds a new type:

        [Parameter()]
        [ValidateSet('Integrated', 'WindowsUser', 'SqlLogin')]
        [System.String]
        $LoginType = 'Integrated'

But the only way to check for credential is to do dynamic parameter i believe. If it's not integrated, then we need a credential and it's mandatory.

Let me know what you think about that. I can update invoke-query to do that as well.

Last thing, should we really have 2 query functions? I didn't notice we had invoke-sqlscript and invoke-query. Shouldn't we just combine these? We can leave one or the other for backward compatibility and then just call one with the other. What do you think?

johlju

I rewrote the connect-sql to make it a bit more clear.

I did not see any new commits yet. 🙂

https://github.com/kungfu71186/SqlServerDsc/blob/85f2d104d2aec5c60062b827820727e745920098/Tests/Unit/DscResource.Common.Tests.ps1#L2342
Not sure how it's going to use windows credentials here, since no credentials are specified, so it actually uses integrated security.

It is testing that it is possible to connect with current credentials, it should be possible to do this, like you mentioned in a comment.

https://github.com/kungfu71186/SqlServerDsc/blob/85f2d104d2aec5c60062b827820727e745920098/Tests/Unit/DscResource.Common.Tests.ps1#L2438
No idea what this test is trying to do. Not sure how you get an instance back that you didn't specify. But in any case, I updated that test to check if you can't connect. Otherwise, let me know what that unit test was trying to test for.

This test can be removed. It seems to test that Connect-SQL throws. But that should not be tested here, it should be, and probably are, tested in the tests for Connect-SQL.

But the only way to check for credential is to do dynamic parameter i believe.

To connect with current credentials we should need to specify 'Integrated'. The default should be to connect using no credentials which uses the current logged in user. 🤔

If it's not integrated, then we need a credential and it's mandatory.

Credentials should not be mandatory.

Last thing, should we really have 2 query functions? I didn't notice we had invoke-sqlscript and invoke-query. Shouldn't we just combine these? We can leave one or the other for backward compatibility and then just call one with the other. >What do you think?

If we can combine the two, we should do that in another PR, and at the same time update all the resources so they use the combined one, and so we can remove the one that is no longer used. We should not leave code for backward compatibility. This should be another PR. :)

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @johlju and @kungfu71186)

Modules/DscResource.Common/DscResource.Common.psm1, line 1430 at r3 (raw file):

Previously, kungfu71186 wrote…

Actually, I just realized, it's not actually mandatory. You don't need credentials at all. There's actually 3 options, Windows, SQL, Integrated. If the credentials are not specified, it uses Integrated. Credentials are only mandatory when using Windows and SQL.

Right, agree. I didn't think of that. If not providing credentials it should try to connect with the credentials of the current user (the user running the resource).

Modules/DscResource.Common/DscResource.Common.psm1, line 1441 at r3 (raw file):

Previously, kungfu71186 wrote…

don't matter to me, i think i saw it specified like this somewhere else, so i was just making it more consistent.

I think we should not hard code this in a helper function, unless there is a specific reason. 🤔

Modules/DscResource.Common/DscResource.Common.psm1, line 1460 at r3 (raw file):

Previously, kungfu71186 wrote…

Doesn't matter to me, although that is what SMO is https://docs.microsoft.com/en-us/sql/relational-databases/server-management-objects-smo/sql-server-management-objects-smo-programming-guide?view=sql-server-2017

It is rather that SQL Server Management Objects (SMO) is a collection of classes, where the class Serveris one, which we use here.
[Microsoft.SqlServer.Management.Smo.Server] is the object type (class) Server within the namespace Microsoft.SqlServer.Management.Smo. Just like String is a type within the namespace System, written like [System.String].

That is why the parameter below that was changed to $smoConnectObject prior to this change was called $serverObject.

johlju · 2019-05-12T16:22:20Z

I hope I answered all questions, let me know if something I says seems odd. Hard to keep track on the changes in all PR's 😄

kungfoome · 2019-05-12T16:46:46Z

@johlju yeah no problem. Makes sense. I haven't committed my changes yet since it takes 40 minutes to do tests, but wanted to hear your thoughts first. I updated the code to hopefully make it more clear.

I'm also trying to not use a bunch of if statements when not needed, so hopefully a few changes I made makes it a bit easier to read.

I'll make those same changes to invoke as well and then commit them. This PR is for both connect and invoke.

On mobile, so can't quote.
"It is testing that it is possible to connect with current credentials, it should be possible to do this, like you mentioned in a comment"

But what I mean by that test case is that credentials is not specified, so it actually uses integrated creds, even though Windows is specified, it actually doesn't do anything in this case

invoke-query with integrated parameter

kungfoome

Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @johlju)

Modules/DscResource.Common/DscResource.Common.psm1, line 1430 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Right, agree. I didn't think of that. If not providing credentials it should try to connect with the credentials of the current user (the user running the resource).

Left it but added another value to the set for more clarity. Done.

Modules/DscResource.Common/DscResource.Common.psm1, line 1441 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I think we should not hard code this in a helper function, unless there is a specific reason. 🤔

Yeah, sounds good to me. Done.

Modules/DscResource.Common/DscResource.Common.psm1, line 1460 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It is rather that SQL Server Management Objects (SMO) is a collection of classes, where the class Serveris one, which we use here.
[Microsoft.SqlServer.Management.Smo.Server] is the object type (class) Server within the namespace Microsoft.SqlServer.Management.Smo. Just like String is a type within the namespace System, written like [System.String].

That is why the parameter below that was changed to $smoConnectObject prior to this change was called $serverObject.

Right, makes sense. I forgot im using the server. Done.

Modules/DscResource.Common/DscResource.Common.psm1, line 1468 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if (-not $PSBoundParameters.ContainsKey('SqlManagementObject'))
See above, maybe we could check what parameter set is used instead? 🤔

Going back to earlier discussions. Only way to really make this work is using dynamic parameters. So, I left it, since we can't make logontype mandatory since it's dependent on credentials as well. Done.

Tests/Unit/DscResource.Common.Tests.ps1, line 1505 at r3 (raw file):

Previously, kungfu71186 wrote…

dang, i thought i put that in there. Maybe i took it out when i was trying to get it to work right. I'll put it back in.

Done.

johlju

Reviewed 1 of 3 files at r4.
Reviewable status: 2 of 4 files reviewed, 10 unresolved discussions (waiting on @johlju and @kungfu71186)

Modules/DscResource.Common/DscResource.Common.psm1, line 1430 at r3 (raw file):

Previously, kungfu71186 wrote…

Left it but added another value to the set for more clarity. Done.

Still think we should look at using parameter sets here. One for using SqlServerObject, and another parameter set for manually specifying the SQLServer, etc. Then we can make those parameters mandatory again for that parameter set.

Modules/DscResource.Common/DscResource.Common.psm1, line 1441 at r3 (raw file):

Previously, kungfu71186 wrote…

Yeah, sounds good to me. Done.

Still seeing 'master' here. 🙂

Modules/DscResource.Common/DscResource.Common.psm1, line 1468 at r3 (raw file):

Previously, kungfu71186 wrote…

Going back to earlier discussions. Only way to really make this work is using dynamic parameters. So, I left it, since we can't make logontype mandatory since it's dependent on credentials as well. Done.

It is not, see comment in Connect-SQL. We should be able to user parametersets in this function 🤔

Modules/DscResource.Common/DscResource.Common.psm1, line 649 at r4 (raw file):

'Integrated'

Not seeing we need this login type, as it should default to 'WindowsUser', see other comment below.

Modules/DscResource.Common/DscResource.Common.psm1, line 659 at r4 (raw file):

$databaseEngineInstance = '{0}\{1}' -f $ServerName, $InstanceName

When called with default values, this would result in 'TEST01\MSSQLSERVER'. This would fail, because there are no instance that is named MSSQLSERVER? To connect to the default instance we should just set the instance name to the server name, e.g. 'TEST01'?

Modules/DscResource.Common/DscResource.Common.psm1, line 666 at r4 (raw file):

Quoted 4 lines of code…

    if ($LoginType -eq 'Integrated')
    {
        $connectUserName = [Environment]::UserName
    }

This is not necessary to connect using the current logged on user. To connect with the current user, all that is needed is.

$sqlServerObject = New-Object -TypeName Microsoft.SqlServer.Management.Smo.Server
$sqlServerObject.ConnectionContext.ServerInstance = 'localhost\sql2017'
$sqlServerObject.ConnectionContext.Connect()
$sqlServerObject.Status
Online

So if no credentials is specified, the default should be to connect without setting any credentials (like the original code did).

Modules/DscResource.Common/DscResource.Common.psm1, line 675 at r4 (raw file):

$sqlServerObject.ConnectionContext

We could use $sqlConnectionContext here instead? And for the other below as well?

Modules/DscResource.Common/DscResource.Common.psm1, line 699 at r4 (raw file):

if ( $sql.Status -match '^Online$' )

Why are we no longer checking that we actually connected to an instance that is online? Connect will connect to an instance even if it is not actually online.

Modules/DscResource.Common/DscResource.Common.psm1, line 1455 at r4 (raw file):

Integrated'

Default should be WindowsUser, see other comment in Connect-SQL.

kungfoome

Reviewable status: 2 of 4 files reviewed, 10 unresolved discussions (waiting on @johlju)

Modules/DscResource.Common/DscResource.Common.psm1, line 1430 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Still think we should look at using parameter sets here. One for using SqlServerObject, and another parameter set for manually specifying the SQLServer, etc. Then we can make those parameters mandatory again for that parameter set.

Ahh, yes that would work. I forgot to put back mandatory on SQLServer. I'm with you now.

Modules/DscResource.Common/DscResource.Common.psm1, line 1441 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Still seeing 'master' here. 🙂

Maybe I undid something, I know I took this out. Probably the same thing that happened above.

Modules/DscResource.Common/DscResource.Common.psm1, line 1468 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It is not, see comment in Connect-SQL. We should be able to user parametersets in this function 🤔

Sorry got confused here, yes this is fine to do a parameter set for sql server and instance and the SMO server object.

Modules/DscResource.Common/DscResource.Common.psm1, line 649 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'Integrated'
Not seeing we need this login type, as it should default to 'WindowsUser', see other comment below.

WindowsUser is an actual windows user. This is where I type in a user name and password and not use my currently logged on credentials. Even the old code did the same thing. This wasn't changed, only added for better clarity. Please see other comment for further description.

Modules/DscResource.Common/DscResource.Common.psm1, line 659 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$databaseEngineInstance = '{0}\{1}' -f $ServerName, $InstanceName
When called with default values, this would result in 'TEST01\MSSQLSERVER'. This would fail, because there are no instance that is named MSSQLSERVER? To connect to the default instance we should just set the instance name to the server name, e.g. 'TEST01'?

Correct, that's why MSSQLSERVER is removed after. It's just an easy way to get rid of an IF statement, just looks a bit cleaner.

Modules/DscResource.Common/DscResource.Common.psm1, line 666 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    if ($LoginType -eq 'Integrated')
    {
        $connectUserName = [Environment]::UserName
    }
This is not necessary to connect using the current logged on user. To connect with the current user, all that is needed is.
$sqlServerObject = New-Object -TypeName Microsoft.SqlServer.Management.Smo.Server
$sqlServerObject.ConnectionContext.ServerInstance = 'localhost\sql2017'
$sqlServerObject.ConnectionContext.Connect()
$sqlServerObject.Status
Online
So if no credentials is specified, the default should be to connect without setting any credentials (like the original code did).

This code does exactly what the old code did. I think there is a confusion on what WindowsUser was for. If you look at the current code. WindowsUser does nothing unless Credentials are specified. Yes, Integrated is a Windows user, but it's not trying to impersonate anyone, which is what WindowsUser does.

See here:

https://github.com/PowerShell/SqlServerDsc/blob/32927e87fe044a7ace8c5ad2452e31e83b6e5879/Modules/DscResource.Common/DscResource.Common.psm1#L696

And windowsUser is specified here:
https://github.com/PowerShell/SqlServerDsc/blob/32927e87fe044a7ace8c5ad2452e31e83b6e5879/Modules/DscResource.Common/DscResource.Common.psm1#L677

Only if credentials are specified, which suggests to me that Windows user is for impersonation, no? I'm basing Integrated off Database connection strings, where Integrated Security=true;. Seems to be default among SMO and invoke-sqlcmd, so I can assure you, we do agree on this.

https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/connection-string-syntax

We recommend using Windows Authentication (sometimes referred to as integrated security) to connect to data sources that support it. TLDR: Windows Authentication /= Windows User.

Modules/DscResource.Common/DscResource.Common.psm1, line 675 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$sqlServerObject.ConnectionContext
We could use $sqlConnectionContext here instead? And for the other below as well?

Yes, that was my intention

Modules/DscResource.Common/DscResource.Common.psm1, line 699 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if ( $sql.Status -match '^Online$' )
Why are we no longer checking that we actually connected to an instance that is online? Connect will connect to an instance even if it is not actually online.

Hmmm, ok. I can change that back, but when i was testing, it would return an error if it couldn't connect. I could see this being an issue if the Database was offline/online, but we are connecting to a server. Just doing a quick search online yields the same results (using try/catch) to check if connected.

I believe I changed this because I was having issues with the old code and noticed why. Connect doesnt check if it's actually connected and spits out an error. https://github.com/PowerShell/SqlServerDsc/blob/32927e87fe044a7ace8c5ad2452e31e83b6e5879/Modules/DscResource.Common/DscResource.Common.psm1#L692

I couldn't connect and it wasn't working properly from what i remember. I can test again though.

Modules/DscResource.Common/DscResource.Common.psm1, line 1455 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Integrated'
Default should be WindowsUser, see other comment in Connect-SQL.

I still believe integrated should be left as default and the logic should be based on logon type and not whether credentials were specified. https://github.com/PowerShell/SqlServerDsc/blob/32927e87fe044a7ace8c5ad2452e31e83b6e5879/Modules/DscResource.Common/DscResource.Common.psm1#L694

kungfoome · 2019-05-13T17:13:14Z

@johlju I'm sticking to my guns on Integrated lol. We are saying the same thing, but our terminology is different i believe. Please see comments for what I mean.

Also, I just noticed why I have server without mandatory for invoke-query:

https://github.com/PowerShell/SqlServerDsc/blob/32927e87fe044a7ace8c5ad2452e31e83b6e5879/Modules/DscResource.Common/DscResource.Common.psm1#L635

I changed it to make it more consistent with Connect-SQL. Let me know if you want them to be different or change them both to make them the same.

johlju

Reviewable status: 2 of 4 files reviewed, 7 unresolved discussions (waiting on @johlju and @kungfu71186)

Modules/DscResource.Common/DscResource.Common.psm1, line 666 at r4 (raw file):

Previously, kungfu71186 wrote…

This code does exactly what the old code did. I think there is a confusion on what WindowsUser was for. If you look at the current code. WindowsUser does nothing unless Credentials are specified. Yes, Integrated is a Windows user, but it's not trying to impersonate anyone, which is what WindowsUser does.

See here:

https://github.com/PowerShell/SqlServerDsc/blob/32927e87fe044a7ace8c5ad2452e31e83b6e5879/Modules/DscResource.Common/DscResource.Common.psm1#L696

And windowsUser is specified here:
https://github.com/PowerShell/SqlServerDsc/blob/32927e87fe044a7ace8c5ad2452e31e83b6e5879/Modules/DscResource.Common/DscResource.Common.psm1#L677

Only if credentials are specified, which suggests to me that Windows user is for impersonation, no? I'm basing Integrated off Database connection strings, where Integrated Security=true;. Seems to be default among SMO and invoke-sqlcmd, so I can assure you, we do agree on this.

https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/connection-string-syntax

We recommend using Windows Authentication (sometimes referred to as integrated security) to connect to data sources that support it. TLDR: Windows Authentication /= Windows User.

Sorry, my bad. 😞 I looked a this code in this function, without all the comments, and the "integrated" part looks okay! I misunderstood that the username was just for the verbose message. 🙂

Modules/DscResource.Common/DscResource.Common.psm1, line 699 at r4 (raw file):

Previously, kungfu71186 wrote…

Hmmm, ok. I can change that back, but when i was testing, it would return an error if it couldn't connect. I could see this being an issue if the Database was offline/online, but we are connecting to a server. Just doing a quick search online yields the same results (using try/catch) to check if connected.

I believe I changed this because I was having issues with the old code and noticed why. Connect doesnt check if it's actually connected and spits out an error. https://github.com/PowerShell/SqlServerDsc/blob/32927e87fe044a7ace8c5ad2452e31e83b6e5879/Modules/DscResource.Common/DscResource.Common.psm1#L692

I couldn't connect and it wasn't working properly from what i remember. I can test again though.

Please see #333. This might have changed. If there is a way to resolve it without checking status, I'm all for it.

kungfoome

Reviewable status: 2 of 4 files reviewed, 7 unresolved discussions (waiting on @johlju)

Modules/DscResource.Common/DscResource.Common.psm1, line 699 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please see #333. This might have changed. If there is a way to resolve it without checking status, I'm all for it.

Yes that is true. You can actually build out the object first, I believe this is because it builds the connection string before actually trying to connect and then you connect. So yes, the way I have it now should fix that issue as well.

Buttttt..... You can call the object directly and it will try to connect, but if you set it as variable it builds the object first and won't connect until you call connect.

johlju

Reviewable status: 2 of 4 files reviewed, 7 unresolved discussions (waiting on @johlju and @kungfu71186)

Modules/DscResource.Common/DscResource.Common.psm1, line 699 at r4 (raw file):

Previously, kungfu71186 wrote…

Yes that is true. You can actually build out the object first, I believe this is because it builds the connection string before actually trying to connect and then you connect. So yes, the way I have it now should fix that issue as well.

Buttttt..... You can call the object directly and it will try to connect, but if you set it as variable it builds the object first and won't connect until you call connect.

Ah I see! That's good. So then only one questions is if it we should keep that check if it does not throw if it is in any other state that online, see ServerStatus here
https://docs.microsoft.com/en-us/dotnet/api/microsoft.sqlserver.management.smo.serverstatus?view=sql-smo-140.17283.0
What do you think? Just double-checking to make sure we don't regress anything. 🙂

kungfoome · 2019-05-13T19:27:38Z

@johlju I just did some quick research on this. There is one problem. I need to investigate a few things, but one thing I do know, is that connect should NOT be used without a disconnect.

There may be a few options here, but I'll know more after I investigate a few things. But this is good, because now I know connect shouldn't have been used without a disconnect, otherwise it leaves a connection open.

johlju · 2019-05-13T19:33:45Z

Hmm. I thought it closed when the PS session ended? Although, not sure how long the LCM keep the same session. Does the connection stay open even when a session closes? Then it’s not good. That can have a bad impact. :/

kungfoome · 2019-05-13T20:04:36Z

@johlju I just ran some tests. Connect is the safer option. You can disconnect and pass the object around like normal. It will run in 'Server' mode after that. I'll check the sessions, but it should be fine to disconnect with no impact. Just verified, closing the powershell session closes the sql session too.

https://docs.microsoft.com/en-us/sql/relational-databases/server-management-objects-smo/create-program/connecting-to-an-instance-of-sql-server?view=sql-server-2017

You can create an instance of the Server object and establish a connection to the instance of SQL Server in three ways. The first is using a ServerConnection object variable to provide the connection information. The second is to provide the connection information by explicitly setting the Server object properties. The third is to pass the name of the SQL Server instance in the Server object constructor.

https://docs.microsoft.com/en-us/sql/relational-databases/server-management-objects-smo/create-program/disconnecting-from-an-instance-of-sql-server?view=sql-server-2017

When the Connect method is called, the connection is not automatically released. The Disconnect method must be called explicitly to release the connection to the connection pool. Also, you can request a non-pooled connection. You do this by setting the NonPooledConnection property of the ConnectionContext property that references the ServerConnection object.

It's good for when you need to run a bunch of things in the same session and then close it.

I think the best thing is try catch with finally to close the session. This will verify that you can actually connect. Checking status basically does the same thing as well, but returns it's status. Depending on the status of the server though, you may not be able to run queries, but you can connect.

No matter what the situation is, you need a try catch and then check the status in there if you wanted, but i feel like it's not needed.

kungfoome

Reviewable status: 2 of 4 files reviewed, 7 unresolved discussions (waiting on @johlju)

Modules/DscResource.Common/DscResource.Common.psm1, line 1430 at r3 (raw file):

Previously, kungfu71186 wrote…

Ahh, yes that would work. I forgot to put back mandatory on SQLServer. I'm with you now.

Just need your follow-up on this. I changed it to make it more consistent with Connect-SQL. Let me know if you want them to be different or change them both to make them the same.

Modules/DscResource.Common/DscResource.Common.psm1, line 1441 at r3 (raw file):

Previously, kungfu71186 wrote…

Maybe I undid something, I know I took this out. Probably the same thing that happened above.

Got it now. Done.

Modules/DscResource.Common/DscResource.Common.psm1, line 666 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Sorry, my bad. 😞 I looked a this code in this function, without all the comments, and the "integrated" part looks okay! I misunderstood that the username was just for the verbose message. 🙂

No worries. Added some comments to clarify in code.

Modules/DscResource.Common/DscResource.Common.psm1, line 675 at r4 (raw file):

Previously, kungfu71186 wrote…

Yes, that was my intention

Done.

Modules/DscResource.Common/DscResource.Common.psm1, line 699 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ah I see! That's good. So then only one questions is if it we should keep that check if it does not throw if it is in any other state that online, see ServerStatus here
https://docs.microsoft.com/en-us/dotnet/api/microsoft.sqlserver.management.smo.serverstatus?view=sql-smo-140.17283.0
What do you think? Just double-checking to make sure we don't regress anything. 🙂

After some investigation, i think connect is the better option, but you could use status if wanted to know if it's truly online, but if I can connect even if it's pending or offline, im still connected to the SQL database, so it just depends on how you look at this. Done.

sets to invoke-query

johlju

Reviewed 1 of 3 files at r4, 2 of 2 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

Modules/DscResource.Common/DscResource.Common.psm1, line 1430 at r3 (raw file):

Previously, kungfu71186 wrote…

Just need your follow-up on this. I changed it to make it more consistent with Connect-SQL. Let me know if you want them to be different or change them both to make them the same.

Looks good, let's leave it as-is.

Modules/DscResource.Common/DscResource.Common.psm1, line 699 at r4 (raw file):

Previously, kungfu71186 wrote…

After some investigation, i think connect is the better option, but you could use status if wanted to know if it's truly online, but if I can connect even if it's pending or offline, im still connected to the SQL database, so it just depends on how you look at this. Done.

Let leave it as-is.

The problem I think was that it returned to early (to fast) when the instance was not ready to accept connections. So the resource(s) started to fail on issue that was related to not connection. This was a test to determine that it was actually online before continuing. But with this change that might not be an issue anymore. Let's try it this way.

johlju · 2019-05-28T08:55:28Z

Look's good, I just need a rebase so you can move the change log entries back under the unreleased section after the recent release. 🙂

… commonfix

johlju · 2019-06-01T09:40:37Z

@kungfu71186 Could you please rebase this one again?

johlju · 2019-06-23T13:32:26Z

@kungfu71186 I'm happy to extend the Invoke-Query with the suggested changes in this PR. The Connect-SQL changes are already merged through another PR (and I credited you for those changes). Either rebase this PR or send in a new PR with those changes. 🙂

kungfoome · 2019-06-23T14:59:06Z

@johlju I'll probably do another PR. Be easier. Hopefully get to it in the morning.

kungfoome added 5 commits May 3, 2019 19:30

Updated Connect-SQL And Invoke-Query

66c90ee

Cleaned up invokequery tests cases

6886dae

fixed a pester issue where it was trying to typecast

5db1924

merge with dev

2ba1eb7

Fixed parameters and a few styling issues

b122ddd

Updated changelog with better clarity

2d28036

johlju added the needs review The pull request needs a code review. label May 3, 2019

kungfoome added 2 commits May 3, 2019 21:51

Added Verbose back in to connect due to bug

e03e75d

added tests using SMO object. Code coverage 100%

85f2d10

johlju requested changes May 12, 2019

View reviewed changes

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels May 12, 2019

kungfoome commented May 12, 2019

View reviewed changes

johlju requested changes May 12, 2019

View reviewed changes

Fixed connect-sql tests and updated connectsql and

a1e1d27

invoke-query with integrated parameter

kungfoome commented May 12, 2019

View reviewed changes

johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels May 13, 2019

johlju requested changes May 13, 2019

View reviewed changes

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels May 13, 2019

kungfoome commented May 13, 2019

View reviewed changes

johlju requested changes May 13, 2019

View reviewed changes

kungfoome commented May 13, 2019

View reviewed changes

johlju requested changes May 13, 2019

View reviewed changes

kungfoome commented May 13, 2019

View reviewed changes

fixed connect-sql to disconnect and added parameter

6e2c686

sets to invoke-query

johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels May 19, 2019

johlju approved these changes May 28, 2019

View reviewed changes

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels May 28, 2019

Matthew Martinez added 2 commits May 28, 2019 13:44

Merge branch 'dev' of https://github.com/PowerShell/SqlServerDsc into…

c39e449

… commonfix

Fixed changelog

da5d3af

johlju mentioned this pull request Jun 18, 2019

SqlServerDsc.Common: Updates Connect-SQL #1379

Merged

9 tasks

kungfoome mentioned this pull request Jun 24, 2019

Invoke-query updated to allow smo object #1380

Merged

9 tasks

johlju added closed by author The issue or pull request was closed by the author. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jun 25, 2019

johlju closed this in #1380 Jun 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common Module - Improve Connect-SQL and Invoke-Query #1356

Common Module - Improve Connect-SQL and Invoke-Query #1356

kungfoome commented May 3, 2019 •

edited

Loading

kungfoome commented May 3, 2019

johlju commented May 3, 2019

codecov-io commented May 3, 2019 •

edited

Loading

kungfoome commented May 4, 2019

kungfoome commented May 4, 2019

johlju left a comment

kungfoome left a comment

kungfoome left a comment

kungfoome commented May 12, 2019 •

edited

Loading

johlju left a comment

johlju commented May 12, 2019

kungfoome commented May 12, 2019

kungfoome left a comment

johlju left a comment

kungfoome left a comment

kungfoome commented May 13, 2019 •

edited

Loading

johlju left a comment

kungfoome left a comment

johlju left a comment

kungfoome commented May 13, 2019

johlju commented May 13, 2019

kungfoome commented May 13, 2019

kungfoome left a comment

johlju left a comment

johlju commented May 28, 2019

johlju commented Jun 1, 2019

johlju commented Jun 23, 2019

kungfoome commented Jun 23, 2019

Common Module - Improve Connect-SQL and Invoke-Query #1356

Common Module - Improve Connect-SQL and Invoke-Query #1356

Conversation

kungfoome commented May 3, 2019 • edited Loading

Task list

kungfoome commented May 3, 2019

johlju commented May 3, 2019

codecov-io commented May 3, 2019 • edited Loading

Codecov Report

kungfoome commented May 4, 2019

kungfoome commented May 4, 2019

johlju left a comment

Choose a reason for hiding this comment

kungfoome left a comment

Choose a reason for hiding this comment

kungfoome left a comment

Choose a reason for hiding this comment

kungfoome commented May 12, 2019 • edited Loading

johlju left a comment

Choose a reason for hiding this comment

johlju commented May 12, 2019

kungfoome commented May 12, 2019

kungfoome left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

kungfoome left a comment

Choose a reason for hiding this comment

kungfoome commented May 13, 2019 • edited Loading

johlju left a comment

Choose a reason for hiding this comment

kungfoome left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

kungfoome commented May 13, 2019

johlju commented May 13, 2019

kungfoome commented May 13, 2019

kungfoome left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented May 28, 2019

johlju commented Jun 1, 2019

johlju commented Jun 23, 2019

kungfoome commented Jun 23, 2019

kungfoome commented May 3, 2019 •

edited

Loading

codecov-io commented May 3, 2019 •

edited

Loading

kungfoome commented May 12, 2019 •

edited

Loading

kungfoome commented May 13, 2019 •

edited

Loading