-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SqlServerDsc: Add public commands for setup actions #1784
SqlServerDsc: Add public commands for setup actions #1784
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1784 +/- ##
=====================================
Coverage 88% 88%
=====================================
Files 69 83 +14
Lines 7226 7443 +217
=====================================
+ Hits 6387 6593 +206
- Misses 839 850 +11
|
b484ebd
to
0f1cddf
Compare
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
51ef128
to
765f51a
Compare
It seems there has been new arguments (at least one) added and values for arguments has been changed (at least 'ARC' seems to have changed) for SQL Server 2022. I will add an issue for that so it can be resolved in another PR. |
This PR is ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 16 files at r1, 7 of 10 files at r2, 9 of 13 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @wsmelton)
source/Private/Assert-ElevatedUser.ps1
line 16 at r4 (raw file):
None. #> function Assert-ElevatedUser
We should look at moving this to DscResource.Common (as a public command). Will create a seperate issue for this.
Code quote:
Assert-ElevatedUser
source/Private/Assert-ElevatedUser.ps1
line 39 at r4 (raw file):
[System.Management.Automation.ErrorRecord]::new( $script:localizedData.IsElevated_UserNotElevated, 'TIE0001',
Should be changed to
Suggestion:
AEU0001
source/Private/Assert-RequiredCommandParameter.ps1
line 37 at r4 (raw file):
in the module DscResource.Common, instead of being a separate command. #> function Assert-RequiredCommandParameter
We should look at moving this to DscResource.Common (as a public command). Will create a seperate issue for this.
This could be a private function but is called by public command Assert-BoundParameter that implements a new parmeter set.
Code quote:
Assert-RequiredCommandParameter
source/Private/Invoke-SetupAction.ps1
line 1387 at r4 (raw file):
[System.Management.Automation.ErrorRecord]::new( ('{0} {1}' -f $setupExitMessage, $script:localizedData.Server_SetupFailed), 'ISDS0001', # cspell: disable-line
Should change to
Suggestion:
IS0001
source/Private/Test-IsNumericType.ps1
line 19 at r4 (raw file):
[System.Boolean] #> function Test-IsNumericType
We should look at moving this to DscResource.Common (as a public command). Will create a seperate issue for this.
Code quote:
Test-IsNumericType
source/Private/Test-ServiceAccountRequirePassword.ps1
line 16 at r4 (raw file):
[System.Boolean] #> function Test-ServiceAccountRequirePassword
We should look at moving this to DscResource.Common (as a public command). Will create a seperate issue for this. Maybe change from "ServiceAccount" to the more generic "Account" in the public command.
Code quote:
Test-ServiceAccountRequirePassword
source/Private/Assert-SetupActionProperties.ps1
line 24 at r4 (raw file):
.NOTES This function is used by the command Install-SqlDscServer to verify that
Should be changed to:
Suggestion:
Invoke-SetupAction
source/Private/Assert-SetupActionProperties.ps1
line 165 at r4 (raw file):
[System.Management.Automation.ErrorRecord]::new( ($script:localizedData.InstallSqlServerProperties_ASServerModeInvalidValue -f $SetupAction), 'AISSP0001', # cSpell: disable-line
Should change to
Suggestion:
ASAP0001
source/Private/Assert-SetupActionProperties.ps1
line 181 at r4 (raw file):
[System.Management.Automation.ErrorRecord]::new( ($script:localizedData.InstallSqlServerProperties_RsInstallModeInvalidValue -f $SetupAction), 'AISSP0002', # cSpell: disable-line
Should change to
Suggestion:
ASAP0002
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r5, all commit messages.
Dismissed @wsmelton from 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johlju)
source/Private/Assert-ElevatedUser.ps1
line 39 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Should be changed to
Done
source/Private/Invoke-SetupAction.ps1
line 1387 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Should change to
Done
source/Public/Install-SqlDscServer.ps1
line 429 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Changed to SecureString on all password parameters.
Done
source/Public/Install-SqlDscServer.ps1
line 686 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
@wsmelton thank you for your comments. Much appreciated! 🙇
Done
source/Private/Assert-SetupActionProperties.ps1
line 24 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Should be changed to:
Done
source/Private/Assert-SetupActionProperties.ps1
line 165 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Should change to
Done
source/Private/Assert-SetupActionProperties.ps1
line 181 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Should change to
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johlju)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johlju)
Pull Request (PR) description
help for more information):
Assert-ElevatedUser
Assert-RequiredCommandParameter
Test-IsNumericType
Assert-SetupActionProperties
Invoke-SetupAction
help for more information):
Install-SqlDscServer
Uninstall-SqlDscServer
Add-SqlDscNode
Remove-SqlDscNode
Repair-SqlDscServer
Complete-SqlDscImage
Complete-SqlDscFailoverCluster
Initialize-SqlDscRebuildDatabase
The syntax from the commands below.
Install-SqlDscServer
Uninstall-SqlDscServer
Add-SqlDscNode
Remove-SqlDscNode
Repair-SqlDscServer
Complete-SqlDscImage
Complete-SqlDscFailoverCluster
Initialize-SqlDscRebuildDatabase
This Pull Request (PR) fixes the following issues
In preparation for issue #1781.
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