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

xSQLServerMemory: Make Cluster Aware #896

Merged
merged 3 commits into from
Oct 31, 2017
Merged

xSQLServerMemory: Make Cluster Aware #896

merged 3 commits into from
Oct 31, 2017

Conversation

randomnote1
Copy link
Contributor

@randomnote1 randomnote1 commented Oct 30, 2017

Pull Request (PR) description
Made the resource cluster aware. When ProcessOnlyOnActiveNode is specified, the resource will only determine if a change is needed if the target node is the active host of the SQL Server instance

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

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 30, 2017

Codecov Report

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

Impacted file tree graph

@@         Coverage Diff          @@
##            dev   #896    +/-   ##
====================================
+ Coverage    96%    96%   +<1%     
====================================
  Files        32     32            
  Lines      3498   3508    +10     
====================================
+ Hits       3378   3388    +10     
  Misses      120    120

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

johlju commented Oct 30, 2017

Awesome work again. Sorry for not having reviewed until now. I migrated my lab Exchange 2010 to Exchange 2016 and that took the entire weekend. :)

Just a few simple review comments. 😄


Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.


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

Instance

Can we make it a lower-case 'I' in 'instance'?


DSCResources/MSFT_xSQLServerMemory/MSFT_xSQLServerMemory.psm1, line 79 at r1 (raw file):

Instance

Can we make it a lower-case 'I' in 'instance'?


DSCResources/MSFT_xSQLServerMemory/MSFT_xSQLServerMemory.psm1, line 208 at r1 (raw file):

Instance

Can we make it a lower-case 'I' in 'instance'?


DSCResources/MSFT_xSQLServerMemory/MSFT_xSQLServerMemory.schema.mof, line 10 at r1 (raw file):

Instance

Can we make it a lower-case 'I' in 'instance'?


Examples/Resources/xSQLServerMemory/2-SetMaxMemoryToAuto.ps1, line 9 at r1 (raw file):

Instance

Can we make it a lower-case 'I' in 'instance'?


Tests/Unit/MSFT_xSQLServerMemory.Tests.ps1, line 345 at r1 (raw file):

Should call...

'Should not call...'


Tests/Unit/MSFT_xSQLServerMemory.Tests.ps1, line 351 at r1 (raw file):

Should call...

'Should not call...'


Tests/Unit/MSFT_xSQLServerMemory.Tests.ps1, line 357 at r1 (raw file):

Should call...

'Should not call...'


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

That sounds like loads of fun!


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


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

Previously, johlju (Johan Ljunggren) wrote…

Instance

Can we make it a lower-case 'I' in 'instance'?

Done.


DSCResources/MSFT_xSQLServerMemory/MSFT_xSQLServerMemory.psm1, line 79 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Instance

Can we make it a lower-case 'I' in 'instance'?

Done.


DSCResources/MSFT_xSQLServerMemory/MSFT_xSQLServerMemory.psm1, line 208 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Instance

Can we make it a lower-case 'I' in 'instance'?

Done.


DSCResources/MSFT_xSQLServerMemory/MSFT_xSQLServerMemory.schema.mof, line 10 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Instance

Can we make it a lower-case 'I' in 'instance'?

Done.


Examples/Resources/xSQLServerMemory/2-SetMaxMemoryToAuto.ps1, line 9 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Instance

Can we make it a lower-case 'I' in 'instance'?

Done.


Tests/Unit/MSFT_xSQLServerMemory.Tests.ps1, line 345 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should call...

'Should not call...'

Done.


Tests/Unit/MSFT_xSQLServerMemory.Tests.ps1, line 351 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should call...

'Should not call...'

Done.


Tests/Unit/MSFT_xSQLServerMemory.Tests.ps1, line 357 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should call...

'Should not call...'

Done.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Oct 31, 2017

:lgtm:

Yes it was. It was long over due to migrate too. 😄

Awesome work again!


Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@johlju johlju merged commit 61862e7 into dsccommunity:dev Oct 31, 2017
@vors vors removed the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Oct 31, 2017
@randomnote1 randomnote1 deleted the xSQLServerMemoryMakeClusterAware branch October 31, 2017 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xSQLServerMemory: Should Be Cluster Aware
4 participants