-
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
BREAKING CHANGE: SqlDatabaseOwner: Support multi-instances #1204
BREAKING CHANGE: SqlDatabaseOwner: Support multi-instances #1204
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1204 +/- ##
====================================
Coverage 97% 97%
====================================
Files 33 33
Lines 4016 4016
====================================
Hits 3929 3929
Misses 87 87 |
@apichaya-s Thank you for sending in this fix! I'm out of time today, but will review as soon as I can. |
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.
Great work on this one! Just a few minor comments.
Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @apichaya-s)
a discussion (no related file):
Can we add another instance of the resource to this example, to test that two different instances can update the same database name? The examples are compiled during testing so it's one kind of regression test.
https://github.com/PowerShell/SqlServerDsc/blob/dev/Examples/Resources/SqlDatabaseOwner/1-SetDatabaseOwner.ps1
a discussion (no related file):
We should update the type qualifier from Write
to Key
for the parameter InstanceName
in the README.md.
CHANGELOG.md, line 10 at r2 (raw file):
[Nick Reilingh (@NReilingh)](https://github.com/NReilingh) - Changes to SqlDatabaseOwner - Support multi-instances which is the instance cannot be omitted
I think this is a breaking change, since existing configuration will break if they haven't specified the instance name since the default value was 'MSSQLSERVER'
? If so I think we have to prefix this entry with 'BREAKING CHANGE:'.
- Changes to SqlDatabaseOwner
- BREAKING CHANGE: Support...
DSCResources/MSFT_SqlDatabaseOwner/MSFT_SqlDatabaseOwner.psm1, line 44 at r2 (raw file):
[ValidateNotNullOrEmpty()] [System.String] $InstanceName = 'MSSQLSERVER'
We should remove the default value from this since it is mandatory now.
DSCResources/MSFT_SqlDatabaseOwner/MSFT_SqlDatabaseOwner.psm1, line 119 at r2 (raw file):
$ServerName = $env:COMPUTERNAME, [Parameter(Mandatory = $true)]
We should remove the default value from this since it is mandatory now.
DSCResources/MSFT_SqlDatabaseOwner/MSFT_SqlDatabaseOwner.psm1, line 194 at r2 (raw file):
$ServerName = $env:COMPUTERNAME, [Parameter(Mandatory = $true)]
We should remove the default value from this since it is mandatory now.
Code fixed, please review |
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 4 of 4 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved
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: all files reviewed, 1 unresolved discussion (waiting on @apichaya-s)
CHANGELOG.md, line 10 at r3 (raw file):
Support multi-instances which is the instance cannot be omitted
Maybe the text reads better as "Support multiple instances on the same node. The parameter InstanceName is now Key and cannot be omitted."?
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 r4.
Reviewable status: complete! all files reviewed, all discussions resolved
babe024
to
e3901d8
Compare
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.
That is what I get for LGTM before the tests has finished 😉
Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved
@apichaya-s Awesome work on this one. Thanks! |
Pull Request (PR) description
Support multi-instances
This Pull Request (PR) fixes the following issues
Fixes #1197
Task list
should say what was changed, and how that affects users (if applicable).
and comment-based help?
See DSC Resource Testing Guidelines.
See DSC Resource Testing Guidelines.
DSC Resource Style Guidelines
and Best Practices?
This change is