Skip to content

Conversation

@potatoqualitee
Copy link
Member

Summary

Adds the /q (quiet mode) command-line switch to SQL Server setup.exe invocation to fix installations on Windows Server Core.

Changes

  • Modified Invoke-DbaAdvancedInstall.ps1 to include /q parameter in the setup.exe command line
  • This supplements the existing QUIET = "True" setting in the configuration file

Fixes

Closes #9707

Windows Server Core requires the /q or /qs command-line parameter to be passed directly to setup.exe, not just in the configuration file. Per Microsoft's documentation:

When running SQL Server Setup on Server Core, you must use the /q (quiet mode) or /qs (quiet simple mode) parameter.


🤖 Generated with Claude Code

Fixes #9707

SQL Server setup.exe on Windows Server Core requires the /q (quiet mode)
command-line parameter to be passed directly to setup.exe, not just in the
configuration file. This change adds the /q switch to the installation
parameters to ensure compatibility with Server Core installations.

Co-authored-by: Chrissy LeMaire <[email protected]>
@claude
Copy link

claude bot commented Oct 31, 2025

Code Review - PR #9898

Summary

This PR adds the /q (quiet mode) command-line switch to SQL Server setup.exe invocation to fix installations on Windows Server Core. The change is well-targeted, minimal, and correct.


✅ Positive Feedback

1. Excellent Problem Analysis
The PR correctly identifies that Windows Server Core requires the /q parameter to be passed directly to setup.exe on the command line, not just in the configuration file. This aligns with Microsoft's documentation.

2. Minimal, Surgical Change
The fix is exactly one line (public/Invoke-DbaAdvancedInstall.ps1:265), which is the ideal approach for this type of bug fix. No unnecessary changes or refactoring.

3. Good Documentation
The PR description clearly explains the issue, the fix, and references Microsoft's documentation. The commit message is also well-written.

4. Backward Compatibility
Adding /q to the command line is safe and maintains backward compatibility with full Windows installations, as it works in conjunction with the existing QUIET = "True" setting in the configuration file.

5. PR Title Follows Convention
The title Invoke-DbaAdvancedInstall - Add /q switch for Server Core compatibility correctly follows the dbatools PR naming convention from CLAUDE.md.


📋 Observations & Considerations

1. Test Coverage
The existing integration tests in tests/Invoke-DbaAdvancedInstall.Tests.ps1 mock the Invoke-Program call, so they won't fail due to this change. However, they also won't verify that /q is actually being passed to setup.exe.

Recommendation: Consider adding a test that verifies the /q parameter is included in the argument list passed to Invoke-Program. This would be a regression test as described in CLAUDE.md's test management guidelines. However, given the simplicity of the fix and the difficulty of testing actual SQL Server installations, the current test coverage may be sufficient.

2. Code Style Check
Looking at the change location (public/Invoke-DbaAdvancedInstall.ps1:264-266):

$installParams = $ArgumentList
$installParams += "/q"
$installParams += "/CONFIGURATIONFILE=\"$remoteConfig\""

The code doesn't use splatting here, which is correct per CLAUDE.md since these are simple array additions, not cmdlet parameter calls. No style violations detected.

3. Backtick Usage
Line 266 uses a backtick in the string for escaping quotes: /CONFIGURATIONFILE=\"$remoteConfig\"
This is correct usage - backticks for escaping within strings are acceptable. The CLAUDE.md rule against backticks applies to line continuation, not string escaping.


🔍 Security & Performance

Security: No security concerns. The /q parameter is a standard SQL Server setup flag.

Performance: No performance impact. This is a command-line argument that affects the installation UI behavior, not execution time.

Error Handling: The existing error handling in Invoke-DbaAdvancedInstall is sufficient for this change.


✅ Final Recommendation

APPROVE - This is a clean, well-documented fix for a legitimate Windows Server Core compatibility issue. The change is minimal, safe, and solves the reported problem without introducing risk.

The PR is ready to merge as-is. Optional enhancement would be to add a regression test, but given the mocked test environment and the simplicity of the fix, it's not strictly necessary.


Great work on the investigation and fix! 🎉

Copy link
Collaborator

@andreasjordan andreasjordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@potatoqualitee
Copy link
Member Author

thank you! me as well. i dont reallly have a setup to test it but looked around and seems reasonable. im also surprised it wasnt there before.

@potatoqualitee potatoqualitee merged commit cc035e8 into development Nov 2, 2025
17 checks passed
@potatoqualitee potatoqualitee deleted the claude/issue-9707-20251031-1254 branch November 2, 2025 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Install-DbaInstance Error: /q or /qs was not specified when running on Server Core. The installation cannot proceed.

2 participants