Fix Test-TargetResource crash when SQL instance is unreachable#2452
Fix Test-TargetResource crash when SQL instance is unreachable#2452manuelgr0 wants to merge 18 commits intodsccommunity:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWraps many resources' Get-TargetResource (and some Compare/Set paths) calls inside Test-TargetResource with try/catch; on exception the code writes a localized SQLInstanceNotReachable verbose message and returns $false instead of propagating the exception. Adds SQLInstanceNotReachable strings and updates unit tests to cover the error path. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_SqlAlias/DSC_SqlAlias.psm1`:
- Around line 368-380: Wrap the initial call to Get-TargetResource (the
invocation that assigns $currentValues earlier in the function) in a try/catch
so any exceptions from contacting the SQL instance are caught there, then remove
the redundant second try/catch block that re-calls Get-TargetResource near the
end of the function; ensure the catch logs the localized message
($script:localizedData.SQLInstanceNotReachable -f $_) and returns $false, and
keep the existing return $result logic using the protected first call's results.
In `@source/DSCResources/DSC_SqlAlias/en-US/DSC_SqlAlias.strings.psd1`:
- Line 9: The code references $script:localizedData.ClientAliasMissing in
DSC_SqlAlias.psm1 but that key was removed from DSC_SqlAlias.en-US.strings.psd1;
restore the missing localization key (ClientAliasMissing) in the strings file
with an appropriate message explaining the client alias is missing, or update
the reference in DSC_SqlAlias.psm1 to an existing, semantically correct key (do
not use SQLInstanceNotReachable) and adjust the message text to reflect a
missing alias; ensure the symbol referenced is exactly ClientAliasMissing (or
the chosen existing key) so $script:localizedData.<key> is not null at runtime.
In
`@source/DSCResources/DSC_SqlDatabaseObjectPermission/DSC_SqlDatabaseObjectPermission.psm1`:
- Around line 719-730: The try/catch block that currently wraps
Get-TargetResource inside Compare-TargetResourceState should be removed so
Compare-TargetResourceState always returns the property-state objects (not a
boolean); instead, move the try/catch into Test-TargetResource (and similarly
into Set-TargetResource if needed) so any error calling
Compare-TargetResourceState results in Test-TargetResource returning $false;
also eliminate the backtick line continuation in the Write-Verbose call (use a
single string or string concatenation) and keep error logging consistent with
existing patterns in Compare-TargetResourceState/Test-TargetResource.
In `@source/DSCResources/DSC_SqlDatabaseUser/DSC_SqlDatabaseUser.psm1`:
- Around line 220-231: Remove the try/catch block (including the backtick
continuation and the "return $false") from Set-TargetResource so it simply calls
Get-TargetResource unprotected; then add a proper try/catch around the
Get-TargetResource call inside Test-TargetResource (wrap the same
Get-TargetResource invocation there), log the localized SQLInstanceNotReachable
message without using backtick continuation, and have the catch return $false
from Test-TargetResource (leaving Set-TargetResource void and not returning any
value).
In `@source/DSCResources/DSC_SqlEndpoint/DSC_SqlEndpoint.psm1`:
- Around line 225-236: Remove the try/catch around the Get-TargetResource call
inside Set-TargetResource so errors propagate instead of returning $false;
specifically eliminate the catch block (and its Write-Verbose/return $false)
that wraps Get-TargetResource, leaving the call to Get-TargetResource and the
subsequent Connect-SQL to surface their errors, and ensure Set-TargetResource
remains void (do not return any value).
In `@source/DSCResources/DSC_SqlProtocol/DSC_SqlProtocol.psm1`:
- Around line 638-649: Revert Compare-TargetResourceState so it always returns
the array of property-state objects (the outputs from
Compare-ResourcePropertyState) and remove the try/catch block that swallows
errors and returns $false; instead, move a try/catch into Test-TargetResource
around the call to Compare-TargetResourceState (the invocation currently
surrounded at Line ~466), catch the same exception, log using Write-Verbose with
$script:localizedData.SQLInstanceNotReachable -f $_ (as the original catch did)
and return $false from Test-TargetResource so callers get a boolean failure
while Compare-TargetResourceState preserves its contract.
In `@source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1`:
- Around line 694-705: Remove the try/catch from Compare-TargetResourceState so
it always returns the expected array of property-state objects (do not return
$false or swallow exceptions in Compare-TargetResourceState); instead, wrap the
call to Compare-TargetResourceState inside Test-TargetResource in a try/catch
around the invocation of Compare-TargetResourceState (the one using
`@PSBoundParameters`), and in that catch log the localized message
($script:localizedData.SQLInstanceNotReachable -f $_) and return $false from
Test-TargetResource so Test-TargetResource correctly reports the resource as not
in desired state when the SQL instance is unreachable.
In `@source/DSCResources/DSC_SqlSecureConnection/DSC_SqlSecureConnection.psm1`:
- Around line 265-276: The try/catch that wraps Get-TargetResource is in the
wrong function—remove the entire try{ $encryptionState = Get-TargetResource
`@parameters` } catch{ ... return $false } block from Set-TargetResource so
Set-TargetResource does not return a value, then add the try/catch around the
Get-TargetResource call inside Test-TargetResource (around the existing
Get-TargetResource invocation) so that on connection errors Test-TargetResource
logs the localized message and returns $false; ensure Test-TargetResource still
returns $true/$false as per DSC guidelines and Set-TargetResource remains void.
In `@source/DSCResources/DSC_SqlSetup/DSC_SqlSetup.psm1`:
- Around line 1033-1044: In Set-TargetResource, remove the surrounding try/catch
that calls Get-TargetResource (the block catching all exceptions and doing
Write-Verbose then return $false); Set-TargetResource must not return a value or
silently swallow failures — let exceptions from Get-TargetResource propagate as
terminating errors so DSC reports failure, or if you need to add handling,
replace the catch with throwing a terminating error that includes $_; do the
change in the Set-TargetResource function and leave any special-case error
handling to Test-TargetResource instead.
In `@source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1`:
- Around line 226-237: In Set-TargetResource, remove the void-return and
prohibited backtick: do not return $false from the catch after calling
Get-TargetResource; instead either remove the try/catch so the error will
naturally propagate, or log the verbose message using
$script:localizedData.SQLInstanceNotReachable and then re-throw the caught
exception (use throw) so DSC sees the failure; also replace the
line-continuation backtick with normal string concatenation or a single-line
Write-Verbose call to avoid using ` as continuation.
In `@source/DSCResources/DSC_SqlWaitForAG/DSC_SqlWaitForAG.psm1`:
- Around line 192-203: The catch block inside Set-TargetResource around the
Get-TargetResource call should not return $false (which makes Set silently
exit); instead let the error propagate or re-throw so DSC reports the failure.
In practice remove the "return $false" from the catch and replace the swallow
with a "throw" (or simply remove the try/catch) so that exceptions from
Get-TargetResource (and the localizedData.SQLInstanceNotReachable message)
bubble up; update the catch to call Write-Verbose then throw the original
exception ($_).
In `@source/DSCResources/DSC_SqlWindowsFirewall/DSC_SqlWindowsFirewall.psm1`:
- Around line 430-441: Remove the whole try/catch wrapper around the
Get-TargetResource call in Set-TargetResource: call Get-TargetResource
-SourcePath $SourcePath -Features $Features -InstanceName $InstanceName directly
(no try/catch), delete the Write-Verbose that references
$script:localizedData.SQLInstanceNotReachable and remove the return $false so
Set-TargetResource remains void; let any legitimate exceptions propagate or be
handled elsewhere since this resource inspects services/registry rather than
performing SQL connectivity.
🟠 Major comments (9)
source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1-453-464 (1)
453-464: 🛠️ Refactor suggestion | 🟠 MajorTry/catch with
return $falseinTest-TargetResourceis correct, but avoid backtick line continuation.The error-handling pattern here aligns with the PR objective — catching connection failures and returning
$falsesoSet-TargetResourcecan remediate. However, line 460 uses a backtick for line continuation, which is prohibited.Proposed fix: remove backtick
Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ + $script:localizedData.SQLInstanceNotReachable -f $_ )As per coding guidelines: "Never use backtick as line continuation in production code."
source/DSCResources/DSC_SqlReplication/DSC_SqlReplication.psm1-406-413 (1)
406-413:⚠️ Potential issue | 🟠 MajorOverly broad catch may mask bugs in
Get-TargetResource.This catches every exception — not just SQL connectivity errors. If
Get-TargetResourcefails due to a coding bug, parameter binding issue, or unexpected runtime error, it will be silently swallowed and$falsereturned. This makes debugging extremely difficult since the error is only emitted as a verbose message.Consider narrowing the catch to connection-related exceptions, or at minimum re-evaluate the severity by using
Write-Warninginstead ofWrite-Verboseso operators have visibility into unexpected failures.source/DSCResources/DSC_SqlDatabaseRole/DSC_SqlDatabaseRole.psm1-474-485 (1)
474-485:⚠️ Potential issue | 🟠 MajorBacktick line continuation and overly broad catch.
Backtick: Line 481 uses a backtick for line continuation, which is prohibited by coding guidelines. Use parentheses or a different formatting approach instead.
Overly broad catch: This catches all exceptions from
Get-TargetResource, not just SQL connection errors. A parameter validation error, aDatabaseNotFoundexception, or any other bug would be silently swallowed and return$false, masking real issues. Consider narrowing the catch or at minimum logging at a higher severity (e.g.,Write-Warning) so operators can distinguish connectivity failures from other errors.As per coding guidelines: "Never use backtick as line continuation in production code."
Proposed fix for backtick (minimal)
try { $getTargetResourceResult = Get-TargetResource `@getTargetResourceParameters` } catch { - Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ - ) + Write-Verbose -Message ( + $script:localizedData.SQLInstanceNotReachable -f $_ + ) + return $false }source/DSCResources/DSC_SqlServiceAccount/DSC_SqlServiceAccount.psm1-173-184 (1)
173-184:⚠️ Potential issue | 🟠 MajorMisleading error message and incorrect catch scope for this resource.
Get-TargetResourcein this resource does not callConnect-SQL. It uses WMI viaGet-ServiceObject(line 71). TheSQLInstanceNotReachablemessage ("Unable to connect to SQL instance…") is factually incorrect here and will mislead operators.More critically,
Get-TargetResourcedeliberately throwsNew-ObjectNotFoundException(line 77) when the service isn't found. Catching that and returning$falsewill causeSet-TargetResourceto run, which will also fail with the sameObjectNotFoundException(line 264) — just with a less helpful error path.This resource should either:
- Not have this catch at all (if the service doesn't exist, there's nothing
Set-TargetResourcecan remediate for service accounts), or- Catch only WMI connectivity exceptions specifically, with an appropriate message.
source/DSCResources/DSC_SqlDatabaseMail/DSC_SqlDatabaseMail.psm1-754-765 (1)
754-765:⚠️ Potential issue | 🟠 MajorOverly broad catch — consider narrowing to connection-related exceptions only.
The bare
catchswallows all exceptions fromGet-TargetResource, not just SQL connectivity failures. IfGet-TargetResourcethrows due to a programming bug (e.g., null reference, parameter binding error), it will be silently logged as "Unable to connect to SQL instance" andTest-TargetResourcewill return$false, masking the real issue and unnecessarily triggeringSet-TargetResource.The linked issue
#2451specifically asks to catch connection errors. Consider filtering on the exception type or message (e.g., checking forSQLCOMMON0019or theConnect-SQL-originated exception) to avoid masking unrelated failures.source/DSCResources/DSC_SqlEndpointPermission/DSC_SqlEndpointPermission.psm1-165-176 (1)
165-176:⚠️ Potential issue | 🟠 Major
Set-TargetResourcemust not return a value —return $falseviolates the MOF resource contract.Per the DSC MOF resource guidelines,
Set-TargetResourcemust be void. Returning$falsehere will not be consumed by the DSC engine (it ignores Set return values), and it signals incorrect intent. IfGet-TargetResourcethrows because the instance is unreachable,Set-TargetResourceshould either let the error propagate (so DSC retries) or log a warning/error andreturnwithout a value.Also, backtick line continuation on lines 172–173 violates the coding guideline: "Never use backtick as line continuation in production code." Use parentheses or a variable instead.
Proposed fix
try { $getTargetResourceResult = Get-TargetResource `@parameters` } catch { - Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ - ) - return $false + $errorMessage = $script:localizedData.SQLInstanceNotReachable -f $_ + + New-InvalidOperationException -Message $errorMessage -ErrorRecord $_ }As per coding guidelines: "
Set-TargetResourcefunction must not return anything (void)" and "Never use backtick as line continuation in production code." Also based on learnings: "UseNew-InvalidOperationExceptionfor invalid operation errors instead ofthrowin MOF-based DSC resources."source/DSCResources/DSC_SqlMaxDop/DSC_SqlMaxDop.psm1-264-275 (1)
264-275: 🛠️ Refactor suggestion | 🟠 MajorGood addition of try/catch for graceful error handling.
The wrapping of
Get-TargetResourcein a try/catch to handle connectivity failures is the right approach for this resource.However, Line 271 uses a backtick for line continuation, which violates the coding guidelines.
Remove backtick line continuation
catch { Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ + $script:localizedData.SQLInstanceNotReachable -f $_ ) return $false }As per coding guidelines: "Never use backtick as line continuation in production code."
source/DSCResources/DSC_SqlScriptQuery/DSC_SqlScriptQuery.psm1-450-456 (1)
450-456:⚠️ Potential issue | 🟠 MajorBare
catchblock catches all exceptions, not just SQL connectivity failures — revert to exception-specific handling.The main branch catches
[Microsoft.SqlServer.Management.PowerShell.SqlPowerShellSqlExecutionException]specifically. This PR changed it to a genericcatchthat swallows all exceptions fromInvoke-SqlScript— including T-SQL syntax errors, permission failures, and timeouts — all logged as "SQLInstanceNotReachable". This masks real errors and provides misleading diagnostics.Revert to catching the specific exception type, or if the generic catch is intentional for a different reason, use a more accurate verbose message that reflects the actual error (e.g., include
$_.Exception.GetType().Namein the log output).source/DSCResources/DSC_SqlSetup/DSC_SqlSetup.psm1-2324-2335 (1)
2324-2335:⚠️ Potential issue | 🟠 MajorOverly broad catch — any
Get-TargetResourcefailure is treated as "SQL instance unreachable".
Get-TargetResourceinDSC_SqlSetupdoes significantly more than connect to SQL: it resolves UNC paths, readssetup.exefile versions, queries the registry, and enumerates services. Catching all exceptions and returning$falsemeans infrastructure failures (e.g., missing install media, credential errors, registry access denied) are misdiagnosed as "SQL instance unreachable" and trigger a full re-install viaSet-TargetResource.Consider narrowing the catch to SQL connectivity errors only, or at minimum validating the exception type/message before returning
$false.
🟡 Minor comments (6)
source/DSCResources/DSC_SqlReplication/DSC_SqlReplication.psm1-402-413 (1)
402-413:⚠️ Potential issue | 🟡 MinorBacktick line continuation violates coding guidelines.
Lines 409-410 use a backtick for line continuation. The guidelines explicitly prohibit this in production code. Restructure to avoid the backtick.
As per coding guidelines: "Never use backtick as line continuation in production code."
Proposed fix
catch { Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ + $script:localizedData.SQLInstanceNotReachable -f $_ ) return $false }source/DSCResources/DSC_SqlDatabaseObjectPermission/en-US/DSC_SqlDatabaseObjectPermission.strings.psd1-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorMissing error code and inaccurate message text.
Two issues with this new localized string:
Missing error code: Every other string in this file includes an error code suffix (e.g.,
(SDOP0001)). This string should follow the same convention for consistency and supportability.Inaccurate message text: The phrase "retrieve option" doesn't match this resource's purpose (database object permissions). This looks like a copy-paste from another resource. Consider something like:
"Unable to connect to the SQL instance '{0}\\{1}'. Assuming the resource is not in the desired state. Error: {2}"
— which would also benefit from accepting the server/instance name for diagnostics.Proposed fix
- SQLInstanceNotReachable = Unable to connect to SQL instance or retrieve option. Assuming resource is not in desired state. Error: {0} + SQLInstanceNotReachable = Unable to connect to the SQL instance. Assuming the resource is not in the desired state. Error: {0} (SDOP0013)source/DSCResources/DSC_SqlDatabaseRole/en-US/DSC_SqlDatabaseRole.strings.psd1-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorFix string key naming and message text.
Two issues:
Naming convention: The key
SQLInstanceNotReachableshould use underscores as word separators per the localization guidelines, e.g.,SQL_Instance_Not_Reachable. Note: The guideline for DSC resource strings specifies underscore separators for multi-word keys, not theVerb_FunctionName_Actionpattern.Generic error message: The message phrase "retrieve option" appears identically across all 32 resource string files and is misleading for
DSC_SqlDatabaseRole(which doesn't deal with configuration options). It should be changed to "Unable to connect to SQL instance. Assuming resource is not in desired state. Error: {0}".Proposed fix
- SQLInstanceNotReachable = Unable to connect to SQL instance or retrieve option. Assuming resource is not in desired state. Error: {0} + SQL_Instance_Not_Reachable = Unable to connect to SQL instance. Assuming resource is not in desired state. Error: {0}source/DSCResources/DSC_SqlScript/DSC_SqlScript.psm1-472-476 (1)
472-476:⚠️ Potential issue | 🟡 MinorBare catch block conflates connection errors with all other exceptions.
The catch block around
Invoke-SqlScriptcatches all exceptions but reports them asSQLInstanceNotReachable. This is misleading when the actual error is:
- A T-SQL syntax error in the test script
- A query timeout
- A permission error unrelated to connectivity
While the current pattern matches
DSC_SqlScriptQueryand complies with guidelines (not an empty catch block), it frames unrelated errors as connection failures, making troubleshooting harder. The test function will still return$false(correct behavior), but the verbose message will be inaccurate.Consider narrowing the catch to only connection-related exceptions:
Suggested approach
catch { # Only treat connection/access errors as "not reachable" if ($_.Exception -match 'connection|login|network|firewall|timeout' -or $_.Exception.InnerException -match 'SqlException') { Write-Verbose -Message ($script:localizedData.SQLInstanceNotReachable -f $_) return $false } # Re-throw other errors for visibility throw }Alternatively, update the localized string to reflect broader failure handling (e.g., "Unable to execute test script...") if the intent is to catch all exceptions.
source/DSCResources/DSC_SqlProtocol/en-US/DSC_SqlProtocol.strings.psd1-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorMissing error code suffix.
All other strings in this file include an error code suffix (e.g.,
(SSP0001)). The newSQLInstanceNotReachableentry omits one. Consider adding a code for consistency with this resource's conventions, which aids log filtering and troubleshooting.source/DSCResources/DSC_SqlConfiguration/DSC_SqlConfiguration.psm1-281-284 (1)
281-284:⚠️ Potential issue | 🟡 MinorAvoid backtick line continuation in production code.
Lines 282–283 use a backtick (
`) for line continuation. This applies to all the new catch blocks across the PR. As per coding guidelines: "Never use backtick as line continuation in production code." Use parentheses or splatting instead.Suggested fix
catch { Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ + $script:localizedData.SQLInstanceNotReachable -f $_ ) + $result = $false }
🧹 Nitpick comments (11)
source/DSCResources/DSC_SqlSecureConnection/en-US/DSC_SqlSecureConnection.strings.psd1 (1)
7-7: Guideline requires underscores as word separators in localized string key names.The coding guidelines specify using underscores for multi-word keys in
.strings.psd1files (e.g.,SQL_Instance_Not_Reachable). However, every existing key in this file (e.g.,GetEncryptionSettings,CertificateSettings) also uses PascalCase without underscores, so the new key is at least consistent with the current file convention.If you do decide to adopt the underscore convention, this should be done as a separate, file-wide effort rather than for just this one key.
As per coding guidelines: "In
.strings.psd1files, use underscores as word separators in localized string key names for multi-word keys."source/DSCResources/DSC_SqlReplication/en-US/DSC_SqlReplication.strings.psd1 (1)
3-3: Localized string key naming: underscores as word separators.The new key
SQLInstanceNotReachabledoesn't use underscores between words (e.g.,SQL_Instance_Not_Reachable). That said, every existing key in this file (GetCurrentState,DistributorMode, etc.) also omits underscores, so this is consistent with the current file's pattern. Worth aligning if the convention is ever enforced across the board.Also, minor grammar nit: "retrieve option" → "retrieve options" (plural).
As per coding guidelines: "In
.strings.psd1files, use underscores as word separators in localized string key names for multi-word keys."source/DSCResources/DSC_SqlRS/DSC_SqlRS.psm1 (1)
773-784: Avoid backtick line continuation.The coding guidelines prohibit backtick (
`) as a line continuation character in production code. Restructure the-fexpression to avoid it.♻️ Suggested refactor
try { $currentConfig = Get-TargetResource `@getTargetResourceParameters` } catch { - Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ - ) + Write-Verbose -Message ( + $script:localizedData.SQLInstanceNotReachable -f $_ + ) return $false }As per coding guidelines: "Never use backtick as line continuation in production code."
source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1 (1)
400-411: Avoid backtick line continuation.Same guideline issue as in DSC_SqlRS — backtick is prohibited in production code. The
-foperator fits on a single line here.♻️ Suggested refactor
try { $getTargetResourceResult = Get-TargetResource `@getTargetResourceParameters` } catch { - Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ - ) + Write-Verbose -Message ( + $script:localizedData.SQLInstanceNotReachable -f $_ + ) return $false }As per coding guidelines: "Never use backtick as line continuation in production code."
source/DSCResources/DSC_SqlEndpointPermission/DSC_SqlEndpointPermission.psm1 (1)
292-303:Test-TargetResourcecatch block — correct intent, but fix the backtick line continuation.Returning
$falsefromTest-TargetResourceon connection failure is the right behavior per the PR objective. However, backtick line continuation on lines 299–300 violates the coding guideline.Proposed fix — remove backtick continuation
catch { - Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ - ) + Write-Verbose -Message ( + $script:localizedData.SQLInstanceNotReachable -f $_ + ) + return $false }As per coding guidelines: "Never use backtick as line continuation in production code."
source/DSCResources/DSC_SqlAlwaysOnService/DSC_SqlAlwaysOnService.psm1 (1)
244-255: Avoid backtick line continuation.Lines 251–252 use a backtick (
`) for line continuation, which violates the coding guidelines. The expression can be reformatted to avoid the backtick by placing the-foperator on the same line or using parenthetical grouping.This same backtick pattern appears across all the other resources modified in this PR — please fix them all consistently.
♻️ Proposed fix
catch { Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ + $script:localizedData.SQLInstanceNotReachable -f $_ ) return $false }As per coding guidelines: "Never use backtick as line continuation in production code."
source/DSCResources/DSC_SqlConfiguration/DSC_SqlConfiguration.psm1 (1)
253-288: Catch-all swallows non-connectivity exceptions with a misleading message.The
catchblock intercepts every exception fromGet-TargetResource, not just SQL connectivity errors. For example, ifOptionNameis invalid,New-ArgumentExceptionis thrown at Line 77, and the catch here logsSQLInstanceNotReachable— a misleading diagnostic — then returns$false.Set-TargetResourcewill then run and throw the real error, so the impact is limited, but the verbose output will be confusing during troubleshooting.Consider narrowing the catch or at least logging at a level that doesn't mislead:
Suggested approach
catch { Write-Verbose -Message ( - $script:localizedData.SQLInstanceNotReachable ` - -f $_ + $script:localizedData.SQLInstanceNotReachable -f $_ ) + $result = $false }A more precise alternative would catch only the expected connectivity error (e.g., checking
$_.Exceptiontype or message pattern), but that may be impractical given the variety of underlying exceptions across resources.source/DSCResources/DSC_SqlWaitForAG/DSC_SqlWaitForAG.psm1 (1)
303-316: Test-TargetResource catch block — same backtick and catch-all concerns.Same pattern as flagged in
DSC_SqlConfiguration: backtick line continuation (Lines 310–311) and overly broad catch. Thereturn $falseis correct here sinceTest-TargetResourcemust return a boolean.source/DSCResources/DSC_SqlWaitForAG/en-US/DSC_SqlWaitForAG.strings.psd1 (1)
3-3: Message text mentions "retrieve option" — not applicable to this Availability Group resource.The string
"Unable to connect to SQL instance or retrieve option."appears to be copied verbatim across all resources. ForDSC_SqlWaitForAG, the phrase "retrieve option" doesn't make sense — this resource checks for cluster group/AG existence, not configuration options. Consider making the message more generic (e.g.,"Unable to connect to SQL instance or retrieve state.") or tailoring it per resource.source/DSCResources/DSC_SqlProtocolTcpIp/en-US/DSC_SqlProtocolTcpIp.strings.psd1 (1)
3-3: New string lacks an error code suffix, unlike all other entries in this file.Every other key in this file includes a parenthetical error code (e.g.,
(SSPTI0001)). The newSQLInstanceNotReachablestring omits this, which is inconsistent with the file's convention. Consider adding one (e.g.,(SSPTI0016)).source/DSCResources/DSC_SqlDatabaseDefaultLocation/DSC_SqlDatabaseDefaultLocation.psm1 (1)
299-311: Missing blank line after the catch block's closing brace.The closing
}of the catch block on Line 310 is immediately followed by the<#comment on Line 311, without the required blank line separator. Also same backtick issue on Line 306 as noted in other files.🔧 Proposed fix
return $false } + <#
source/DSCResources/DSC_SqlDatabaseObjectPermission/DSC_SqlDatabaseObjectPermission.psm1
Outdated
Show resolved
Hide resolved
source/DSCResources/DSC_SqlDatabaseUser/DSC_SqlDatabaseUser.psm1
Outdated
Show resolved
Hide resolved
source/DSCResources/DSC_SqlSecureConnection/DSC_SqlSecureConnection.psm1
Outdated
Show resolved
Hide resolved
source/DSCResources/DSC_SqlWindowsFirewall/DSC_SqlWindowsFirewall.psm1
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
source/DSCResources/DSC_SqlEndpointPermission/DSC_SqlEndpointPermission.psm1
Fixed
Show fixed
Hide fixed
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Unit/DSC_SqlMaxDop.Tests.ps1 (1)
674-679:⚠️ Potential issue | 🟡 MinorAdd standard function metadata to the Get-CimInstance stub.
The stub function is missing required comment-based help, CmdletBinding, and parameter attributes.
Proposed fix
- function script:Get-CimInstance - { - param - ( - $ClassName - ) - } + <# + .SYNOPSIS + Stub for Get-CimInstance used in unit tests. + + .DESCRIPTION + Provides a stub so Pester can mock Get-CimInstance in module scope for cross-platform tests. + + .PARAMETER ClassName + The CIM class name requested by the test. + + .EXAMPLE + Get-CimInstance -ClassName 'Win32_Processor' + + .INPUTS + None. + + .OUTPUTS + None. + #> + function script:Get-CimInstance + { + [CmdletBinding()] + [OutputType()] + param + ( + [Parameter(Mandatory = $true)] + [System.String] + $ClassName + ) + }As per coding guidelines: “Always add comment-based help to all functions and scripts with SYNOPSIS, DESCRIPTION (40+ chars), PARAMETER, and EXAMPLE sections before function/class”, “Include [CmdletBinding()] on every function”, and “All parameters use [Parameter()] attribute”.
tests/Unit/DSC_SqlTraceFlag.Tests.ps1 (1)
1076-1108:⚠️ Potential issue | 🟡 MinorMove the Test-TargetResource scenarios out of the Set-TargetResource Describe.
These two contexts call
Test-TargetResourcebut live underDSC_SqlTraceFlag\Set-TargetResource, which breaks the test organization rule.✅ Suggested move
@@ - Context 'For a nonexistent instance' { - It 'Should return $false for incorrect parameters' { - InModuleScope -ScriptBlock { - Set-StrictMode -Version 1.0 - - $mockSetTargetResourceParameters.InstanceName = 'INST01' - - $result = Test-TargetResource `@mockSetTargetResourceParameters` - - $result | Should -BeFalse - } - } - } - - Context 'For a nonexistent server' { - BeforeAll { - Mock -CommandName New-Object -MockWith { - return $null - } -ParameterFilter { - $TypeName -eq 'Microsoft.SqlServer.Management.Smo.Wmi.ManagedComputer' - } - } - - It 'Should return $false for incorrect parameters' { - InModuleScope -ScriptBlock { - Set-StrictMode -Version 1.0 - - $mockSetTargetResourceParameters.ServerName = 'FakeServer' - $mockSetTargetResourceParameters.InstanceName = 'INST00' # Instance exist - - $result = Test-TargetResource `@mockSetTargetResourceParameters` - - $result | Should -BeFalse - } - } - } @@ Describe 'DSC_SqlTraceFlag\Test-TargetResource' -Tag 'Test' { + Context 'For a nonexistent instance' { + It 'Should return $false for incorrect parameters' { + InModuleScope -ScriptBlock { + Set-StrictMode -Version 1.0 + + $mockSetTargetResourceParameters.InstanceName = 'INST01' + + $result = Test-TargetResource `@mockSetTargetResourceParameters` + + $result | Should -BeFalse + } + } + } + + Context 'For a nonexistent server' { + BeforeAll { + Mock -CommandName New-Object -MockWith { + return $null + } -ParameterFilter { + $TypeName -eq 'Microsoft.SqlServer.Management.Smo.Wmi.ManagedComputer' + } + } + + It 'Should return $false for incorrect parameters' { + InModuleScope -ScriptBlock { + Set-StrictMode -Version 1.0 + + $mockSetTargetResourceParameters.ServerName = 'FakeServer' + $mockSetTargetResourceParameters.InstanceName = 'INST00' # Instance exist + + $result = Test-TargetResource `@mockSetTargetResourceParameters` + + $result | Should -BeFalse + } + } + }As per coding guidelines, One
Describeblock per file matching the tested entity name.
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 19-31: Wrap the newly added "Fixed" changelog lines so no line
exceeds 80 characters: reflow the long sentence that starts with "Fixed `Test()`
method to return `$false`..." and the block beginning "Fixed
`Test-TargetResource` to return `$false`..." at word boundaries, breaking the
resource list (SqlAG, SqlAGDatabase, ... SqlWindowsFirewall) into multiple lines
or bullet-wrapped lines under 80 characters; ensure references to Test(),
Test-TargetResource, and Get-TargetResource are left intact and unchanged while
only adjusting line breaks to meet the 80-character limit.
In `@source/Classes/020.SqlPermission.ps1`:
- Around line 181-189: The try/catch inside the Test() method is over-indented
(12 spaces) compared to the rest of the class (8 spaces); adjust the indentation
of the try, catch, and their contents to match the method body indentation
standard (4-space levels) so the try/catch block aligns with the other method
code in Test() and conforms to the 4-space rule used across the class (look for
the Test() method and the ([ResourceBase] $this).Test() call and the
Write-Verbose line to re-indent).
In `@source/DSCResources/DSC_SqlAlias/en-US/DSC_SqlAlias.strings.psd1`:
- Around line 9-10: Rename the localization key SQLInstanceNotReachable to use
underscores (e.g., SQL_Instance_Not_Reachable) in the DSC_SqlAlias.en-US
.strings.psd1 entry and update every reference to that key in code that accesses
$script:localizedData (search for SQLInstanceNotReachable and replace with the
new key name); ensure the value string remains unchanged and run a quick
build/loc test to verify no missing-key errors.
In
`@source/DSCResources/DSC_SqlDatabaseObjectPermission/DSC_SqlDatabaseObjectPermission.psm1`:
- Around line 583-593: The verbose message in the Test-TargetResource catch
block uses $script:localizedData.SQLInstanceNotReachable which does not follow
the required "Determining the current state of…" prefix; add a Test-specific
localized string (e.g. SQLInstanceNotReachable_Test) that begins with
"Determining the current state of the target resource: ..." and update the
Test-TargetResource catch to call Write-Verbose with that new localized string
instead of $script:localizedData.SQLInstanceNotReachable (the change should be
made in the catch surrounding the Compare-TargetResourceState call inside
Test-TargetResource).
In `@source/DSCResources/DSC_SqlSecureConnection/DSC_SqlSecureConnection.psm1`:
- Around line 393-407: In the catch block that logs instance reachability,
remove the backtick line-continuation from the Write-Verbose call and combine
the format string and argument on a single call so the message is passed as one
expression (reference: the catch block using Write-Verbose,
$script:localizedData.SQLInstanceNotReachable -f $_ and the subsequent return
$false); ensure the formatted message remains the same but written without any
backtick continuations.
In `@source/DSCResources/DSC_SqlSetup/DSC_SqlSetup.psm1`:
- Around line 2313-2324: The try/catch around the Get-TargetResource call
(producing $getTargetResourceResult) is too broad and hides non-SQL errors;
change the catch to only handle SQL-connection-related exceptions (or inspect
$_.Exception.Id/Type/message to detect connection failures) and re-throw or
return/Write-Error for other exception types so file I/O, UNC, registry, or
credential errors aren't mislabeled as "SQLInstanceNotReachable"; also remove
the backtick line-continuation in the Write-Verbose invocation and format the
message on a single line or with proper string concatenation to comply with
coding guidelines.
In `@tests/Unit/DSC_SqlAGDatabase.Tests.ps1`:
- Around line 2921-2937: Remove the redundant nested InModuleScope -ScriptBlock
inside the It block: rely on the outer InModuleScope already provided by the
test harness, move Set-StrictMode -Version 1.0 directly into the It block
(immediately before calling Test-TargetResource), and invoke Test-TargetResource
`@mockTestTargetResourceParameters` directly (without wrapping it in another
InModuleScope). Ensure mocks for Get-TargetResource remain active and the It
block returns the $result for the assertion.
In `@tests/Unit/DSC_SqlRole.Tests.ps1`:
- Around line 654-661: The failing test doesn't supply required parameters so
Test-TargetResource exits early; update the exception-path test block to include
mandatory parameters by adding explicit ServerRoleName and Ensure entries to the
mockTestParameters used in the InModuleScope script so the code follows the
mocked Get-TargetResource path (i.e., ensure mockTestParameters contains keys
ServerRoleName and Ensure with appropriate values before calling
Test-TargetResource).
- Around line 121-143: The mock ScriptMethod param blocks for the AddMember and
DropMember methods use the short type name [String]; update both parameter
declarations inside the ScriptMethod -Name AddMember and -Name DropMember blocks
to use the full type name [System.String] for the $memberName parameters so they
follow the project's coding guidelines.
In `@tests/Unit/DSC_SqlScript.Tests.ps1`:
- Around line 183-201: The test Context that mocks Get-TargetResource but calls
Test-TargetResource is in the wrong Describe; move the entire Context block to
the Describe block that targets SqlScript\Test-TargetResource so tests follow
the one-Describe-per-entity rule; ensure the Mock -CommandName
Get-TargetResource and the It block that calls Test-TargetResource remain
intact, and update any BeforeAll/AfterAll setup scoping if necessary so the mock
and $script:mockDefaultParameters are visible to the Test-TargetResource
Describe.
In `@tests/Unit/DSC_SqlScriptQuery.Tests.ps1`:
- Around line 164-182: Move the exception-path Context that mocks
Get-TargetResource and exercises Test-TargetResource out of the Describe for
"SqlScriptQuery\Get-TargetResource" and into the Describe for
"SqlScriptQuery\Test-TargetResource"; specifically, locate the Context block
that throws 'Unable to connect to SQL instance' (it references Mock -CommandName
Get-TargetResource and calls Test-TargetResource) and cut/paste it under the
Describe that targets Test-TargetResource so each Describe only contains tests
for its respective entity.
In `@tests/Unit/DSC_SqlSecureConnection.Tests.ps1`:
- Around line 344-362: The test Context that mocks Get-TargetResource but calls
Test-TargetResource is placed under the Get-TargetResource Describe; move this
exception-path Context into the Describe block that targets Test-TargetResource
so the file follows "one Describe per file matching the tested entity"
guideline. Locate the Context block that throws 'Unable to connect to SQL
instance' (uses Mock -CommandName Get-TargetResource and calls
Test-TargetResource inside It) and cut/paste or recreate it under the Describe
for Test-TargetResource, keeping the BeforeAll mock, the It block, and the
assertion $result | Should -BeFalse intact, and remove the misplaced Context
from the Get-TargetResource Describe.
🧹 Nitpick comments (6)
source/Classes/020.SqlPermission.ps1 (1)
187-188: Inconsistent error message formatting betweenTest()andGetCurrentState().Line 187 passes
$_(fullErrorRecord) to the format string, while line 238 passes$_.Exception.Message. TheToString()of a fullErrorRecordcan be much more verbose and differently structured. Use$_.Exception.Messagein both places for consistency.♻️ Align the format argument
- Write-Verbose -Message ($this.localizedData.SQLInstanceNotReachable -f $_) + Write-Verbose -Message ($this.localizedData.SQLInstanceNotReachable -f $_.Exception.Message)Also applies to: 238-238
source/Classes/020.SqlAgentAlert.ps1 (1)
155-167: Consider narrowing the catch block or documenting why the broad exception handling is intentional.This catch pattern (catching all exceptions and returning
$false) is consistent across all class resources in this codebase (SqlAgentAlert, SqlAudit, SqlDatabase, SqlDatabasePermission, SqlPermission, SqlRSSetup). While this design handles SQL connectivity failures, it also silently swallows exceptions from other sources—such as bugs inGetCurrentState(),Modify(), orAssertProperties()—converting them to a misleading "instance not reachable" message.If this broad catch is intentional (to treat any failure as "instance unreachable"), add a clarifying comment explaining the design rationale. If it should be narrower, consider catching only connection-related exception types (e.g.,
SqlException,InvalidOperationException) so genuine coding errors surface for debugging.source/Classes/020.SqlDatabase.ps1 (1)
686-694: Consider narrowing the catch to connection-related exceptions.Catching all exceptions here can hide non-connectivity bugs. If feasible, filter known connection error IDs/types and rethrow the rest so unexpected issues surface.
tests/Unit/DSC_SqlSetup.Tests.ps1 (1)
2085-2107: Pin the test to the exception path with a call-count assertion.Right now the test can pass even if
Test-TargetResourcereturns$falsefor a different reason. Add aShould -InvokeonGet-TargetResourceto ensure the exception path is actually exercised.💡 Suggested update
It 'Should return $false' { InModuleScope -ScriptBlock { Set-StrictMode -Version 1.0 @@ $result = Test-TargetResource `@mockTestTargetResourceParameters` $result | Should -BeFalse } + + Should -Invoke -CommandName Get-TargetResource -Exactly -Times 1 -Scope It } }tests/Unit/DSC_SqlAlias.Tests.ps1 (1)
1070-1087: Consider adding aShould -Invokeassertion forGet-TargetResource.The new test context correctly validates that
Test-TargetResourcereturns$falsewhenGet-TargetResourcethrows. However, every otherContextin thisDescribeblock includes a companionItthat asserts mock invocation counts. Adding one here would keep the pattern consistent and confirm the mock was actually exercised.♻️ Suggested addition
It 'Should return $false' { InModuleScope -ScriptBlock { Set-StrictMode -Version 1.0 $result = Test-TargetResource `@mockTestTargetResourceParameters` $result | Should -BeFalse } } + + It 'Should call the correct mocks' { + Should -Invoke -CommandName Get-TargetResource -Exactly -Times 1 -Scope It + } }tests/Unit/DSC_SqlDatabaseMail.Tests.ps1 (1)
634-653: Consider adding aShould -Invokeassertion forGet-TargetResource.Same as the
DSC_SqlAliastest — the exception-path context is correct but lacks a companionItblock asserting mock invocation, which every other context in thisDescribeincludes.♻️ Suggested addition
It 'Should return $false' { InModuleScope -ScriptBlock { Set-StrictMode -Version 1.0 $testTargetResourceParameters = $script:mockDefaultParameters.Clone() $result = Test-TargetResource `@testTargetResourceParameters` $result | Should -BeFalse } } + + It 'Should call the correct mocks' { + Should -Invoke -CommandName Get-TargetResource -Exactly -Times 1 -Scope It + } }
source/DSCResources/DSC_SqlDatabaseObjectPermission/DSC_SqlDatabaseObjectPermission.psm1
Show resolved
Hide resolved
source/DSCResources/DSC_SqlSecureConnection/DSC_SqlSecureConnection.psm1
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1`:
- Around line 448-451: The Write-Verbose call uses a backtick continuation;
remove the backtick and make the expression a single statement or build the
formatted message in a temporary variable. Replace the multi-line Write-Verbose
that references $script:localizedData.SQLInstanceNotReachable -f $_ with a
single-line call (or assign $message =
$script:localizedData.SQLInstanceNotReachable -f $_ and call Write-Verbose
-Message $message) so no backtick line continuation is used.
In `@source/DSCResources/DSC_SqlWaitForAG/DSC_SqlWaitForAG.psm1`:
- Around line 298-301: The Write-Verbose call in DSC_SqlWaitForAG.psm1 uses a
backtick continuation on the string format; remove the backtick and rewrite the
expression as a single-line or use parentheses to wrap the formatted string
(targeting the Write-Verbose invocation that formats
$script:localizedData.SQLInstanceNotReachable -f $_) so it no longer uses the
backtick line continuation.
In `@tests/Unit/DSC_SqlAGDatabase.Tests.ps1`:
- Around line 2921-2926: Move the Mock for Get-TargetResource out of the
InModuleScope block: remove the Mock call currently inside InModuleScope and
instead add a BeforeAll in the parent Describe that defines Mock -CommandName
Get-TargetResource with the same MockWith throw behavior, and ensure it targets
the DSC module by specifying -ModuleName $script:dscResourceName (or set via
PSDefaultParameterValues) so the mock still applies when InModuleScope runs;
update any references to the mock accordingly and delete the inner mock to
comply with the test guidelines.
🧹 Nitpick comments (3)
tests/Unit/DSC_SqlScript.Tests.ps1 (1)
308-318: Use$script:mockTestTargetResourceParametersfor consistency with other tests in thisDescribe.The
BeforeEachat line 296 already clones$script:mockDefaultParametersinto$script:mockTestTargetResourceParameters. Every otherItblock in thisDescribeuses that variable, but this one re-clones from$script:mockDefaultParametersinto a local$testTargetResourceParameters.Suggested diff
It 'Should return $false' { InModuleScope -ScriptBlock { Set-StrictMode -Version 1.0 - $testTargetResourceParameters = $script:mockDefaultParameters.Clone() - - $result = Test-TargetResource `@testTargetResourceParameters` + $result = Test-TargetResource `@mockTestTargetResourceParameters` $result | Should -BeFalse } }tests/Unit/DSC_SqlSecureConnection.Tests.ps1 (1)
570-588: New exception-path test is correctly placed and well-structured.The test context is now under the
Test-TargetResourceDescribe block (addressing the prior review feedback), follows naming conventions (When …/Should …), mocks inBeforeAll, usesSet-StrictMode, and asserts with-BeFalse.One minor inconsistency: this test creates a local
$testTargetResourceParameters(line 581) by cloning$script:mockDefaultParameters, whereas every otherItblock in thisDescribeuses$script:mockTestTargetResourceParameters(set inBeforeEachat line 566). The test is functionally correct either way, but using the shared variable would be more consistent.Suggested alignment with existing pattern
It 'Should return $false' { InModuleScope -ScriptBlock { Set-StrictMode -Version 1.0 - $testTargetResourceParameters = $script:mockDefaultParameters.Clone() - - $result = Test-TargetResource `@testTargetResourceParameters` + $result = Test-TargetResource `@mockTestTargetResourceParameters` $result | Should -BeFalse } }source/DSCResources/DSC_SqlWaitForAG/DSC_SqlWaitForAG.psm1 (1)
292-303: Catch block swallows all exceptions, not just connection failures.This catches every exception from
Get-TargetResource(e.g., permission errors, unexpected bugs) and silently returns$false. Consider narrowing the catch to connection-related exceptions (or at minimum logging atWrite-Warninglevel for unexpected errors) so that genuine configuration bugs aren't masked as "instance not reachable."This appears to be the pattern across all resources in this PR, so if this is intentional, it may be worth documenting the rationale.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
|
I’m reverting this to a draft; during testing, I discovered that Get-TargetResource is also affected. I’ll open a Microsoft support ticket to confirm if this exception handling aligns with their recommended practices. |
1 participant
Pull Request (PR) description
This PR fixes an issue where Test-TargetResource would throw an unhandled exception if the target SQL Server instance was not installed, the service was stopped, or the instance was otherwise unreachable.
This behavior caused Azure Arc Guest Configuration (and standard DSC) to fail immediately with DscConfigurationExecutionFailed, preventing Set-TargetResource from ever running to apply the configuration.
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
This change is