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

xSQLServerAlwaysOnAvailabilityGroup: Resolved FQDN and Logic Bugs #853

Merged
merged 19 commits into from
Oct 9, 2017
Merged

xSQLServerAlwaysOnAvailabilityGroup: Resolved FQDN and Logic Bugs #853

merged 19 commits into from
Oct 9, 2017

Conversation

randomnote1
Copy link
Contributor

@randomnote1 randomnote1 commented Sep 29, 2017

Pull Request (PR) description

  • Changes to xSQLServerAlwaysOnAvailabilityGroup
    • Refactored the unit tests to allow them to be more user friendly and to test
      additional SQLServer variations.
      • Each test will utilize the Import-SQLModuleStub to ensure the correct module is loaded.
    • Fixed an issue when setting the SQLServer parameter to a Fully Qualified Domain Name (FQDN).
    • Fixed the logic so that if a parameter is not supplied to the resource, the resource will not attempt to apply the defaults on subsequent checks.
  • Added the CommonTestHelper.psm1 to store common testing functions.
    • Added the Import-SQLModuleStub function to ensure the correct version of the module stubs are loaded.

This Pull Request (PR) fixes the following issues:
Fixes #468
Fixes #784
Fixes some of #517

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 Sep 29, 2017

Codecov Report

Merging #853 into dev will increase coverage by <1%.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff          @@
##            dev   #853    +/-   ##
====================================
+ Coverage    96%    96%   +<1%     
====================================
  Files        31     31            
  Lines      3429   3430     +1     
====================================
+ Hits       3310   3311     +1     
  Misses      119    119

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

johlju commented Oct 5, 2017

Thinking that we should revert the changes to SQLPSStub to be pure SQLPS 2014 with this change? But unsure if there is other changes/tests that also need to be resolved first. Maybe a separate issue for reverting SQLPSStub to the correct SQLPS 2014 "level"?


Reviewed 2 of 3 files at r1, 1 of 2 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 13 unresolved discussions.


CHANGELOG.md, line 65 at r4 (raw file):

    additional SQLServer variations.
    - Each test will utilize the Import-SQLModuleStub to ensure the correct
      module is loaded ([issue #784](https://github.com/PowerShell/xSQLServer/issues/784))

Add full stop ('.') at the end of the sentence.


CHANGELOG.md, line 73 at r4 (raw file):

  - Use the Get-PrimaryReplicaServerObject helper function.
  - Fixed an issue when setting the SQLServer parameter to a Fully Qualified
    Domain Name (FQDN). ([issue #468](https://github.com/PowerShell/xSQLServer/issues/468))

Move full stop ('.') to the end of the sentence.


CHANGELOG.md, line 75 at r4 (raw file):

    Domain Name (FQDN). ([issue #468](https://github.com/PowerShell/xSQLServer/issues/468))
  - Fixed the logic so that if a parameter is not supplied to the resource, the
    resource will not attempt to apply the defaults on subsequent checks.

Move full stop ('.') to the end of the sentence.


CHANGELOG.md, line 122 at r4 (raw file):

- Added the CommonTestHelper.psm1 to store common testing functions.
  - Added the Import-SQLModuleStub function to ensure the correct version of the
    module stubs are loaded. ([issue #784](https://github.com/PowerShell/xSQLServer/issues/784))

Move full stop ('.') to the end of the sentence.


DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroup/MSFT_xSQLServerAlwaysOnAvailabilityGroup.psm1, line 371 at r4 (raw file):

            {
                # Get the parameters that were submitted to the function
                [array]$submittedParameters = $PSBoundParameters.Keys

Change to [System.Array] $... (namespace in the name and space before $).


Tests/TestHelpers/CommonTestHelper.psm1, line 17 at r4 (raw file):

[UInt32]

[System.UInt32]


Tests/TestHelpers/CommonTestHelper.psm1, line 20 at r4 (raw file):

Mandatory = $false

Remove this. Should not be needed (?). Or should it be mandatory in that parameter set?


Tests/TestHelpers/CommonTestHelper.psm1, line 22 at r4 (raw file):

        [Parameter(Mandatory = $false, ParameterSetName = 'Module')]
        [ValidateSet('SQLPS','SQLServer')]
        [String]

[System.String]


Tests/TestHelpers/CommonTestHelper.psm1, line 49 at r4 (raw file):

    # Ensure none of the other stub modules are loaded
    [array]$otherStubModules = $modulesAndStubs.Values | Where-Object -FilterScript { $_ -ne $stubModuleName }

Change to [System.Array] $... (namespace in the name and space before $).


Tests/TestHelpers/CommonTestHelper.psm1, line 51 at r4 (raw file):

    [array]$otherStubModules = $modulesAndStubs.Values | Where-Object -FilterScript { $_ -ne $stubModuleName }

    $otherStubModules | Foreach-Object -Process {

Don't think you need to loop. Just do Remove-Module -Name $otherStubModules?

PS J:\> Get-Module

ModuleType Version    Name                                ExportedCommands
---------- -------    ----                                ----------------
Script     3.3.1      AzureRM.Backup                      {Backup-AzureRmBackupItem, Disable-AzureRmBackupProtection...
Script     3.3.1      AzureRM.Batch                       {Disable-AzureBatchAutoScale, Disable-AzureBatchComputeNod...
Manifest   3.1.0.0    Microsoft.PowerShell.Management     {Add-Computer, Add-Content, Checkpoint-Computer, Clear-Con...
Manifest   3.1.0.0    Microsoft.PowerShell.Utility        {Add-Member, Add-Type, Clear-Variable, Compare-Object...}
Script     1.2        PSReadline                          {Get-PSReadlineKeyHandler, Get-PSReadlineOption, Remove-PS...


PS J:\> $a = @('AzureRM.Backup','AzureRM.Batch')
PS J:\> Remove-Module -Name $a -Verbose
VERBOSE: Performing the operation "Remove-Module" on target "Microsoft.Azure.Commands.AzureBackup (Path:
'C:\Users\johlju\Documents\WindowsPowerShell\Modules\AzureRM.Backup\3.3.1\Microsoft.Azure.Commands.AzureBackup.dll')".
VERBOSE: Performing the operation "Remove-Module" on target "AzureRM.Backup (Path:
'C:\Users\johlju\Documents\WindowsPowerShell\Modules\AzureRM.Backup\3.3.1\AzureRM.Backup.psm1')".
VERBOSE: Performing the operation "Remove-Module" on target "Microsoft.Azure.Commands.Batch (Path:
'C:\Users\johlju\Documents\WindowsPowerShell\Modules\AzureRM.Batch\3.3.1\Microsoft.Azure.Commands.Batch.dll')".
VERBOSE: Performing the operation "Remove-Module" on target "AzureRM.Batch (Path:
'C:\Users\johlju\Documents\WindowsPowerShell\Modules\AzureRM.Batch\3.3.1\AzureRM.Batch.psm1')".
VERBOSE: Removing the imported "Reactivate-AzureBatchTask" alias.
VERBOSE: Removing the imported "Get-AzureRmBatchSubscriptionQuotas" alias.

Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1, line 58 at r4 (raw file):

                #NetBIOS = 'Server1'
            }
            Server2 = @{

Maybe a blank row before this row?


Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1, line 82 at r4 (raw file):

                NetName = 'Server1'
            }
            Server2 = @{

Maybe a blank row before this row?


Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1, line 111 at r4 (raw file):

                Role = 'Primary'
            }
            Server2 = @{

Maybe a blank row before this row?


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 5, 2017
@randomnote1
Copy link
Contributor Author

Good call...I forgot that there were changes made.

I think we should track that in a separate issue since I have no idea what that might break.


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


CHANGELOG.md, line 65 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add full stop ('.') at the end of the sentence.

Done.


CHANGELOG.md, line 73 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Move full stop ('.') to the end of the sentence.

Done.


CHANGELOG.md, line 75 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Move full stop ('.') to the end of the sentence.

Done.


CHANGELOG.md, line 122 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Move full stop ('.') to the end of the sentence.

Done.


DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroup/MSFT_xSQLServerAlwaysOnAvailabilityGroup.psm1, line 371 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to [System.Array] $... (namespace in the name and space before $).

Done.


Tests/TestHelpers/CommonTestHelper.psm1, line 17 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[UInt32]

[System.UInt32]

Done.


Tests/TestHelpers/CommonTestHelper.psm1, line 20 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Mandatory = $false

Remove this. Should not be needed (?). Or should it be mandatory in that parameter set?

Done.


Tests/TestHelpers/CommonTestHelper.psm1, line 22 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[System.String]

Done.


Tests/TestHelpers/CommonTestHelper.psm1, line 49 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Change to [System.Array] $... (namespace in the name and space before $).

Done.


Tests/TestHelpers/CommonTestHelper.psm1, line 51 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Don't think you need to loop. Just do Remove-Module -Name $otherStubModules?

PS J:\> Get-Module

ModuleType Version    Name                                ExportedCommands
---------- -------    ----                                ----------------
Script     3.3.1      AzureRM.Backup                      {Backup-AzureRmBackupItem, Disable-AzureRmBackupProtection...
Script     3.3.1      AzureRM.Batch                       {Disable-AzureBatchAutoScale, Disable-AzureBatchComputeNod...
Manifest   3.1.0.0    Microsoft.PowerShell.Management     {Add-Computer, Add-Content, Checkpoint-Computer, Clear-Con...
Manifest   3.1.0.0    Microsoft.PowerShell.Utility        {Add-Member, Add-Type, Clear-Variable, Compare-Object...}
Script     1.2        PSReadline                          {Get-PSReadlineKeyHandler, Get-PSReadlineOption, Remove-PS...


PS J:\> $a = @('AzureRM.Backup','AzureRM.Batch')
PS J:\> Remove-Module -Name $a -Verbose
VERBOSE: Performing the operation "Remove-Module" on target "Microsoft.Azure.Commands.AzureBackup (Path:
'C:\Users\johlju\Documents\WindowsPowerShell\Modules\AzureRM.Backup\3.3.1\Microsoft.Azure.Commands.AzureBackup.dll')".
VERBOSE: Performing the operation "Remove-Module" on target "AzureRM.Backup (Path:
'C:\Users\johlju\Documents\WindowsPowerShell\Modules\AzureRM.Backup\3.3.1\AzureRM.Backup.psm1')".
VERBOSE: Performing the operation "Remove-Module" on target "Microsoft.Azure.Commands.Batch (Path:
'C:\Users\johlju\Documents\WindowsPowerShell\Modules\AzureRM.Batch\3.3.1\Microsoft.Azure.Commands.Batch.dll')".
VERBOSE: Performing the operation "Remove-Module" on target "AzureRM.Batch (Path:
'C:\Users\johlju\Documents\WindowsPowerShell\Modules\AzureRM.Batch\3.3.1\AzureRM.Batch.psm1')".
VERBOSE: Removing the imported "Reactivate-AzureBatchTask" alias.
VERBOSE: Removing the imported "Get-AzureRmBatchSubscriptionQuotas" alias.

Good catch!
Done


Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1, line 58 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe a blank row before this row?

Done.


Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1, line 82 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe a blank row before this row?

Done.


Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1, line 111 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe a blank row before this row?

Done.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Oct 7, 2017

Reviewed 3 of 4 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroup/MSFT_xSQLServerAlwaysOnAvailabilityGroup.psm1, line 371 at r4 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Done.

Please add a space before $

[System.Array] $submittedParameters...

Tests/TestHelpers/CommonTestHelper.psm1, line 49 at r4 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

Done.

Please add a space before $

[System.Array] $otherStubModules...

Comments from Reviewable

@johlju
Copy link
Member

johlju commented Oct 7, 2017

@randomnote1 Changed the PR description for issues fixed to "Partially #517" so the issue doesn't get auto-closed on merge. "Resolves", "Fixes" triggers auto-close and my English isn't good enough to come up with a word that does not trigger auto-close 😄

@randomnote1
Copy link
Contributor Author

How about "Fixes some of" or "Resolves part of"?

@randomnote1
Copy link
Contributor Author

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


DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroup/MSFT_xSQLServerAlwaysOnAvailabilityGroup.psm1, line 371 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a space before $

[System.Array] $submittedParameters...

Done.


Tests/TestHelpers/CommonTestHelper.psm1, line 49 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a space before $

[System.Array] $otherStubModules...

Done.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Oct 8, 2017

Ah good suggestions! Both of them sounds good. Changed to 'Fixes some of' so it looks similar to the other ones. But I think "Resolves part of" sounds better :)
Either way, it's more clear now, what it does. Thanks!


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


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Oct 8, 2017

Can you please rebase this one since I merged another PR.


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


Comments from Reviewable

@randomnote1
Copy link
Contributor Author

@johlju, I'm not sure if you saw, this PR has been rebased and is ready for your review.

@johlju
Copy link
Member

johlju commented Oct 9, 2017

@randomnote1 No, I missed that sorry :/

@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 9, 2017
@johlju
Copy link
Member

johlju commented Oct 9, 2017

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 83 at r8 (raw file):

    Group is created.
  - Use the new helper function "Test-ClusterPermissions".
  - Refactored the unit tests to allow them to be more user friendly and to test

Seems there is changes here that should be removed? Line 83-86?


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 9, 2017
@randomnote1
Copy link
Contributor Author

No worries! A lot of stuff came in all at the same time. Very easy to miss something.


Review status: all files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 83 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Seems there is changes here that should be removed? Line 83-86?

Done.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Oct 9, 2017

Reviewed 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 47 at r9 (raw file):

      environment variable in the (LCM) PowerShell session did not contain the new
      module path.
  - Added new helper function "Test-ClusterPermissions" ([issue #446](https://github.com/PowerShell/xSQLServer/issues/446).

A parenthesis was lost at the end here. (Sorry missed that in previous review)


Comments from Reviewable

@randomnote1
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


CHANGELOG.md, line 47 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

A parenthesis was lost at the end here. (Sorry missed that in previous review)

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 9, 2017
@johlju
Copy link
Member

johlju commented Oct 9, 2017

:lgtm:


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


Comments from Reviewable

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

johlju commented Oct 9, 2017

Waiting for the tests to pass before merging.

@johlju johlju merged commit 1bbdeca into dsccommunity:dev Oct 9, 2017
@randomnote1 randomnote1 deleted the xSQLServerAlwaysOnAvailabilityGroupFQDN branch October 9, 2017 18:36
@johlju johlju mentioned this pull request Jan 5, 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