-
Notifications
You must be signed in to change notification settings - Fork 225
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
SqlServerLogin: Fix password test fails for nativ sql users #1049
SqlServerLogin: Fix password test fails for nativ sql users #1049
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1049 +/- ##
=====================================
+ Coverage 97% 97% +<1%
=====================================
Files 32 32
Lines 3925 3930 +5
=====================================
+ Hits 3839 3844 +5
Misses 86 86 |
@claudiospizzi Thanks for sending in a PR for this! I see Codecov reporting misses in code coverage. Is that false negatives? If you run |
Reviewed 3 of 4 files at r1. a discussion (no related file): SqlServerDscHelper.psm1, line 70 at r1 (raw file):
Please add a blank row before this one. SqlServerDscHelper.psm1, line 76 at r1 (raw file):
Please add a blank row before this one. DSCResources/MSFT_SqlServerLogin/MSFT_SqlServerLogin.psm1, line 425 at r1 (raw file):
I don't think this is needed? It can never be hit since line 398 only allows SqlLogin in this code path, and we can't change password for Windows accounts? Comments from Reviewable |
This should be tested in the integration tests, since I'm not currently enforcing changing the integration tests, I added an issue #1050 to track this. @claudiospizzi If you are up to adding this test you are more than welcome to do so, but I will merge this PR without it if you feel that you don't have time, or if it is to new for you. |
@johlju |
Reviewed 3 of 3 files at r2. Comments from Reviewable |
Pull Request (PR) description
Bug in the Connect-SQL helper function, the resource is not able to test a nativ SQL user password. It tries to test the native SQL user password like a Windows login, which always fails.
This Pull Request (PR) fixes the following issues:
Fixes #1048
Task list:
This change is