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

xSQLServerRole: Fix for Issue #790 #791

Merged
merged 16 commits into from Sep 7, 2017
Merged

xSQLServerRole: Fix for Issue #790 #791

merged 16 commits into from Sep 7, 2017

Conversation

seizste
Copy link
Contributor

@seizste seizste commented Sep 5, 2017

Pull Request (PR) description
Explicit Casting to System.String[] for $membersInRole

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

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

kwirkykat and others added 12 commits August 10, 2016 13:28
Release of version 1.8.0.0 of xSQLServer
Release of version 3.0.0.0 of xSQLServer
Release of version 5.0.0.0 of xSqlServer
Release of version 6.0.0.0 of xSqlServer
Release of version 7.0.0.0 of xSQLServer
Release of version 7.1.0.0 of xSqlServer
Release of version 8.0.0.0 of xSQLServer
Release of version 8.1.0.0 of xSQLServer
@msftclas
Copy link

msftclas commented Sep 5, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@codecov-io
Copy link

codecov-io commented Sep 5, 2017

Codecov Report

Merging #791 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #791   +/-   ##
===================================
  Coverage    97%    97%           
===================================
  Files        30     30           
  Lines      3255   3255           
===================================
  Hits       3170   3170           
  Misses       85     85

@johlju
Copy link
Member

johlju commented Sep 5, 2017

First, thanks for sending this fix in! 😄

Could you please

  • Add this change to the Unreleased section of the CHANGELOG.md.
  • Add a regression test (that fails for the old code, and passes for the new code) in MSFT_xSQLServerRole.Tests.ps1.

@johlju johlju added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Sep 5, 2017
@johlju
Copy link
Member

johlju commented Sep 5, 2017

If you need help with anything, let me know! 😄

@johlju
Copy link
Member

johlju commented Sep 5, 2017

I updated the PR description to use the template. You removed that by mistake when entering you PR description, when you sent in the PR. It's easier for me to have the PR's use the template. But no worries. It was easy fixed. :)

@seizste
Copy link
Contributor Author

seizste commented Sep 6, 2017

Added entry to changelog.md and "Should return the members as string array" Test

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

johlju commented Sep 6, 2017

Just minor review comments! Great work! 😄


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


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

Fixed Error due to Return Variable Type

Please change to something more descriptive. Like "When running Get-DscConfiguration it longer throws an error saying that the property Members is not an array (Issue #790)."


DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 70 at r4 (raw file):

            try
            {
                [System.String[]]$membersInRole = $sqlServerRoleObject.EnumMemberNames()

Please add a space between the type and the variable name

[System.String[]] $membersInRole = ...

Tests/Unit/MSFT_xSQLServerRole.Tests.ps1, line 215 at r4 (raw file):

                }

                It 'Should return the members as string array' {

Could you please add a comment saying that this is a regression test?

# Regression test for issue #790 

Please do this for each of these new tests


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 Sep 6, 2017
@seizste
Copy link
Contributor Author

seizste commented Sep 6, 2017

Did the changes you mentioned in the review

@seizste
Copy link
Contributor Author

seizste commented Sep 6, 2017

i'm new to "code-review/reviewable", how can I acknowledge the changes you mentioned in the discussion ?

@johlju
Copy link
Member

johlju commented Sep 6, 2017

Could you please go into Reviewable and write 'Done' (or click the Done-button) on each review comment you solved (or comment if it needs to be discussed). After you replied on all review comments, then you press the big publish-button at the top of the Reviewable page and all your comments will be sent back here.
When you do this I can acknowledge the comments which will resolve the reviews comments.

You find the link to Reviewable in the big purple button in the PR description.

@seizste
Copy link
Contributor Author

seizste commented Sep 6, 2017

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

Previously, johlju (Johan Ljunggren) wrote…

Fixed Error due to Return Variable Type

Please change to something more descriptive. Like "When running Get-DscConfiguration it longer throws an error saying that the property Members is not an array (Issue #790)."

Done.


Comments from Reviewable

@seizste
Copy link
Contributor Author

seizste commented Sep 6, 2017

DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 70 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a space between the type and the variable name

[System.String[]] $membersInRole = ...

Done.


Comments from Reviewable

@seizste
Copy link
Contributor Author

seizste commented Sep 6, 2017

Tests/Unit/MSFT_xSQLServerRole.Tests.ps1, line 215 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please add a comment saying that this is a regression test?

# Regression test for issue #790 

Please do this for each of these new tests

Done, Added comment in front of Regression Test


Comments from Reviewable

@seizste
Copy link
Contributor Author

seizste commented Sep 6, 2017

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


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

johlju commented Sep 7, 2017

:lgtm:


Reviewed 3 of 3 files at r5.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@johlju johlju merged commit de6a25b into dsccommunity:dev Sep 7, 2017
@vors vors removed the needs review The pull request needs a code review. label Sep 7, 2017
@johlju
Copy link
Member

johlju commented Sep 7, 2017

@seizste Awesome work! Thanks for this! 😄

@seizste
Copy link
Contributor Author

seizste commented Sep 8, 2017

@johlju Thanks for your help and guidance

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.

xSQLServerRole: Returns an error when calling Get-DscConfiguration
6 participants