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

xSQLServerRSConfig: Remove parameter SQLAdminCredential #568

Closed

bozho opened this issue May 23, 2017 · 6 comments

Labels

Contributor

bozho commented May 23, 2017 •

edited by johlju

Loading

There is no need for the $SQLAdminCredential parameter to be mandatory. Remove the restriction.

Update: In pr #573 it was decided to remove SQLAdminCredential. PR title changed accordingly,

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] xSQLServerRSConfig: $SQLAdminCredential is now opt…

3d5939b

…ional

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] xSQLServerRSConfig: $SQLAdminCredential is now opt…

35e5edd

…ional

Contributor Author

bozho commented May 24, 2017

Ok, I've made the changes to make $SQLAdminCredential optional, but I have two problems:

I'd like to fix issues xSQLServerRSConfig: Replace sqlcmd.exe with Invoke-Sqlcmd calls #567, xSQLServerRSConfig: Remove parameter SQLAdminCredential #568, xSQLServerRSConfig: Virtual directory creation for SQL 2016 #569 and xSQLServerRSConfig: Configurable URL reservations #570 in order (xSQLServerRSConfig: Remove parameter SQLAdminCredential #568 branch off xSQLServerRSConfig: Replace sqlcmd.exe with Invoke-Sqlcmd calls #567, xSQLServerRSConfig: Virtual directory creation for SQL 2016 #569 branch off xSQLServerRSConfig: Remove parameter SQLAdminCredential #568 and so on). If I submit a PR for xSQLServerRSConfig: Remove parameter SQLAdminCredential #568, it is compared to the dev branch in this repository and shows changes from both xSQLServerRSConfig: Replace sqlcmd.exe with Invoke-Sqlcmd calls #567 and xSQLServerRSConfig: Remove parameter SQLAdminCredential #568, which makes it confusing. What would be the best workflow here? Should I wait until xSQLServerRSConfig: Replace sqlcmd.exe with Invoke-Sqlcmd calls #567 is merged and then submit the PR for xSQLServerRSConfig: Remove parameter SQLAdminCredential #568?
We still need to use CredSSP authentication when running Invoke-Sqlcmd with $SQLAdminCredential using Invoke-Command, even though we're running it on the same machine. I don't know how this behaves when run in a domain, but running it on a non-domain machine requires enabling/configuring CredSSP - I'm not sure if we should consider this to be within the remit of this resource, or just document the fact that CredSSP needs to be enabled and configured.

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] xSQLServerRSConfig: $SQLAdminCredential is now opt…

d4397f4

…ional

Member

johlju commented May 24, 2017

three ways of doing this I think, either
1.1 We do and merge each PR in turn before continuing on the next PR.
1.2 You do all changes (all issues) in one PR.
1.3 You branch off to a new working branch from the previous working branch (your suggestion above). When sending in a PR for the branched off PR's, write in the PR description which PR(s) it depends on. I will review and merge them in the correct order.

To have a flow I think we could go with 1.3, what do you think? After the first PR is merged, you can rebase the second PR (always rebase, no pull or merge) which will get the commit history in the right order and then replay each commit onto of that (after any conflicts are resolved).

johlju added enhancement in progress labels

Contributor Author

bozho commented May 24, 2017

Yeah, I agree with 1.3, review in order, rebase before the next PR.

Contributor Author

bozho commented May 24, 2017

I'll create a PR for this issue, since I'd like to discuss $SQLAdminCredential in the code...

bozho mentioned this issue

BREAKING CHANGE: xSQLServerRSConfig: remove $SQLAdminCredential parameter #573

Merged

5 tasks

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] MOF schema fix.

b4014c6

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] We don't need ComputerName when not using $SQLAdmi…

02aba24

…nCredential

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Passing in SQL helper module path as an argument t…

b0b669a

…o Invoke-Command

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Removed $SQLAdminCredential parameter.

ab64212

xSQLServer module aims to drop WMF 4.0 support. WMF 5.0 and later
supports a common parameter (PsDscRunAsCredential) for running DSC
resources under different credentials, which eliminates the need for
the $SQLAdminCredential parameter.

johlju pushed a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] xSQLServerRSConfig: $SQLAdminCredential is now opt…

ba03670

…ional

johlju pushed a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] MOF schema fix.

bccf9a0

johlju pushed a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] We don't need ComputerName when not using $SQLAdmi…

a327850

…nCredential

johlju pushed a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Passing in SQL helper module path as an argument t…

01596a4

…o Invoke-Command

johlju pushed a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Removed $SQLAdminCredential parameter.

fe55875

xSQLServer module aims to drop WMF 4.0 support. WMF 5.0 and later
supports a common parameter (PsDscRunAsCredential) for running DSC
resources under different credentials, which eliminates the need for
the $SQLAdminCredential parameter.

johlju changed the title ~~xSQLServerRSConfig: make $SQLAdminCredential an optional argument~~ xSQLServerRSConfig: Remove parameter SQLAdminCredential

Member

johlju commented May 27, 2017

I change the title of this issue. Since it was discussed in PR #573 that we should remove the parameter SQLAdminCredential

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] xSQLServerRSConfig: $SQLAdminCredential is now opt…

732d29f

…ional

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] MOF schema fix.

a081c0f

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] We don't need ComputerName when not using $SQLAdmi…

6a49d52

…nCredential

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Passing in SQL helper module path as an argument t…

b033a3d

…o Invoke-Command

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Removed $SQLAdminCredential parameter.

b496b93

xSQLServer module aims to drop WMF 4.0 support. WMF 5.0 and later
supports a common parameter (PsDscRunAsCredential) for running DSC
resources under different credentials, which eliminates the need for
the $SQLAdminCredential parameter.

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Updated README.md

d5de40f

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] xSQLServerRSConfig: $SQLAdminCredential is now opt…

d26b1ef

…ional

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] MOF schema fix.

cc051b0

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] We don't need ComputerName when not using $SQLAdmi…

a6acec1

…nCredential

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Passing in SQL helper module path as an argument t…

…o Invoke-Command

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Removed $SQLAdminCredential parameter.

b3b3076

xSQLServer module aims to drop WMF 4.0 support. WMF 5.0 and later
supports a common parameter (PsDscRunAsCredential) for running DSC
resources under different credentials, which eliminates the need for
the $SQLAdminCredential parameter.

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Test fix for CHANGELOG.md

78bcbdc

kewalaka added a commit to kewalaka/xSQLServer that referenced this issue


          fixes dsccommunity#568,dsccommunity#569,dsccommunity#570

ad63ebf

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] xSQLServerRSConfig: $SQLAdminCredential is now opt…

44c9ff1

…ional

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] MOF schema fix.

7c5da18

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] We don't need ComputerName when not using $SQLAdmi…

700011c

…nCredential

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Passing in SQL helper module path as an argument t…

02d0ac9

…o Invoke-Command

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Removed $SQLAdminCredential parameter.

e8f0bdb

xSQLServer module aims to drop WMF 4.0 support. WMF 5.0 and later
supports a common parameter (PsDscRunAsCredential) for running DSC
resources under different credentials, which eliminates the need for
the $SQLAdminCredential parameter.

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Test fix for CHANGELOG.md

c1baaf2

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] CR updates

97c7b3a

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] CR updates

6b45b3a

bozho added a commit to bozho/SqlServerDsc that referenced this issue


          [dsccommunity#568] Added requirements for PsDscRunAsCredential

df1be02

johlju pushed a commit that referenced this issue


          BREAKING CHANGE: xSQLServerRSConfig: remove $SQLAdminCredential param…

…eter (#573)

- Changes to xSQLServerRSConfig
  - BREAKING CHANGE: Removed `$SQLAdminCredential` parameter. Use common parameter PsDscRunAsCredential (WMF 5.0+) to run the resource under different credentials. 
 - PsDscRunAsCredential Windows account must be a sysadmin on SQL Server (issue #568). In addition, the resource no longer uses Invoke-Command cmdlet that was used to impersonate the Windows user specified by $SQLAdminCredential. The call also needed CredSSP authentication to be enabled and configured on the target node, which complicated deployments in non-domain scenarios. Using PsDscRunAsCredential solves this problems for us.

Member

johlju commented Jun 8, 2017

This issue was resolved in merged PR #573. Closing this issue.

johlju closed this as completed

johlju removed the in progress label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment