xSQLServerRSConfig: Replaced sqlcmd.exe with Invoke-Sqlcmd

[bozho]

Pull Request (PR) description
Replaced two sqlcmd.exe executions with Invoke-Sqlcmd calls.

A small code cleanup as the result of the above change.

Removed the conditional use of $RSConfig.MachineAccountIdentity as
$RSSvcAccountUsername, since it was always empty (tested on SQL2014
only). Need to double-check this.

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

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 

[bozho]

Hi @johlju, here's the first PR for xSQLServerRSConfig :)

As I've mentioned in the commit message, I've removed a bit of code that conditionally changed $RSSvcAccountUsername to $RSConfig.MachineAccountIdentity (lines 139-142). I found that $RSConfig.MachineAccountIdentity is empty on two of my SQL Server 2014 test machines (one with configured SSRS, and the other one with a fresh SQL/SSRS install, where SSRS has not been configured yet)

Please let me know if that code is needed (if it is, we'll still have to handle the scenario where $RSConfig.MachineAccountIdentity is empty.

I'll update CHANGELOG.md shortly.

[codecov-io]

Codecov Report

Merging #571 into dev will increase coverage by <1%.
The diff coverage is 0%.

@@         Coverage Diff          @@
##            dev   #571    +/-   ##
====================================
+ Coverage    80%    81%   +<1%     
====================================
  Files        32     32            
  Lines      3488   3462    -26     
====================================
  Hits       2808   2808            
+ Misses      680    654    -26

[johlju]

First! Big thanks for creating the issues and sending in this PR! Awesome work! 😃

I rearranged the PR description a bit. Mostly to add 'Fixes...' so that the issue will auto-close on merge. :)

Could you add an issue for '$RSConfig.MachineAccountIdentity is always empty on SQL2014' and suggest that is replaced in the way you replaced it? Also add to the PR description that this PR fixes that issue.
Could you provide me the configuration for which you tested installation on SQL2014? With it I could verify if the same problem exists on SQL2008R2, SQL2012 and SQL2016 in my lab. Then I know I use the same config as you.

This resource lacks Pester unit tests, I would really like unit test for at least the code that is changed. Would you be up to write those? Otherwise I would be more than happy to help with that. 😄

[bozho]

Regarding $RSConfig.MachineAccountIdentity, I simply removed that bit of code and $RSSvcAccountUsername will be the one used to run SSRS service.

Reading MS docs here: https://docs.microsoft.com/hr-hr/sql/reporting-services/wmi-provider-library-reference/configurationsetting-property-machineaccountidentity

It looks like the property holds the machine's domain account and my test machine is not in a domain - which would explain $RSConfig.MachineAccountIdentity being empty.

It's simple enough to make $RSSvcAccountUsername = $RSConfig.MachineAccountIdentity conditional (if $RSConfig.MachineAccountIdentity is not empty), but I still wonder why do we need to assign SSRS rights to the machine domain account if SSRS is run under LocalSystem or one of the service accounts (e.g. NT Service\ReportServer)

[bozho]

Regarding Pester tests, I've never used Pester, so it might be faster if you wrote at least some initial tests. I promise I'll start learning it :)

[johlju]

I have to agree, it is very odd chaning user to MachineAccountIdentity. I find no references to that one should use that approach in any way. Only reference I found that could be close is for SQL2005 with IIS5.0 which we cannot support.
There is no mention about it here in possible accounts.
https://docs.microsoft.com/en-us/sql/reporting-services/install-windows/configure-the-report-server-service-account-ssrs-configuration-manager

So I would say it should not be necessary. If it is, then hopefully we get an issue for it and it can be documented appropriately.

[johlju]

I will look into fixing Pester tests for this PR. 😄 Is it okay if you enable "Allow edits from maintainers"? You find it to the right, somewhere under 'Notifications'. This way I can push code directly into your PR.

[bozho]

Ok, I'll keep the change that removes $RSConfig.MachineAccountIdentity, then.

I've checked "Allow edits from maintainers" on all three PRs.

[johlju]

I will start with the tests for this as soon as I sent in the PR I'm working on now. So later tonight or tomorrow.

[johlju]

@bozho My suggestion that you could make several PR's, and each having the previous PR's commits, back fired a bit now. It's making it hard to do a test, I don't want to do test code for something that should not be there in the end. 😄 And I don't like the idea to merge something that is not tested.
Sorry for messing things up. 😞 But to resolve this I suggest the following.

It seems #575 has all code from #571 and #573, am i correct? If so, then I think we need to do the following. Let me know if you think differently.

1. If you can change the PR title for PR xSQLServerRSConfig: Virtual directory creation for SQL 2016 #575 to include the other two's PR title (xSQLServerRSConfig: Replaced sqlcmd.exe with Invoke-Sqlcmd #571 and BREAKING CHANGE: xSQLServerRSConfig: remove $SQLAdminCredential parameter #573), and start the PR title with BREAKING CHANGE.
2. Change the PR description of PR xSQLServerRSConfig: Virtual directory creation for SQL 2016 #575 and reference the other two PR's (xSQLServerRSConfig: Replaced sqlcmd.exe with Invoke-Sqlcmd #571 and BREAKING CHANGE: xSQLServerRSConfig: remove $SQLAdminCredential parameter #573) (since we have comment history in PR's xSQLServerRSConfig: Replaced sqlcmd.exe with Invoke-Sqlcmd #571 and BREAKING CHANGE: xSQLServerRSConfig: remove $SQLAdminCredential parameter #573).
3. Change the PR description to include all issues that will be fixed by PR xSQLServerRSConfig: Virtual directory creation for SQL 2016 #575.
4. We close PR xSQLServerRSConfig: Replaced sqlcmd.exe with Invoke-Sqlcmd #571 and BREAKING CHANGE: xSQLServerRSConfig: remove $SQLAdminCredential parameter #573

After that we continue work on #575 so it gets merged.
And while you do that I will do the unit test for the code in #575.

Sorry again 😞

[bozho]

Well, the PRs are all compared against PowerShell:dev branch. I suspect that if we wrap up #571 first and merge it into dev, #573 differences should "shrink" to those between #571 and #573. If that works, the same should apply to #573 / #575 differences...

Would you like to try it that way?

I understand your concerns about testing, but I'm not even sure how would you unit-test #571 or #573. Testing sqlcmd replacing seems more like an integration test material :)

xSQLServerRSConfig doesn't have any tests now anyway. We can probably come up with unit tests for #575 and #570 (and its future PR)

[johlju]

Okay, lets continue the original plan. But I will make the unit tests for #575 (half way there). And in #575 i would like to do some style changes as well before we continue, if you don't mind. :)

I will review this one so it can get merged.

[johlju]

This is partially finished. Work continues in #573 and #575. Tests will be added to the code in PR #575

---

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[johlju]

I did manually integration test (yes we need real integration tests 😉 ) and this code works flawless of what I can see.

Awesome work @bozho!! 😄