SPInstallPrereqs: Improve diagnostics for PrerequisiteInstaller exit code 1000 - #1483
Open
raandree wants to merge 4 commits into
Open
SPInstallPrereqs: Improve diagnostics for PrerequisiteInstaller exit code 1000#1483raandree wants to merge 4 commits into
raandree wants to merge 4 commits into
Conversation
Contributor
WalkthroughSPInstallPrereqs now treats prerequisite installer exit code 1000 as an actionable hard failure without requesting a reboot. Tests, documentation, and the changelog cover this behavior. Build settings also update module output, task loading, and related configuration. ChangesPrerequisite installer failure handling
Build configuration updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 29-34: Update the SPInstallPrereqs changelog entry to reference
issue 1482 using the required linked “[issue `#1482`](...)” format, and wrap
PrerequisiteInstaller.exe, PrerequisiteInstaller*.log, SXSpath, and exit code
1000 in inline-code Markdown.</code>
In `@SharePointDsc/DSCResources/MSFT_SPInstallPrereqs/MSFT_SPInstallPrereqs.psm1`:
- Around line 1198-1203: Replace the literal exit-code-1000 message assigned to
$message with a localized string lookup that accepts the temporary-directory
path as its format argument. Add the corresponding string key to every
localization resource and update the relevant tests to validate the key and
formatted diagnostic, preserving the existing guidance and $env:TEMP value.
In `@SharePointDsc/DSCResources/MSFT_SPInstallPrereqs/Readme.md`:
- Around line 58-64: Update the “PrerequisiteInstaller.exe exit code 1000”
heading and its explanatory sentence in the prerequisite documentation to format
the executable name as inline code, preserving the surrounding wording and
Markdown structure.
In `@tests/Unit/SharePointDsc/SharePointDsc.SPInstallPrereqs.Tests.ps1`:
- Line 276: Update the Pester mocks and assertions in the affected tests around
Add-SPDscEvent to use Pester v5 syntax: remove the empty mock body and replace
each Assert-MockCalled assertion with an exact Should -Invoke assertion,
preserving the existing command, invocation count, and parameter expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a162e2e2-3e06-4aeb-8d6b-e6acd69a6edd
📒 Files selected for processing (4)
CHANGELOG.mdSharePointDsc/DSCResources/MSFT_SPInstallPrereqs/MSFT_SPInstallPrereqs.psm1SharePointDsc/DSCResources/MSFT_SPInstallPrereqs/Readme.mdtests/Unit/SharePointDsc/SharePointDsc.SPInstallPrereqs.Tests.ps1
Comment on lines
+29
to
+34
| - SPInstallPrereqs | ||
| - Improved diagnostics for PrerequisiteInstaller.exe exit code 1000. This undocumented | ||
| failure result is now treated as a hard failure that throws an actionable error pointing | ||
| to the native PrerequisiteInstaller*.log and, for offline installations, to the SXSpath | ||
| parameter, instead of a generic unknown exit code message. Exit code 1000 no longer | ||
| requests a reboot (#1482). |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required changelog issue-reference and Markdown format.
The new entry should link issue 1482 using the required format and inline-code file names/literals:
-- SPInstallPrereqs
- - Improved diagnostics for PrerequisiteInstaller.exe exit code 1000. This undocumented
- failure result is now treated as a hard failure that throws an actionable error pointing
- to the native PrerequisiteInstaller*.log and, for offline installations, to the SXSpath
- parameter, instead of a generic unknown exit code message. Exit code 1000 no longer
- requests a reboot (`#1482`).
+- _SPInstallPrereqs_
+ - Improved diagnostics for `PrerequisiteInstaller.exe` exit code `1000`. This
+ undocumented failure result is now treated as a hard failure that throws an
+ actionable error pointing to the native `PrerequisiteInstaller*.log` and,
+ for offline installations, to the `SXSpath` parameter, instead of a generic
+ unknown exit code message. Exit code `1000` no longer requests a reboot
+ ([issue `#1482`](https://github.com/dsccommunity/SharePointDsc/issues/1482)).As per path instructions, changelog issue references must use the linked [issue #<issue_number>](...) format.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - SPInstallPrereqs | |
| - Improved diagnostics for PrerequisiteInstaller.exe exit code 1000. This undocumented | |
| failure result is now treated as a hard failure that throws an actionable error pointing | |
| to the native PrerequisiteInstaller*.log and, for offline installations, to the SXSpath | |
| parameter, instead of a generic unknown exit code message. Exit code 1000 no longer | |
| requests a reboot (#1482). | |
| - _SPInstallPrereqs_ | |
| - Improved diagnostics for `PrerequisiteInstaller.exe` exit code `1000`. This | |
| undocumented failure result is now treated as a hard failure that throws an | |
| actionable error pointing to the native `PrerequisiteInstaller*.log` and, | |
| for offline installations, to the `SXSpath` parameter, instead of a generic | |
| unknown exit code message. Exit code `1000` no longer requests a reboot | |
| ([issue `#1482`](https://github.com/dsccommunity/SharePointDsc/issues/1482)). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 29 - 34, Update the SPInstallPrereqs changelog
entry to reference issue 1482 using the required linked “[issue `#1482`](...)”
format, and wrap PrerequisiteInstaller.exe, PrerequisiteInstaller*.log, SXSpath,
and exit code 1000 in inline-code Markdown.</code>
Source: Path instructions
Comment on lines
+1198
to
+1203
| $message = ("The prerequisite installer failed with undocumented exit code 1000. " + ` | ||
| "Review the latest PrerequisiteInstaller*.log in the temporary directory " + ` | ||
| "of the account running this resource ($env:TEMP). For offline " + ` | ||
| "installations, verify SXSpath points to the SXS store from matching " + ` | ||
| "Windows Server installation media, or preinstall all required Windows " + ` | ||
| "features from that source.") |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Localize the new user-facing error message.
This branch adds a user-facing message as a literal string. Add a localized string key for the exit-code-1000 diagnostic, passing the temporary path as a format argument, and update the corresponding localization resources and tests.
As per path instructions, all strings must be localized using string keys.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SharePointDsc/DSCResources/MSFT_SPInstallPrereqs/MSFT_SPInstallPrereqs.psm1`
around lines 1198 - 1203, Replace the literal exit-code-1000 message assigned to
$message with a localized string lookup that accepts the temporary-directory
path as its format argument. Add the corresponding string key to every
localization resource and update the relevant tests to validate the key and
formatted diagnostic, preserving the existing guidance and $env:TEMP value.
Source: Path instructions
Comment on lines
+58
to
+64
| ### PrerequisiteInstaller.exe exit code 1000 | ||
|
|
||
| Exit code `1000` is an undocumented failure result from the | ||
| PrerequisiteInstaller.exe. In contrast to exit code `3010`, which Microsoft | ||
| documents as the restart-required result, `1000` is undocumented and does | ||
| not point to a specific cause by itself. This resource treats it as a hard | ||
| failure: it does not retry the installation and does not request a reboot. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Format the executable name as inline code.
PrerequisiteInstaller.exe is a file name and should be formatted as inline code in both the heading and the explanatory sentence.
-### PrerequisiteInstaller.exe exit code 1000
+### `PrerequisiteInstaller.exe` exit code `1000`
-... from the PrerequisiteInstaller.exe.
+... from the `PrerequisiteInstaller.exe`.As per path instructions, Markdown file names and paths must use inline-code formatting.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### PrerequisiteInstaller.exe exit code 1000 | |
| Exit code `1000` is an undocumented failure result from the | |
| PrerequisiteInstaller.exe. In contrast to exit code `3010`, which Microsoft | |
| documents as the restart-required result, `1000` is undocumented and does | |
| not point to a specific cause by itself. This resource treats it as a hard | |
| failure: it does not retry the installation and does not request a reboot. | |
| ### `PrerequisiteInstaller.exe` exit code `1000` | |
| Exit code `1000` is an undocumented failure result from the | |
| `PrerequisiteInstaller.exe`. In contrast to exit code `3010`, which Microsoft | |
| documents as the restart-required result, `1000` is undocumented and does | |
| not point to a specific cause by itself. This resource treats it as a hard | |
| failure: it does not retry the installation and does not request a reboot. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SharePointDsc/DSCResources/MSFT_SPInstallPrereqs/Readme.md` around lines 58 -
64, Update the “PrerequisiteInstaller.exe exit code 1000” heading and its
explanatory sentence in the prerequisite documentation to format the executable
name as inline code, preserving the surrounding wording and Markdown structure.
Source: Path instructions
|
|
||
| It "Should call the prerequisite installer from the set method and throw an actionable error for undocumented exit code 1000 without requesting a reboot" { | ||
| Mock -CommandName Start-Process { return @{ ExitCode = 1000 } } | ||
| Mock -CommandName Add-SPDscEvent { } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the repository’s Pester v5 mock syntax.
Omit the empty mock body and replace Assert-MockCalled with exact Should -Invoke assertions:
- Mock -CommandName Add-SPDscEvent { }
+ Mock -CommandName Add-SPDscEvent
- Assert-MockCalled -CommandName Start-Process -Scope It
+ Should -Invoke -CommandName Start-Process -Exactly -Times 1 -Scope It
- Assert-MockCalled -CommandName Add-SPDscEvent -Scope It -Times 1 -Exactly -ParameterFilter {
+ Should -Invoke -CommandName Add-SPDscEvent -Exactly -Times 1 -Scope It -ParameterFilter {As per path instructions, Pester v5 tests must use Should -Invoke and omit empty mock blocks.
Also applies to: 293-296
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Unit/SharePointDsc/SharePointDsc.SPInstallPrereqs.Tests.ps1` at line
276, Update the Pester mocks and assertions in the affected tests around
Add-SPDscEvent to use Pester v5 syntax: remove the empty mock body and replace
each Assert-MockCalled assertion with an exact Should -Invoke assertion,
preserving the existing command, invocation count, and parameter expectations.
Source: Path instructions
The 'offline mode, with SXSstore specified' context always used the SharePoint 2016 offline parameter list, so Set-TargetResource threw 'In offline mode for version 16 parameter DotNet472/DotNet48 is required' for the SP2019 and SPSE stubs. The failure was masked because a previous context leaked a truthy global:DSCMachineStatus, which made the SXS branch return before the offline parameter validation. Adding the exit code 1000 test reset that flag to 0 and exposed the defect. Select the required parameters per SharePoint version and reset global:DSCMachineStatus in each test so the context no longer depends on leaked state. Co-authored-by: AI Assistant <ai@example.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/SharePointDsc/SharePointDsc.SPInstallPrereqs.Tests.ps1 (1)
289-292: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover the complete exit-code-1000 diagnostic contract.
Add environment-independent assertions for
temporary directoryandmatching Windows Server installation media; the current checks do not protect those documented guidance strings.Proposed fix
$exception.Exception.Message | Should -Match 'PrerequisiteInstaller\*\.log' $exception.Exception.Message | Should -Match 'SXSpath' + $exception.Exception.Message | Should -Match 'temporary directory' + $exception.Exception.Message | Should -Match 'matching Windows Server installation media'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/SharePointDsc/SharePointDsc.SPInstallPrereqs.Tests.ps1` around lines 289 - 292, Extend the assertions in the exit-code-1000 test around $exception.Exception.Message to also match the documented guidance for the temporary directory and matching Windows Server installation media. Use stable, environment-independent message fragments and preserve the existing checks for 1000, PrerequisiteInstaller*.log, and SXSpath.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/Unit/SharePointDsc/SharePointDsc.SPInstallPrereqs.Tests.ps1`:
- Around line 289-292: Extend the assertions in the exit-code-1000 test around
$exception.Exception.Message to also match the documented guidance for the
temporary directory and matching Windows Server installation media. Use stable,
environment-independent message fragments and preserve the existing checks for
1000, PrerequisiteInstaller*.log, and SXSpath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c9d645e-4427-4574-bdcf-c39cfbfc014f
📒 Files selected for processing (1)
tests/Unit/SharePointDsc/SharePointDsc.SPInstallPrereqs.Tests.ps1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request (PR) description
MSFT_SPInstallPrereqspreviously handled the undocumentedPrerequisiteInstaller.exeexit code1000through the genericdefaultbranch ("The prerequisite installer ran with the following unknown exit code
1000"). That message does not direct operators to the native
PrerequisiteInstaller*.log, where the actual failed prerequisite is recorded(in the reported case, a failed IIS feature install caused by missing offline
Windows feature payloads).
This PR adds an explicit
1000branch to the exit-code switch that:1000as a hard failure — it does not retry and does not classify itas success or restart-required.
$global:DSCMachineStatus, so exit code1000neverrequests a DSC reboot.
Add-SPDscEventpattern(
EntryType 'Error',EventID 100) and throws an actionable exception thatreferences exit code
1000,PrerequisiteInstaller*.log, the temporarydirectory of the account running the resource, and the
SXSpathparameter,with guidance to use matching Windows Server installation media or preinstall
the required Windows features from that source.
Microsoft documents
3010(not1000) as the restart-required result; thatdistinction is preserved. Behavior for exit codes
0,1,2,1001,3010,-1,-2147467259, and the genericdefaultbranch is unchanged. Thechange does not parse, copy, or attach native installer logs — the native log
remains the source of truth for the underlying failure. Resource
troubleshooting documentation and the changelog are updated accordingly.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
help.
This change is