Open johlju opened 1 year ago
I like the idea of combing into a single resource. I do have two comments after reading this
DisableVariable
parameter? I currently rely on the ability to disable them to be able to add some of the solutions provided by the SQL Server Maintenance Solution via SqlScript
resources. I promise it's not just me being biased since I added the support for it. š ConnectTimeout
property here and to Connect-SQL
function instead of reusing the StatementTimeout
? It might not be necessary, but it is probably better to control the two independently. Waiting to connect to an offline server for 10 minutes instead of 30 seconds might not be the end of the world but at least that can be up the author of their config.Is there a reason we'd drop the use of the DisableVariable parameter?
My thought that the parameter were not needed if we move away from using Invoke-SqlCmd
and instead using ExecuteNonQuery() and ExecuteWithResults methods on the SMO database object. Not even sure if we need the parameter Variable
as we could use PowerShell to build the query strings, my thought was to leave out the Variable
parameter in a first iteration until there is a god use case for it.
should we consider adding in the ConnectTimeout property here and to Connect-SQL function instead of reusing the StatementTimeout?
I agree that it is probably better to control the two independently. We should also move the logic in Connect-Sql
to the public function Connect-SqlDscDatabaseEngine
so we could make it better then at the same time. š¤
My thought that the parameter were not needed if we move away from using Invoke-SqlCmd and instead using ExecuteNonQuery() and ExecuteWithResults methods
Okay, this should be straight forward just execute the string.
Not even sure if we need the parameter
Variable
as we could use PowerShell to build the query strings
Not quite sure I follow what this would look like with the proposed resource. I use the variable feature as often as possible to simplify and reuse scripts across multiple resources. Here is a simple example
Existing DSC Resource Example
SqlScript 'UpdateSchedule1'
{
ServerName = $serverName
InstanceName = $sqlInstanceName
GetFilePath = "$PSScriptRoot\updateJobStartTime\getScript.sql"
SetFilePath = "$PSScriptRoot\updateJobStartTime\setScript.sql"
TestFilePath = "$PSScriptRoot\updateJobStartTime\testScript.sql"
Variable = "jobName='My Job Step 1'",
("activeStartTime={0}" -f $localStartTime1)
PsDscRunAsCredential = $credential
}
SqlScript 'UpdateSchedule2'
{
ServerName = $serverName
InstanceName = $sqlInstanceName
GetFilePath = "$PSScriptRoot\updateJobStartTime\getScript.sql"
SetFilePath = "$PSScriptRoot\updateJobStartTime\setScript.sql"
TestFilePath = "$PSScriptRoot\updateJobStartTime\testScript.sql"
Variable = "jobName='My Job Step 2'",
("activeStartTime={0}" -f $localStartTime2)
PsDscRunAsCredential = $credential
}
T-SQL getScript.sql from above file
EXEC dbo.sp_update_schedule
@name = $(jobName),
@active_start_time = $(activestartTime)
GO
I realize there are a few ways to handle substitution or string building in PowerShell natively, but in the context of the proposed resource I'm not sure what that looks like. Is the expectation that you'd just handle that outside the resource completely?
I agree that it is probably better to control the two independently. We should also move the logic in
Connect-Sql
to the public functionConnect-SqlDscDatabaseEngine
so we could make it better then at the same time. š¤
I wasn't sure if it would be best to lump it in with this proposal, but it is related and might be as good a time as any other to improve both.
I realize there are a few ways to handle substitution or string building in PowerShell natively, but in the context of the proposed resource I'm not sure what that looks like. Is the expectation that you'd just handle that outside the resource completely?
Ah, for files then the Variable would be a necessity, then we can't parse the strings in the configuration that we can do for the string that is passed to the suggested parameter GetQuery
.
I guess we then need parameter Variable
. If parameter variable is assigned a value then we can run logic that parses the query that is passed in the parameter GetQuery
or parse the content of the files that is passed in the parameter GetQueryFilePath
. I'm thinking that Variable
is a hashtable (or mor exactly an instance of MSFT_KeyValuePair
) instead of an string array for better handling, similar to xRemoteFile.
So in the example would instead look like this:
SqlQuery 'UpdateSchedule2'
{
ServerName = $serverName
InstanceName = $sqlInstanceName
GetQueryFilePath = "$PSScriptRoot\updateJobStartTime\getScript.sql"
SetQueryFilePath = "$PSScriptRoot\updateJobStartTime\setScript.sql"
TestQueryFilePath = "$PSScriptRoot\updateJobStartTime\testScript.sql"
Variable = @{
jobName = 'My Job Step 2'
activeStartTime = '{0}' -f $localStartTime2
}
PsDscRunAsCredential = $credential
}
Then, since the parameter Variable
was passed I'm thinking it would parse the content the file, e.g in the example it would loop through the variables (the keys) in the hashtable and look for $(jobName)
and replace it.
EXEC dbo.sp_update_schedule
@name = $(jobName),
@active_start_time = $(activestartTime)
GO
to
EXEC dbo.sp_update_schedule
@name = 'My Job Step 2',
@active_start_time = '2023-04-26 09:00:00'
GO
Would that work?
Would it be a problem parsing some of the variables depending on the data type or can all be made strings so they are converted automatically by SQL Server? š¤ I feel it was a long time since I manipulated data types in T-SQL š
Resource proposal
Suggest adding a new class-based resource SqlQuery that will deprecate SqlScript and SqlScriptQuery.
Proposed properties
'MSSQLSERVER'
.Get
was called.Special considerations or limitations
As mentioned in the comment https://github.com/dsccommunity/SqlServerDsc/pull/1904#issuecomment-1518551360 it would help using different modules that provides the SMO assemblies if this module is not dependent on a specific command from a specific module (that also is dependent on SMO assemblies).
The new resource should use the command
Invoke-SqlDscQuery
. ForGet()
andTest
it will pass thePassThru
parameter to return the expected values. ForSet()
it will not pass the parameterPassThru
since it will just execute and do not return any values.If possible the T-SQL statement
PRINT
should be outputted as verbose messages (similar to whatInvoke-SqlCmd
does). This might require changes toInvoke-SqlDscQuery
.It should not be possible to specify both a query string and a query file for the same method, e.g.
GetQuery
andGetQueryFilePath
. But it should be possible to specifyGetQuery
andTestQueryFilePath
.It should throw an exception if a method (
Get(),
Set(), or
Test`) is called without having any of the Query parameters assigned.Might be worth skipping the parameter
Variable
in a first iteration of the resource so more feedback can be gathered. š¤