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

SqlProtocolTcpIp: Auto-detect the TCP/IP address group name by IP address #1940

Conversation

claudiospizzi
Copy link
Contributor

@claudiospizzi claudiospizzi commented May 8, 2023

Pull Request (PR) description

Currently, it's not possible to detect an TCP/IP address group by using the IP. The TCP/IP address group name is mandatory during compilation time. For SQL Server running multiple instances with multiple per-instance IP's, it's important to define con compile time, which IPx address group belongs to which TCP/IP address.

This Pull Request (PR) fixes the following issues

The resource should support entering the IP address in the group name field, so that the group is detected by the IP address.

Task list

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Resource documentation updated in the resource's README.md.
  • Resource parameter descriptions updated in schema.mof.
  • Comment-based help updated, including parameter descriptions.
  • Localization strings updated.
  • Examples updated.
  • Unit tests updated. See DSC Community Testing Guidelines.
  • Integration tests updated (where possible). See DSC Community Testing Guidelines.
  • Code changes adheres to DSC Community Style Guidelines.

This change is Reviewable

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #1940 (360e02f) into main (d296853) will decrease coverage by 1%.
The diff coverage is 28%.

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #1940   +/-   ##
====================================
- Coverage    91%     91%   -1%     
====================================
  Files        92      92           
  Lines      7806    7891   +85     
====================================
+ Hits       7181    7251   +70     
- Misses      625     640   +15     
Flag Coverage Δ
unit 91% <28%> (-1%) ⬇️
Impacted Files Coverage Δ
...ces/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1 91% <28%> (-9%) ⬇️

... and 2 files with indirect coverage changes

@johlju johlju added the needs review The pull request needs a code review. label May 9, 2023
@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 May 20, 2023
@johlju
Copy link
Member

johlju commented May 20, 2023

Please see comment in issue.

@github-actions
Copy link

github-actions bot commented Jun 4, 2023

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added the abandoned The pull request has been abandoned. label Jun 4, 2023
@johlju
Copy link
Member

johlju commented Jun 6, 2023

Closing this as it has been no response on the issue #1939. Will keep the issue open for a while longer. This PR can be re-opened if need be after the discussion in the issue, and the missing tests that need to be contributed.

@johlju johlju closed this Jun 6, 2023
@johlju johlju added on hold The issue or pull request has been put on hold by a maintainer. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. abandoned The pull request has been abandoned. on hold The issue or pull request has been put on hold by a maintainer. labels Jun 6, 2023
@johlju johlju reopened this Jul 3, 2023
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reopened and have reviewed.

Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @claudiospizzi and @github-code-scanning[bot])

a discussion (no related file):
The unit tests need to be added that tests this change.



source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1 line 545 at r1 (raw file):

    $IpAddressGroup = Find-IpAddressGroup -IpAddressGroup $IpAddressGroup -InstanceName $InstanceName

    $IpAddressGroup = Convert-IpAdressGroupCasing -IpAddressGroup $IpAddressGroup

This should not be needed in the Test-TargetResource function since it calls Compare-function that calls Get-function. Get-function will sort this out. 🤔

Code quote:

    $IpAddressGroup = Find-IpAddressGroup -IpAddressGroup $IpAddressGroup -InstanceName $InstanceName

    $IpAddressGroup = Convert-IpAdressGroupCasing -IpAddressGroup $IpAddressGroup

source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1 line 770 at r1 (raw file):

        The IP address already stored in an IP address group.
#>
function Find-IpAddressGroup

We should make this a public command Find--SqlDscManagedComputerServerProtocolIpAddressGroup. See other comments as well.


source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1 line 782 at r1 (raw file):

        [Parameter(Mandatory = $true)]
        [AllowEmptyString()]

The IP Group can never be empty so this should be removed.

Code quote:

[AllowEmptyString()]

source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1 line 796 at r1 (raw file):

        )

        Import-SqlDscPreferredModule

This should not be needed as it done when SqlServerDsc is imported (which will be done when the public command is used). We should be able to remove this. See next comment too.

Code quote:

Import-SqlDscPreferredModule

source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1 line 808 at r1 (raw file):

        }

        $serverProtocolProperties = Get-ServerProtocolObject @getServerProtocolObjectParameters

We should make this a public command Get-SqlDscManagedComputerServerProtocol (that also resuses Get-SqlDscManagedComputer). See command Get-SqlDscManagedComputerService for example how the new command should work. The command Get-ServerProtocolObject can be kept also and be switched to the new public command in other PR's.

Code quote:

Get-ServerProtocolObject

@johlju johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jul 3, 2023
@github-actions
Copy link

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added the abandoned The pull request has been abandoned. label Jul 18, 2023
@johlju
Copy link
Member

johlju commented Sep 30, 2023

Closing this as there have been no more progress on this. Please reopen to continue working on this, or another contributor can open a new PR.

@johlju johlju closed this Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned The pull request has been abandoned. waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SqlProtocolTcpIp: Auto-detect the TCP/IP address group name by IP address
2 participants