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

SqlDatabase: Support Changing Snapshot Isolation #1622

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

Conversation

awickham10
Copy link
Contributor

@awickham10 awickham10 commented Oct 3, 2020

Pull Request (PR) description

Add the ability to change snapshot isolation on a database.

This Pull Request (PR) fixes the following issues

Closes #845

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

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

Awesome work on this one! Thank you! Found som logic changes that need to be implemented.

Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @awickham10)


source/DSCResources/DSC_SqlDatabase/DSC_SqlDatabase.psm1, line 77 at r2 (raw file):

$null

I think we need to set this to the value $false instead. It would be more natural for it to return either $false or $true, and the comparison in Test-function would be more intuitive (it would otherwise compare against $null). 🤔


source/DSCResources/DSC_SqlDatabase/DSC_SqlDatabase.psm1, line 323 at r2 (raw file):

                try
                {
                    $sqlDatabaseObjectToCreate = New-Object -TypeName 'Microsoft.SqlServer.Management.Smo.Database' -ArgumentList $sqlServerObject, $Name

It must also set isolation level when the database is created. Probably why the integration test is failing.


tests/Integration/DSC_SqlDatabase.Integration.Tests.ps1, line 290 at r2 (raw file):

$configurationName = "$($script:dscResourceName)_AddDatabase6_Config"

Can we also add a test that turns off or on isolation level on an existing database, to test that part of the code?

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: all files reviewed, 4 unresolved discussions (waiting on @awickham10)


tests/Integration/DSC_SqlDatabase.Integration.Tests.ps1, line 386 at r2 (raw file):

Should -BeNullOrEmpty

if the change in the Get-TargetResource is done (see other comment) then this should be changed to Should -BeFalse.

@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. invalid Used for PR's to tell hacktoberfest that the PR should not count and removed needs review The pull request needs a code review. labels Oct 5, 2020
@stale
Copy link

stale bot commented Oct 21, 2020

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.

@stale stale bot added the abandoned The pull request has been abandoned. label Oct 21, 2020
@johlju johlju removed the invalid Used for PR's to tell hacktoberfest that the PR should not count label Dec 12, 2020
@stale stale bot removed the abandoned The pull request has been abandoned. label Dec 12, 2020
@stale
Copy link

stale bot commented Dec 26, 2020

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.

@stale stale bot added the abandoned The pull request has been abandoned. label Dec 26, 2020
@johlju johlju changed the base branch from master to main January 8, 2021 15:46
@github-actions github-actions bot removed the abandoned The pull request has been abandoned. label Jul 28, 2022
@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 Aug 12, 2022
@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
Development

Successfully merging this pull request may close these issues.

SqlDatabase: Enable snapshot isolation
2 participants