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.

Sign up for GitHub

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

Jump to bottom

SqlSetup: Accounts containing '$' will be able to be used #1060

Merged
merged 7 commits into from
Mar 11, 2018
Merged

SqlSetup: Accounts containing '$' will be able to be used #1060

merged 7 commits into from
Mar 11, 2018

Conversation

johlju
Copy link
Member

@johlju johlju commented Mar 10, 2018 •
edited
Loading

Pull Request (PR) description

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

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

@johlju
Added regression test for issue dsccommunity#1055
35af582
@johlju
Clean up tests for helper function Get-ServiceAccountParameters
907b379
@johlju
Changes to SqlSetup
7aa20f8 
- Now accounts containing '$' will be able to be used for installing
  SQL Server. Although, if the account ends with '$' it is considered a
  Managed Service Account (issue dsccommunity#1055).
@johlju johlju added the needs review The pull request needs a code review. label Mar 10, 2018
@codecov-io
Copy link

codecov-io commented Mar 10, 2018 •
edited
Loading

Codecov Report

Merging #1060 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff          @@
##            dev   #1060   +/-   ##
====================================
  Coverage    97%     97%           
====================================
  Files        32      32           
  Lines      3930    3930           
====================================
  Hits       3844    3844           
  Misses       86      86

@johlju
Copy link
Member Author

johlju commented Mar 10, 2018

Closing and reopening this PR to kick off the tests again.

@johlju johlju closed this Mar 10, 2018
@johlju johlju reopened this Mar 10, 2018
@johlju
Copy link
Member Author

johlju commented Mar 11, 2018

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

README.md, line 1483 at r2 (raw file):

have

has

Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Mar 11, 2018

:LGTM:

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

Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Mar 11, 2018

:LGTM:

Apparently there was a new version markdown lint checker which caught a few errors it didn't before. Those are resolved now and the tests passed.

Review status: 3 of 4 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Mar 11, 2018

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

Comments from Reviewable

@johlju johlju merged commit be8f2ec into dsccommunity:dev Mar 11, 2018
@johlju johlju mentioned this pull request Mar 11, 2018
Closed
@johlju johlju deleted the fix-issue-1055 branch March 11, 2018 17:09
@johlju johlju removed the needs review The pull request needs a code review. label Mar 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Install fails when using a Domain Service account that begins with $
2 participants
@johlju @codecov-io