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.

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

SQLRS: Various improvements #1758

Open
wants to merge 68 commits into
base: main
Choose a base branch
from

Conversation

randomnote1
Copy link
Contributor

@randomnote1 randomnote1 commented Jun 14, 2022

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

Task list

  • 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).
  • Resource documentation updated in the resource's README.md.
  • Resource parameter descriptions updated in schema.mof.
  • Comment-based help updated, including parameter descriptions.
  • Localization strings updated.
  • Examples updated.
  • Unit tests updated. See DSC Community Testing Guidelines.
  • Integration tests updated (where possible). See DSC Community Testing Guidelines.
  • Code changes adheres to DSC Community Style Guidelines.

This change is Reviewable

@randomnote1 randomnote1 reopened this Jun 14, 2022
@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

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

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

Impacted file tree graph

@@          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%) ⬇️
Impacted Files Coverage Δ
source/DSCResources/DSC_SqlRS/DSC_SqlRS.psm1 40% <35%> (-60%) ⬇️
...dules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 98% <100%> (-1%) ⬇️

... and 49 files with indirect coverage changes

@randomnote1 randomnote1 marked this pull request as ready for review June 23, 2022 01:20
Copy link
Contributor Author

@randomnote1 randomnote1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion


source/DSCResources/DSC_SqlRS/DSC_SqlRS.psm1 line 1091 at r1 (raw file):

                    throw $_
                }
            }

I couldn't figure out how to mock Invoke-RsCimMethod to throw an error the first time and work the second time.

Code quote:

            catch [System.Management.Automation.RuntimeException]
            {
                if ( $_.Exception -match 'The report server was unable to validate the integrity of encrypted data in the database' )
                {
                    # Restore key here
                    $invokeRsCimMethodRestoreEncryptionKeyParameters = @{
                        CimInstance = $reportingServicesData.Configuration
                        MethodName  = 'RestoreEncryptionKey'
                        Arguments   = @{
                            KeyFile = $backupEncryptionKeyResult.KeyFile
                            Length = $restoreEncryptionKeyResult.Length
                            Password = $EncryptionKeyBackupCredential.GetNetworkCredential().Password
                        }
                    }

                    $restoreEncryptionKeyResult = Invoke-RsCimMethod @invokeRsCimMethodRestoreEncryptionKeyParameters

                    if ( $restoreEncryptionKeyResult.HRESULT -eq 0 )
                    {
                        # Finally, try and initialize the server again
                        Invoke-RsCimMethod @invokeRsCimMethodInitializeReportServerParameters
                    }
                }
                else
                {
                    throw $_
                }
            }

@johlju johlju added the needs review The pull request needs a code review. label Jun 23, 2022
@johlju
Copy link
Member

johlju commented Jul 19, 2022

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
Copy link
Contributor Author

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

@johlju
Copy link
Member

johlju commented Jul 27, 2022

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@johlju
Copy link
Member

johlju commented Jul 27, 2022

@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
Copy link
Contributor Author

@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
Copy link
Member

johlju commented Jul 27, 2022

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.

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @johlju and @randomnote1)


source/DSCResources/DSC_SqlRS/DSC_SqlRS.psm1 line 1091 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

I couldn't figure out how to mock Invoke-RsCimMethod to throw an error the first time and work the second time.

I think it might be better to do a do-while-loop aroudn the entire try-catch-block? While $initialized -eq $false it tries to call Invoke-RsCimMethod? Then you don't have to add a second call to Invoke-RsCimMethod inside the catch block.
Suggest that you also add a variable $restoreKey and if that is true (default $false, and set to $true by catch block) then on next loop it tries to restore the key outside of the catch block.

You can mock it by adding a script variable inside the mock, e.g.

BeforeAll {
    Mock -CommandName Invoke-RsCimMethod -ParameterFilter {
        $MethodName -eq 'InitializeReportServer'
    } -MockWith {
        $script:callInvokeRsCimMethodCount += 1
    
        if ($script:callInvokeRsCimMethodCount -eq 2)
        { 
             throw 'terminating error'
        }
        else 
        {
            throw 'exception to try again' 
        }
    }
}

BeforeEach {
    $script:callInvokeRsCimMethodCount = 0
}

@johlju johlju self-requested a review July 28, 2022 07:58
@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jul 28, 2022
Copy link
Contributor Author

@randomnote1 randomnote1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 12 files reviewed, all discussions resolved (waiting on @johlju)


source/DSCResources/DSC_SqlRS/DSC_SqlRS.psm1 line 1091 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I think it might be better to do a do-while-loop aroudn the entire try-catch-block? While $initialized -eq $false it tries to call Invoke-RsCimMethod? Then you don't have to add a second call to Invoke-RsCimMethod inside the catch block.
Suggest that you also add a variable $restoreKey and if that is true (default $false, and set to $true by catch block) then on next loop it tries to restore the key outside of the catch block.

You can mock it by adding a script variable inside the mock, e.g.

BeforeAll {
    Mock -CommandName Invoke-RsCimMethod -ParameterFilter {
        $MethodName -eq 'InitializeReportServer'
    } -MockWith {
        $script:callInvokeRsCimMethodCount += 1
    
        if ($script:callInvokeRsCimMethodCount -eq 2)
        { 
             throw 'terminating error'
        }
        else 
        {
            throw 'exception to try again' 
        }
    }
}

BeforeEach {
    $script:callInvokeRsCimMethodCount = 0
}

Done

@randomnote1
Copy link
Contributor Author

/AzurePipelines run

@azure-pipelines
Copy link

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

@randomnote1
Copy link
Contributor Author

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

@johlju
Copy link
Member

johlju commented Jul 28, 2022

Restarted.

@johlju
Copy link
Member

johlju commented Jul 28, 2022

AzurePipelines run

@johlju
Copy link
Member

johlju commented Jul 28, 2022

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

@johlju
Copy link
Member

johlju commented Jul 28, 2022

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@johlju
Copy link
Member

johlju commented Jul 29, 2022

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

@randomnote1
Copy link
Contributor Author

All right, thanks. I'll continue troubleshooting

@johlju
Copy link
Member

johlju commented Mar 26, 2023

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 github-actions bot removed the abandoned The pull request has been abandoned. label Mar 27, 2023
@github-actions
Copy link

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.

@github-actions github-actions bot added the abandoned The pull request has been abandoned. label Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned The pull request has been abandoned. waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author.
Projects
None yet
2 participants