SQLRS: Various improvements

randomnote1 commented 2 years ago

Pull Request (PR) description

Added Power BI Report Server support
Added the ability to change the service account
Added the ability to define the database name

This Pull Request (PR) fixes the following issues

Fixes #148
Fixes #149
Fixes #587
Fixes #626
Fixes #860
Fixes #936

Task list

[x] Added an entry to the change log under the Unreleased section of the file CHANGELOG.md. Entry should say what was changed and how that affects users (if applicable), and reference the issue being resolved (if applicable).
[x] Resource documentation updated in the resource's README.md.
[x] Resource parameter descriptions updated in schema.mof.
[x] Comment-based help updated, including parameter descriptions.
[x] Localization strings updated.
[x] Examples updated.
[x] Unit tests updated. See DSC Community Testing Guidelines.
[ ] Integration tests updated (where possible). See DSC Community Testing Guidelines.
[x] Code changes adheres to DSC Community Style Guidelines.

This change is

codecov[bot] commented 2 years ago

Codecov Report

Merging #1758 (04685ba) into main (c780423) will decrease coverage by 7%. The diff coverage is 35%.

:exclamation: Current head 04685ba differs from pull request most recent head 3b225a3. Consider uploading reports for the commit 3b225a3 to get more accurate results

@@          Coverage Diff           @@
##           main   #1758     +/-   ##
======================================
- Coverage    92%     85%     -7%     
======================================
  Files        85      36     -49     
  Lines      7640    6490   -1150     
======================================
- Hits       7038    5533   -1505     
- Misses      602     957    +355

Flag	Coverage Δ
unit	`85% <35%> (-7%)`	:arrow_down:

Impacted Files	Coverage Δ
source/DSCResources/DSC_SqlRS/DSC_SqlRS.psm1	`40% <35%> (-60%)`	:arrow_down:
...dules/SqlServerDsc.Common/SqlServerDsc.Common.psm1	`98% <100%> (-1%)`	:arrow_down:

... and 49 files with indirect coverage changes

johlju commented 2 years ago

I'm waiting for the build workers to get the Windows patch for DSC so the integration tests can run. I will review then.

randomnote1 commented 2 years ago

Looks like my Azure VMs started working again last week, so, hopefully soon!

johlju commented 2 years ago

/AzurePipelines run

azure-pipelines[bot] commented 2 years ago

Azure Pipelines successfully started running 1 pipeline(s).

johlju commented 2 years ago

@randomnote1 the integration test runs now - there are some failing tests in integration and unit tests. Let me know when they pass and I review.

randomnote1 commented 2 years ago

@johlju, how do we log into the build servers to troubleshoot what is wrong? I recall seeing something somewhere, but I don't remember where!

johlju commented 2 years ago

It is not possible with the Azure DevOps build servers. It was possible way back when we used AppVeyor. I usually add debug verbose messages to see how far it comes before it throws to locate the line that fails. Not optimal, but often only way to find the issue if unit tests don’t catch it.

randomnote1 commented 2 years ago

/AzurePipelines run

azure-pipelines[bot] commented 2 years ago

Commenter does not have sufficient privileges for PR 1758 in repo dsccommunity/SqlServerDsc

randomnote1 commented 2 years ago

@johlju, looks like the only failure in the pipelines was well before SqlRs was attempted. Can you re-start the pipeline to verify?

johlju commented 2 years ago

Restarted.

johlju commented 2 years ago

AzurePipelines run

johlju commented 2 years ago

Restart all jobs again because SQL2016 for one job only ran for 10 minutes before failing without stopping the pipeline.

johlju commented 2 years ago

/AzurePipelines run

azure-pipelines[bot] commented 2 years ago

Azure Pipelines successfully started running 1 pipeline(s).

johlju commented 2 years ago

Now both jobs for SQL2016 seems to fail on the same spot.

randomnote1 commented 2 years ago

All right, thanks. I'll continue troubleshooting

randomnote1 commented 2 years ago

@johlju, what's going on with the build right now? I keep getting this from AppVeyor:

The build phase is set to "MSBuild" mode (default), but no Visual Studio project or solution files were found in the root directory. If you are not building Visual Studio project switch build mode to "Script" and provide your custom build command.

johlju commented 2 years ago

@randomnote1 I added Appveyor as a build job for debug purpose. If it helps you. Haven't merged the README.md yet (but as soon as the tests passes), but you can read here: https://github.com/dsccommunity/SqlServerDsc/blob/03f4a281044a950f43f7b7fd25f2234924e5653a/tests/Integration/README.md#debugging

johlju commented 2 years ago

I just merged the appveyor.yml so you won't get the above error.

johlju commented 2 years ago

You have to rebase to get that though. 🙂

randomnote1 commented 2 years ago

Awesome, you're the man! Thanks!

johlju commented 2 years ago

You have to uncomment a row in appveyor.yml to have the build worker online after the tests finishes. I didn't want to do that by default since for each commit it would hold up other builds that use AppVeyor still.

johlju commented 2 years ago

Saw that it doesn't work to add $blockRdp = $true to the init: step. Must add this in appveyor.yml to paus the build at the end of the run. I will update the file on main-branch.

on_finish:
  - ps: $blockRdp = $true; iex ((new-object net.webclient).DownloadString('https://raw.githubusercontent.com/appveyor/ci/master/scripts/enable-rdp.ps1'))

johlju commented 2 years ago

@randomnote1 how does it go finding the cause of the failing tests? Would of course be great if this worked for all versions , but if you see that there is an issue getting this to work for the Reporting Services 2016 should we just add logic that if it is 2016 then it uses the previous (simpler) functionality? Then we could try to get the SQL 2016 moved to new functionality in a new PR? Just a suggestion to unblock you, you decide. Let me know if I can help with anything.

randomnote1 commented 2 years ago

I found where and why the issue is occurring. I just haven't had time to think through resolutions yet.

johlju commented 2 years ago

@randomnote1 you need to rebase this PR so it runs the tests again. 🙂

randomnote1 commented 2 years ago

Yup, working on it. The changes to the rebase conflict resolution in VS Code is weird. Probably gonna have to manually fix the ChangeLog

johlju commented 2 years ago

@randomnote1 was hoping to get in this PR before release next major release. Do you think it is gonna take a while to get the tests to pass (to fix this issue) so I should just release a new version?

randomnote1 commented 2 years ago

I think I'm going to just say anything older than SQL 2017 is unsupported for this resource. I'll work on getting that in today.

randomnote1 commented 2 years ago

@johlju, I think it's finally good to go. Let me know if you find anything in review.

randomnote1 commented 2 years ago

source/DSCResources/DSC_SqlRS/DSC_SqlRS.psm1 line 190 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Currently this returns only the filename, not the full path + the file name. If this would be set to the full path then we could re-use that value in Set-function instead of calculating the path there again?

This probably would have worked better before I split out the encryption key backup to a different function. Leaving as-is for now.

randomnote1 commented 2 years ago

@johlju, I reverted the changes dealing with enforcing default parameters. I don't have the cycles right now to test these changes. It looks like they'll be a bit more involved than what I initially thought, especially for the first-time initialization. Can we merge the changes now and open issues to address the enforcing of default values at a later time?

johlju commented 2 years ago

I think I'm going to just say anything older than SQL 2017 is unsupported for this resource. I'll work on getting that in today.

I rather see that we not exclude SQL2016 since it still have several years left on its lifecycle. It is okay if these new changes does not work on 2016, but can we have two code paths? 🤔 One code path with the old code prior to this PR that will be used by SQL2016, and then a new code path with the new code that is using this new code - also old code path could throw an unsupported message if SQL2016 is used with one of the new parameters. That way SQL2016 still works (at least as good/bad as it did before) and other PRs can suggest to make the new code work with SQL2016, or we just remove the SQL2016 code path at the end of its lifecycle.

randomnote1 commented 2 years ago

That's an interesting idea for the set command. Get and Test should still be ok. In any case, that'll take a good bit of testing and modifications of the unit tests, particularly declaring how many times each mock is executed.

johlju commented 2 years ago

We can just split upp the tests in two code paths to. Or two describe/context block. 🤔What ever is easiest.

github-actions[bot] commented 2 years ago

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

johlju commented 1 year ago

I think we need to simplify these changes by splitting them up in several PR that add each new functionality instead of this massive PR. 🤔

github-actions[bot] commented 1 year ago

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

dsccommunity / SqlServerDsc