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

SqlServerDsc: Added SQL Server 2019 Integration Tests (and unit test) plus additional, 'SqlSetup' resource InstallSharedDir fix. #1676

Merged

Conversation

SphenicPaul
Copy link
Contributor

@SphenicPaul SphenicPaul commented Jan 23, 2021

Pull Request (PR) description

Made a number of changes to enable SQL Server 2019 integration tests to be executed as part of the Azure DevOps, pipeline.

Changes include:

  • Amending the SqlServerDsc module, Azure DevOps pipeline (defined in azure-pipelines.yml definition) to add additional "Job" in the "Test" stage to run integration tests for SQL Server 2019.
  • Amended integration scripts for the SqlAgentAlert, SqlAgentFailsafe, SqlAgentOperator, SqlDatabase, SqlDatabaseDefaultLocation, SqlDatabaseMail, SqlDatabaseObjectPermission, SqlDatabasePermission, SqlDatabaseUser, SqlEndpoint, SqlLogin, SqlProtocol, SqlProtocolTcpIp, SqlRS, SqlRSSetup, SqlReplication, SqlRole, SqlScript, SqlScriptQuery, SqlSecureConnection, SqlServiceAccount and SqlSetup resources to support SQL Server 2019 execution.
  • Added functionality (within integration tests) to download SQL Server 2019 ISO via an additional EXE.
  • Updating integration test README.md file to include SQL Server 2019.
  • Updated relevant unit tests, where required.
  • Amended SqlSetup resource to output the correct InstallSharedDir value when used with SQL Server 2019 - This failure/omission was identified by the amended integration tests and resolved/fixed as part of this change.
  • Updated a few other comments to reference SQL Server 2019

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

…check. Also added '150', $script:sqlVersion assignment for SQL2019.
…SQL2019) into SSRS, integration tests and configs.
@codecov
Copy link

codecov bot commented Jan 24, 2021

Codecov Report

Merging #1676 (4aebf11) into main (c392042) will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #1676   +/-   ##
====================================
  Coverage    97%     97%           
====================================
  Files        38      38           
  Lines      6259    6259           
====================================
  Hits       6102    6102           
  Misses      157     157           
Flag Coverage Δ
unit 97% <100%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/DSCResources/DSC_SqlRS/DSC_SqlRS.psm1 100% <ø> (ø)
...dules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 97% <ø> (ø)
source/DSCResources/DSC_SqlSetup/DSC_SqlSetup.psm1 98% <100%> (ø)

@johlju johlju added the needs review The pull request needs a code review. label Jan 24, 2021
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.

Awesome work on these! Just small comments. 😃

Reviewed 33 of 35 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @SphenicPaul)


tests/Integration/DSC_SqlRS.config.ps1, line 26 at r2 (raw file):

Quoted 15 lines of code…
    if($script:sqlVersion -eq '150')
    {
        # SQL2019
        $instanceName = 'SSRS'
        $isoImageName = 'SQL2019.iso'
        # Additional variables required as ISO is downloaded via additional EXE
        $downloadExeName = 'SQL2019_Download.exe'
        $downloadIsoName = 'SQLServer2019-x64-ENU-Dev.iso'
    }
    elseif($script:sqlVersion -eq '140')
    {
        # SQL2017
        $instanceName = 'SSRS'
        $isoImageName = 'SQL2017.iso'
    }

I think this is never used. Looking at the test now I think we should change the "dependencies-block" so this is not needed? See comment below.


tests/Integration/DSC_SqlRS.config.ps1, line 62 at r2 (raw file):

                DownloadExePath      = "$env:TEMP\$downloadExeName"
                DownloadIsoPath      = "$env:TEMP\$downloadIsoName"

We can remove this, it looks like it is never used here.


tests/Integration/DSC_SqlRS.config.ps1, line 93 at r2 (raw file):

Quoted 12 lines of code…
        MountImage 'MountIsoMedia'
        {
            ImagePath   = $Node.ImagePath
            DriveLetter = $Node.DriveLetter
            Ensure      = 'Present'
        }
        WaitForVolume 'WaitForMountOfIsoMedia'
        {
            DriveLetter      = $Node.DriveLetter
            RetryIntervalSec = 5
            RetryCount       = 10
        }

This is only needed if $script:sqlVersion -eq '130'. We could move it into the if-block below right before SqlSetup, then we don't need the ISO's for the other versions in this test. What do you think?


tests/Integration/DSC_SqlRSSetup.Integration.Tests.ps1, line 4 at r2 (raw file):

'Integration_SQL2019'

Maybe we should remove this as it downloads October 2017 executable? See next comment.


tests/Integration/DSC_SqlRSSetup.Integration.Tests.ps1, line 41 at r2 (raw file):

        $script:mockSourceMediaDisplayName = 'Microsoft SQL Server Reporting Services (October 2017)'
        $script:mockSourceMediaUrl = 'https://download.microsoft.com/download/E/6/4/E6477A2A-9B58-40F7-8AD6-62BB8491EA78/SQLServerReportingServices.exe'

We could create and issue to add tests for newer (2019) reporting services. I know there are issues reported in the repos with newer RS when used with the resources, so might not be a quick fix adding an additional exe to download.

Also, maybe we should add separate pipeline jobs to test RS 2017 and newer?

This can be done in another PR. Would you mind creating an issue for it so we can track it?

@johlju johlju added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Jan 24, 2021
…and 'DownloadIsoPath' within 'SqlRS' integration test configuration.
…soMedia' into $script:sqlVersion -eq '130', if-block within 'SqlRS', integration test configuration.
Copy link
Contributor Author

@SphenicPaul SphenicPaul 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: 33 of 35 files reviewed, 5 unresolved discussions (waiting on @johlju)


tests/Integration/DSC_SqlRS.config.ps1, line 26 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
    if($script:sqlVersion -eq '150')
    {
        # SQL2019
        $instanceName = 'SSRS'
        $isoImageName = 'SQL2019.iso'
        # Additional variables required as ISO is downloaded via additional EXE
        $downloadExeName = 'SQL2019_Download.exe'
        $downloadIsoName = 'SQLServer2019-x64-ENU-Dev.iso'
    }
    elseif($script:sqlVersion -eq '140')
    {
        # SQL2017
        $instanceName = 'SSRS'
        $isoImageName = 'SQL2017.iso'
    }

I think this is never used. Looking at the test now I think we should change the "dependencies-block" so this is not needed? See comment below.

Done.


tests/Integration/DSC_SqlRS.config.ps1, line 62 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
                DownloadExePath      = "$env:TEMP\$downloadExeName"
                DownloadIsoPath      = "$env:TEMP\$downloadIsoName"

We can remove this, it looks like it is never used here.

Done.


tests/Integration/DSC_SqlRS.config.ps1, line 93 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
        MountImage 'MountIsoMedia'
        {
            ImagePath   = $Node.ImagePath
            DriveLetter = $Node.DriveLetter
            Ensure      = 'Present'
        }
        WaitForVolume 'WaitForMountOfIsoMedia'
        {
            DriveLetter      = $Node.DriveLetter
            RetryIntervalSec = 5
            RetryCount       = 10
        }

This is only needed if $script:sqlVersion -eq '130'. We could move it into the if-block below right before SqlSetup, then we don't need the ISO's for the other versions in this test. What do you think?

Yes. That makes sense.

I've now moved '[MountImage]MountIsoMedia' and '[WaitForVolume]WaitForMountOfIsoMedia' resource instances/definitions into the '130'-specific, if-block.


tests/Integration/DSC_SqlRSSetup.Integration.Tests.ps1, line 4 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
'Integration_SQL2019'

Maybe we should remove this as it downloads October 2017 executable? See next comment.

Done.

I've removed 'SqlRSSetup' integration tests running as part of 'Integration_SQL2019' build and updated the comment to infer it has been done deliberately (rather than missed accidentally).

This was initially a question I'd raised then deleted as I was a little unclear if SSRS was directly tied into the SQL Server versions or not. Running the same tests for SQL2019 with the same EXE looked like it would provide little, additionally benefit but I'd updated more for consistency rather than anything.


tests/Integration/DSC_SqlRSSetup.Integration.Tests.ps1, line 41 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
        $script:mockSourceMediaDisplayName = 'Microsoft SQL Server Reporting Services (October 2017)'
        $script:mockSourceMediaUrl = 'https://download.microsoft.com/download/E/6/4/E6477A2A-9B58-40F7-8AD6-62BB8491EA78/SQLServerReportingServices.exe'

We could create and issue to add tests for newer (2019) reporting services. I know there are issues reported in the repos with newer RS when used with the resources, so might not be a quick fix adding an additional exe to download.

Also, maybe we should add separate pipeline jobs to test RS 2017 and newer?

This can be done in another PR. Would you mind creating an issue for it so we can track it?

I agree - I suspect if SSRS is more of a component in it's own right (and not directly tied into SQL versions), it makes more sense to test it in isolation from the rest of the SQL versions going forward so there's not a untidy, staggered coupling of these within the integration tests.

I'll raise an issue for this one shortly.

I was also tempted to add a couple of other issues for SQL2012 and SQL2014 support in these integration tests given these versions could be used/around for a while...

SQL2012 - Lifecycles - 12th July 2022 (end of extended support of latest service pack)

SQL2014 - Lifecycles - 9th July 2024 (end of extended support of latest service pack)

Personally, I'd find SQL2014 tests useful (SQL2012 less so but could be useful to others), and unless there are problematic, functionality changes needing lots of refactoring/fixing, the changes aren't too significant in nature (if similar to adding SQL2019).

Let me know what you think.

Copy link
Contributor Author

@SphenicPaul SphenicPaul 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: 33 of 35 files reviewed, 5 unresolved discussions (waiting on @johlju)


tests/Integration/DSC_SqlRSSetup.Integration.Tests.ps1, line 41 at r2 (raw file):

12th July 2020
Correction: SQL2012 is 12th July 2022 (not 12th July 2020)

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.

:lgtm:

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


tests/Integration/DSC_SqlRSSetup.Integration.Tests.ps1, line 41 at r2 (raw file):

Previously, SphenicPaul (Paul J. Wilcox) wrote…

12th July 2020
Correction: SQL2012 is 12th July 2022 (not 12th July 2020)

Please add issues for SQL2012 and SQL2014 as well, agree they will be useful.

@johlju
Copy link
Member

johlju commented Jan 24, 2021

@SphenicPaul We need to skip the integration tests for SqlRS too for the 2019 job, it fails now since it does not install Reporting Services. Those tests will need to be added to the (separate) jobs for Reporting Services, and could be mentioned in the issue you are about to create.

@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 waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Jan 24, 2021
Copy link
Contributor Author

@SphenicPaul SphenicPaul left a comment

Choose a reason for hiding this comment

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

OK. I've removed the 'Integration_SQL2019' condition so the 'SqlRS', integration tests won't run in SQL2019, integration tests. I've also removed the '150' references to remove all SQL2019 'evidence'.

Build running again now.

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

Copy link
Contributor Author

@SphenicPaul SphenicPaul left a comment

Choose a reason for hiding this comment

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

Looks like the SQL2019 integration tests have now passed ... others still running.

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

@SphenicPaul
Copy link
Contributor Author

OK. I've now added the following issues off the back of this:

...feel free to add to these if you want to add more information/thoughts.

@johlju
Copy link
Member

johlju commented Jan 24, 2021

Thank you for adding the issues! I have nothing to add to them at the moment. 🙂

Waiting for the tests to pass then I merge this.

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.

:lgtm:

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju johlju added ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jan 24, 2021
@johlju johlju merged commit 8a040bf into dsccommunity:main Jan 24, 2021
@johlju johlju removed the ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. label Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SqlServerDsc: Add a new pipeline job to test SQL Server 2019 family
2 participants