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: xSQLServerNetwork: Support for configuring SQL Server to use a static port #887

Merged
merged 8 commits into from
Oct 17, 2017
Merged

BREAKING CHANGE: xSQLServerNetwork: Support for configuring SQL Server to use a static port #887

merged 8 commits into from
Oct 17, 2017

Conversation

nabrond
Copy link
Contributor

@nabrond nabrond commented Oct 15, 2017

Pull Request (PR) description
Corrects an issue where changing from a dynamic to a static TCP port is not possible. This PR renamed the parameter TcpDynamicPorts to TcpDynamicPort and changed its type to Boolean. Additionally, localization for en-US was added in support of the (localization project).

This Pull Request (PR) fixes the following issues:

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

@codecov-io
Copy link

codecov-io commented Oct 15, 2017

Codecov Report

Merging #887 into dev will increase coverage by <1%.
The diff coverage is 97%.

Impacted file tree graph

@@         Coverage Diff          @@
##            dev   #887    +/-   ##
====================================
+ Coverage    96%    96%   +<1%     
====================================
  Files        31     31            
  Lines      3430   3436     +6     
====================================
+ Hits       3311   3317     +6     
  Misses      119    119

@johlju johlju added the needs review The pull request needs a code review. label Oct 15, 2017
@johlju
Copy link
Member

johlju commented Oct 15, 2017

Thanks for localizing this one as well!

There was a strange error in the tests when run in the AppVeyor. Had nothing to do with this PR. If it persists I have to look into it. Hope it was a temporary glitch and it works when you push next commit. 😄


Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


CHANGELOG.md, line 24 at r1 (raw file):

    module stubs are loaded ([issue #784](https://github.com/PowerShell/xSQLServer/issues/784)).
- Changes to xSQLServerNetwork
  - Renamed parameter TcpDynamicPorts to TcpDynamicPort and changed type to Boolean

Please add 'BREAKING CHANGE:' before this entry.


CHANGELOG.md, line 24 at r1 (raw file):

    module stubs are loaded ([issue #784](https://github.com/PowerShell/xSQLServer/issues/784)).
- Changes to xSQLServerNetwork
  - Renamed parameter TcpDynamicPorts to TcpDynamicPort and changed type to Boolean

I think we should end each sentence with a full stop (.) .


CHANGELOG.md, line 25 at r1 (raw file):

where

Should this word be "when"?


CHANGELOG.md, line 27 at r1 (raw file):

  - Resolved issue where switching from dynamic to static port
    configuration ([issue #534](https://github.com/PowerShell/xSQLServer/issues/534))
  - Added localization (en-US) for all strings in resource and unit tests

Please add reference to the issue here as well.


README.md, line 979 at r1 (raw file):

TcpDynamicPorts

TcpDynamicPort


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 42 at r1 (raw file):

        $managedComputerObject = New-Object -TypeName Microsoft.SqlServer.Management.Smo.Wmi.ManagedComputer

        Write-Verbose ($script:localizedData.GetNetworkProtocol -f $SQLInstanceName, $ProtocolName)

I think the values should be switched here?


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 142 at r1 (raw file):

    if ($TcpDynamicPort -and $TcpPort)
    {
        throw New-TerminatingError -ErrorType $script:localizedData.ErrorDynamicAndStaticPortSpecified -ErrorCategory InvalidOperation

Could you please change to the new helper functions (to align with the other resources modules).
https://github.com/PowerShell/xSQLServer/blob/dev/CONTRIBUTING.md#new-invalidargumentexception


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 321 at r1 (raw file):

    $getTargetResourceResult = Get-TargetResource -SQLInstanceName $SQLInstanceName -ProtocolName $ProtocolName

    Write-Verbose $script:localizedData.CompareStates

Please use named parameters, -Message. Throughout.


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.schema.mof, line 4 at r1 (raw file):

class MSFT_xSQLServerNetwork : OMI_BaseResource
{
    [Key, Description("The name of the SQL instance to be configured.")] String SQLInstanceName;

Should we leave this as InstanceName until issue #308 fixed this for all resources? Right now the discussion in the issue lean towards using InstanceName.


Comments from Reviewable

@johlju johlju added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Oct 15, 2017
@nabrond
Copy link
Contributor Author

nabrond commented Oct 15, 2017

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


CHANGELOG.md, line 24 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add 'BREAKING CHANGE:' before this entry.

Done.


CHANGELOG.md, line 24 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I think we should end each sentence with a full stop (.) .

Done.


CHANGELOG.md, line 25 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

where

Should this word be "when"?

Done.


CHANGELOG.md, line 27 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add reference to the issue here as well.

Done.


README.md, line 979 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

TcpDynamicPorts

TcpDynamicPort

Done.


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 42 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I think the values should be switched here?

Done.


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 142 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please change to the new helper functions (to align with the other resources modules).
https://github.com/PowerShell/xSQLServer/blob/dev/CONTRIBUTING.md#new-invalidargumentexception

Done.


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 321 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please use named parameters, -Message. Throughout.

Done.


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.schema.mof, line 4 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should we leave this as InstanceName until issue #308 fixed this for all resources? Right now the discussion in the issue lean towards using InstanceName.

Done. I did not see that discussion until after I had completed this change. I will change them all back to InstanceName.


Comments from Reviewable

@johlju johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Oct 16, 2017
@johlju
Copy link
Member

johlju commented Oct 16, 2017

Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 984 at r2 (raw file):

TcpDynamicPorts

TcpDynamicPort


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 13 at r2 (raw file):

SQLInstanceName

InstanceName


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 73 at r2 (raw file):

SQLInstanceName

InstanceName


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 244 at r2 (raw file):

SQLInstanceName

InstanceName


Comments from Reviewable

@johlju johlju added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Oct 16, 2017
@nabrond
Copy link
Contributor Author

nabrond commented Oct 16, 2017

Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 984 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

TcpDynamicPorts

TcpDynamicPort

Done.


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 13 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SQLInstanceName

InstanceName

Done.


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 73 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SQLInstanceName

InstanceName

Done.


DSCResources/MSFT_xSQLServerNetwork/MSFT_xSQLServerNetwork.psm1, line 244 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

SQLInstanceName

InstanceName

Done.


Comments from Reviewable

@johlju johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Oct 17, 2017
@johlju
Copy link
Member

johlju commented Oct 17, 2017

:lgtm:


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@johlju johlju merged commit 4a84423 into dsccommunity:dev Oct 17, 2017
@nabrond nabrond deleted the issue-855-network-dynamic-ports branch November 8, 2017 16:06
@johlju johlju removed the needs review The pull request needs a code review. label Mar 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants