Skip to content
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: SqlServerDsc: Fix inconsistent parameter names in all resources #917

Merged
merged 33 commits into from
Dec 1, 2017

Conversation

johlju
Copy link
Member

@johlju johlju commented Nov 28, 2017

Pull Request (PR) description
This pull request will fix all inconsistent parameter names so that InstanceName is the parameter used to set the name of the instance to configure, and ServerName is used to specify the NetBios or FQDN name where the instance can be found.

This Pull Request (PR) fixes the following issues:
Fixes #308

Task list:

  • Change details added to Unreleased section of CHANGELOG.md?
  • Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
  • Examples appropriately updated?
  • New/changed code adheres to Style Guidelines?
  • Unit and (optional) Integration tests created/updated where possible?

This change is Reviewable

@johlju
Copy link
Member Author

johlju commented Nov 28, 2017

@randomnote1 @nabrond If you have time, then please help me review this one. I really need a second pair of eyes on this one. It's hard to be objective of ones own code. 😄

@PlagueHO If you get bored on your mini-holiday, then you can help out reviewing this one. Haha 😉

@johlju johlju added the needs review The pull request needs a code review. label Nov 28, 2017
@codecov-io
Copy link

codecov-io commented Nov 28, 2017

Codecov Report

Merging #917 into dev will not change coverage.
The diff coverage is 99%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #917   +/-   ##
===================================
  Coverage    96%    96%           
===================================
  Files        32     32           
  Lines      3515   3515           
===================================
  Hits       3395   3395           
  Misses      120    120

@randomnote1
Copy link
Contributor

Reviewed 15 of 129 files at r1.
Review status: 15 of 129 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@nabrond
Copy link
Contributor

nabrond commented Nov 29, 2017

@johlju, I noticed that no helper functions were included in this PR. Functions like Connect-SQLServer are still utilizing the "old" parameters. Do you think it is too ambitious to bring them into scope of this pull?


Comments from Reviewable

@nabrond
Copy link
Contributor

nabrond commented Nov 29, 2017

Reviewed 7 of 129 files at r1.
Review status: 22 of 129 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@nabrond
Copy link
Contributor

nabrond commented Nov 29, 2017

Reviewed 6 of 129 files at r1.
Review status: 28 of 129 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@nabrond
Copy link
Contributor

nabrond commented Nov 29, 2017

Reviewed 1 of 129 files at r1.
Review status: 29 of 129 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Nov 29, 2017

@nabrond I thought about it, but I didn't want to do that in this PR since it can be done later without a breaking change (was a lot of work getting all the tests working any way 😄 ). But I will make sure to add an issue for that so it doesn't get forgotten.

@randomnote1
Copy link
Contributor

Reviewed 2 of 129 files at r1.
Review status: 31 of 129 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@randomnote1
Copy link
Contributor

Reviewed 15 of 129 files at r1.
Review status: 44 of 129 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 129 at r1 (raw file):

        Name            = $Name
        ServerName      = $ServerName
        InstanceName = $InstanceName

Fix formatting


DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 342 at r1 (raw file):

    $getTargetResourceParameters = @{
        InstanceName = $PSBoundParameters.InstanceName

Fix formatting


Examples/Resources/SqlDatabasePermission/1-GrantDatabasePermissions.ps1, line 23 at r1 (raw file):

            Name                 = 'CONTOSO\SQLAdmin'
            LoginType            = 'WindowsUser'
            ServerName           = 'sqltest.company.local'

Just curious...Why did you update this to an FQDN?


Comments from Reviewable

@randomnote1
Copy link
Contributor

Reviewed 11 of 129 files at r1.
Review status: 55 of 129 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Nov 30, 2017

Review status: 55 of 129 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Examples/Resources/SqlDatabasePermission/1-GrantDatabasePermissions.ps1, line 23 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Just curious...Why did you update this to an FQDN?

It said 'SQLServer' before so it was messing with my search and replace, and when changing the name I just wrote FQDN by habit. 😄 I can change it back to NetBIOS if you like.


Comments from Reviewable

@randomnote1
Copy link
Contributor

Reviewed 16 of 129 files at r1.
Review status: 71 of 129 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Examples/Resources/SqlDatabasePermission/1-GrantDatabasePermissions.ps1, line 23 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It said 'SQLServer' before so it was messing with my search and replace, and when changing the name I just wrote FQDN by habit. 😄 I can change it back to NetBIOS if you like.

Ahh, that makes sense. No big deal. Like I said, I was just curious!


Tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 58 at r1 (raw file):

        $instanceParameters = @{
            InstanceName = 'MSSQLSERVER'
            ServerName = 'Server1'

Hash table should be formatted with the appropriate spacing.


Comments from Reviewable

@randomnote1
Copy link
Contributor

Reviewed 26 of 129 files at r1.
Review status: 97 of 129 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Comments from Reviewable

@randomnote1
Copy link
Contributor

@johlju, There are only a few formatting issues. Other than that, I think this looks good.


Reviewed 30 of 129 files at r1.
Review status: 127 of 129 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Comments from Reviewable

@randomnote1
Copy link
Contributor

Review status: 127 of 129 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Examples/Resources/SqlDatabasePermission/1-GrantDatabasePermissions.ps1, line 23 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Ahh, that makes sense. No big deal. Like I said, I was just curious!

:lgtm:


Comments from Reviewable

- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    (issue dsccommunity#308).
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer has been renamed to ServerName
    (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
@johlju
Copy link
Member Author

johlju commented Dec 1, 2017

Many thanks for reviewing! This was a big one! 😄 All comments should be resolved. Please sign off on them.
Also opened more issues regarding rename, please comment on those to see if we should do them (or not) before release.


Review status: 124 of 129 files reviewed at latest revision, 4 unresolved discussions.


DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 129 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Fix formatting

Done.


DSCResources/MSFT_SqlDatabaseRole/MSFT_SqlDatabaseRole.psm1, line 342 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Fix formatting

Done.


Examples/Resources/SqlDatabasePermission/1-GrantDatabasePermissions.ps1, line 23 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

:lgtm:

Done.


Tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 58 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Hash table should be formatted with the appropriate spacing.

Done. For some reason auto-formatting did not touch this one. I moved it manually.


Comments from Reviewable

@randomnote1
Copy link
Contributor

Glad to help! Just one more thing I missed earlier and it should be good to go.


Reviewed 2 of 3 files at r2, 2 of 2 files at r3.
Review status: 128 of 129 files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 8 at r3 (raw file):

  ([issue #916](https://github.com/PowerShell/SqlServerDsc/issues/916)).
- BREAKING CHANGE: Significant rename to reduce length of resource names
  - See [issue #851](https://github.com/PowerShell/xSQLServer/issues/851) for a

Should be https://github.com/PowerShell/SqlServerDsc/issues here and throughout the rest of the Unreleased section.

Or should it be throughout the whole file?


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Dec 1, 2017

Good catch - totally missed that! Can you please sign off on the last changes. 😄


Review status: 128 of 129 files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 8 at r3 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Should be https://github.com/PowerShell/SqlServerDsc/issues here and throughout the rest of the Unreleased section.

Or should it be throughout the whole file?

Done.


Comments from Reviewable

@randomnote1
Copy link
Contributor

Review status: 128 of 129 files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 8 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Done.

Still says xSQLServer on this line


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Dec 1, 2017

Review status: 128 of 129 files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 8 at r3 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Still says xSQLServer on this line

Done. Really this time. 😄 I looked over this twice to make sure, and still managed to miss one. :s


Comments from Reviewable

@randomnote1
Copy link
Contributor

Looks good!
:lgtm:


Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Dec 1, 2017

@randomnote1 Awesome thank you! Waiting for the tests to pass and I will merge this one.

@johlju johlju merged commit af10f71 into dsccommunity:dev Dec 1, 2017
@vors vors removed the needs review The pull request needs a code review. label Dec 1, 2017
@johlju johlju deleted the fix-issue-308 branch December 5, 2017 15:19
@johlju johlju added the breaking change When used on an issue, the issue has been determined to be a breaking change. label Dec 7, 2017
@johlju johlju changed the title SqlServerDsc: Fix inconsistent parameter names in all resources BREAKING CHANGE: SqlServerDsc: Fix inconsistent parameter names in all resources Dec 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: BREAKING CHANGE: Host and instance parameter is not consistent throughout the module
5 participants