Fix/graceful sqlscript file missing#2449
Conversation
WalkthroughAdds pre-validation file-existence checks to the SqlScript DSC resource: Get-TargetResource and Set-TargetResource now throw localized file-not-found errors when their script paths are missing; Test-TargetResource logs verbose and returns false when its file is missing. CHANGELOG, localization strings, unit and integration tests updated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_SqlScript/DSC_SqlScript.psm1`:
- Around line 287-291: Replace the bare Throw inside the Test-Path check for
$SetFilePath with creation and throwing of an ItemNotFoundException that uses a
localized string; add a new resource key SetFilePath_FileNotFound to
en-US/DSC_SqlScript.strings.psd1 (value like "The file specified in SetFilePath
('{0}') does not exist or is not accessible. Cannot apply desired state."), then
in the Test-Path block build the message from the localized key (e.g. $msg =
$LocalizedData['SetFilePath_FileNotFound'] -f $SetFilePath) and throw
(New-Object System.Management.Automation.ItemNotFoundException($msg)) instead of
the current Throw, ensuring you reference $SetFilePath in the message.
- Around line 124-128: Replace the bare Throw when the file at $GetFilePath is
missing with a call to New-ObjectNotFoundException using a localized message
from $script:localizedData; add a new key (e.g., "GetFilePath_FileNotFound") to
en-US/DSC_SqlScript.strings.psd1 with the message template and then change the
check in DSC_SqlScript.psm1 to construct the message via -f $GetFilePath (or the
helper used elsewhere) and pass it to New-ObjectNotFoundException so the
exception follows MOF DSC guidelines and uses localization.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
26-34: Add issue reference and wrap long lines in Fixed entry.The new Fixed items lack an issue reference in the required format, and multiple lines exceed 80 characters. Please add an issue link and wrap lines at word boundaries. As per coding guidelines: “Reference issues using format issue #<issue_number> in CHANGELOG.md” and “Wrap lines at word boundaries when over 80 characters.”
Proposed update (placeholder issue number)
- SqlScript - - Fixed logic in `Get-TargetResource` and `Set-TargetResource` to throw an error - when the SQL script file is missing, instead of incorrectly reporting success - or using a null variable. - - Fixed `Test-TargetResource` to return `$false` (instead of throwing) when - the SQL script file is missing, enabling `DependsOn` scenarios where the file - is created at runtime. + - Fixed logic in `Get-TargetResource` and `Set-TargetResource` to throw an + error when the SQL script file is missing, instead of incorrectly reporting + success or using a null variable + ([issue `#2449`](https://github.com/dsccommunity/SqlServerDsc/issues/2449)). + - Fixed `Test-TargetResource` to return `$false` (instead of throwing) when + the SQL script file is missing, enabling `DependsOn` scenarios where the + file is created at runtime + ([issue `#2449`](https://github.com/dsccommunity/SqlServerDsc/issues/2449)).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_SqlScript/en-US/DSC_SqlScript.strings.psd1`:
- Around line 8-9: Add a new localized string key TestFilePath_FileNotFound in
the same .strings.psd1 block (matching the pattern of GetFilePath_FileNotFound
and SetFilePath_FileNotFound) and update the Test-TargetResource implementation
to use this key instead of the hardcoded English message; specifically, add the
TestFilePath_FileNotFound entry with the appropriate localized message text and
replace the hardcoded Write-Verbose/Write-Error call in Test-TargetResource to
retrieve and use the localized string (e.g., via the existing
Get-LocalizedString helper or the module's localization lookup used for the
other keys).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2449 +/- ##
=====================================
Coverage 94% 94%
=====================================
Files 227 227
Lines 10778 10788 +10
=====================================
+ Hits 10149 10159 +10
Misses 629 629
🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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_SqlScript.Tests.ps1 (2)
263-377:⚠️ Potential issue | 🟠 MajorMissing
Test-Pathmocks and file-missing test scenario for Test-TargetResource.Same pattern as Set-TargetResource — the PR objectives state
Test-TargetResourcenow returns$falsewhen TestFilePath is missing, but:
Existing "desired state" contexts (lines 286–318) don't mock
Test-Path. The tests at lines 297 and 315 assertShould -BeTrue, but with the new pre-check,Test-Path 'test.sql'will return$falseon the runner, causing the function to return$falsebeforeInvoke-SqlScriptruns — failing these assertions.Missing a
Context 'When the TestFilePath file is missing'under "not in desired state" to verify the function returns$false(and optionally that the verbose message is written, though per guidelines verbose messages aren't tested).Suggested additions
Add
Mock -CommandName Test-Path -MockWith { return $true }to theBeforeAllblocks at lines 287 and 303, and to the "not in desired state" contexts at lines 323, 343, and 361.Add a new Context:
Context 'When the TestFilePath file is missing' { BeforeAll { Mock -CommandName Test-Path -MockWith { return $false } } It 'Should return false' { InModuleScope -ScriptBlock { Set-StrictMode -Version 1.0 $result = Test-TargetResource `@mockTestTargetResourceParameters` $result | Should -BeFalse } } }
209-260:⚠️ Potential issue | 🟠 MajorAdd
Test-Pathmocks to Set-TargetResource test contexts and add missing file-validation test scenario.Per the PR,
Set-TargetResourcenow validates thatSetFilePathexists before callingInvoke-SqlScript(line 288). Two gaps:
Existing test contexts need
Mock -CommandName Test-Path -MockWith { return $true }— Without this,Test-Pathwill return$falsefor the mock path'set.sql', causing the file validation to throwSetFilePath_FileNotFoundbeforeInvoke-SqlScriptis ever called, breaking these tests.Missing a new
Context 'When the SetFilePath file is missing'to verify the localized terminating error is thrown (analogous to lines 170–184 for Get-TargetResource).Suggested additions
Add to each existing
BeforeAllin Set-TargetResource contexts (lines 211 and 227):BeforeAll { + Mock -CommandName Test-Path -MockWith { return $true } + Mock -CommandName Invoke-SqlScript -MockWith {Add a new Context after line 260:
Context 'When the SetFilePath file is missing' { BeforeAll { Mock -CommandName Test-Path -MockWith { return $false } } It 'Should throw the localized file not found exception' { InModuleScope -ScriptBlock { Set-StrictMode -Version 1.0 $expectedError = $script:localizedData.SetFilePath_FileNotFound -f $script:mockSetTargetResourceParameters.SetFilePath { Set-TargetResource `@mockSetTargetResourceParameters` } | Should -Throw -ExpectedMessage $expectedError } } }
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 30-36: Change the plain "SqlScript" heading to use Markdown
italics: wrap the resource name (SqlScript) with underscores (or asterisks) so
it appears as _SqlScript_ in the changelog; update the line containing the
resource name and any other occurrences in the same block referencing the
resource (e.g., Get-TargetResource, Set-TargetResource, Test-TargetResource
entries) to keep formatting consistent.
- Around line 28-36: The CHANGELOG.md Unreleased "Fixed" entry for SqlScript is
missing the required issue reference; update the SqlScript bullets under "Fixed"
to append the issue link in the required format (e.g. [issue
#<n>](https://github.com/<owner>/<repo>/issues/<n>)) so each notable change (the
fixes to Get-TargetResource, Set-TargetResource and Test-TargetResource) cites
the correct issue; locate the SqlScript section and add the appropriate [issue
#...] link(s) after the sentence(s) for Get-TargetResource, Set-TargetResource
and Test-TargetResource using the real owner/repo and issue number.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/Unit/DSC_SqlScript.Tests.ps1`:
- Around line 415-425: Remove the entire "It 'Should write the verbose message'"
test block: delete the It { ... } that calls Test-TargetResource and uses Should
-WriteVerbose, including the local variables $expectedMessage,
$verbosePreference and the invocation { Test-TargetResource
`@mockTestTargetResourceParameters` }, because testing verbose output (and using
the non-standard Should -WriteVerbose assertion) violates the coding guideline;
leave other tests intact.
- Around line 167-181: The test uses wrong localized string keys causing
$script:localizedData lookups to return $null; update the key names used where
$expectedError is built to match the strings file pattern
"{FunctionName}_FileNotFound" — replace GetScriptFileNotFound with
GetFilePath_FileNotFound for the Get-TargetResource test, and likewise update
the other tests to use SetFilePath_FileNotFound and TestFilePath_FileNotFound so
that $script:localizedData.GetFilePath_FileNotFound (and the Set/Test
equivalents) return the expected message for the Should -Throw assertion.
🧹 Nitpick comments (1)
tests/Unit/DSC_SqlScript.Tests.ps1 (1)
249-251: Minor: extra blank line beforeItblock.There are two consecutive blank lines (Lines 249-250) before the
Itblock. The coding guidelines state "Maximum two consecutive newlines," so this just barely passes, but the surrounding code consistently uses single blank lines between blocks.
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manuelgr0).
|
I will merge once tests passes, I will keep an eye on them. |
johlju
added
ready for merge
|
So there seems to be a bug in the integration tests with the xScript resource that hasn't surface until now. Unrelated to this PR I believe. |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manuelgr0).
johlju
removed
the
ready for merge
2 participants
Pull Request (PR) description
This PR improves the logic for handling missing SQL script files across the
Get,Test, andSetmethods in theSqlScriptresource. It addresses issues where errors were previously swallowed or logic prevented bootstrapping scenarios.Crucially, this fix is necessary for Azure Arc Guest Configuration (Machine Configuration). In Guest Configuration, if
Test-TargetResourcethrows an exception due to a missing file, the entire configuration agent halts the consistency check. This prevents the remediation (Set) phase from ever running. Consequently, any dependent resources (viaDependsOn) that are responsible for creating the SQL file are never executed, leading to a permanent failure loop.Changes:
$GetFilePathis missing. Previously, it returned an empty result object, which masked configuration errors (such as typos in the file path) and falsely reported success.$false(and write a Verbose message) if$TestFilePathis missing. This fixes scenarios where the SQL file is generated by a dependent resource (usingDependsOn). Returning$falseallows the DSC engine to proceed to the Set phase where the file can be created.$FilePath) instead of the correct parameter ($SetFilePath).This Pull Request (PR) fixes the following issues
None
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