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.

Sign up for GitHub

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

Jump to bottom

SqlServerLogin: Set default database #1485

Merged
merged 10 commits into from
Apr 11, 2020
Merged

SqlServerLogin: Set default database #1485

merged 10 commits into from
Apr 11, 2020

Conversation

bozho
Copy link
Contributor

@bozho bozho commented Mar 27, 2020 •
edited by johlju
Loading

Pull Request (PR) description

Adds DefaultDatabase parameter.

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 added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

@bozho
Copy link
Contributor Author

bozho commented Mar 27, 2020

@johlju I could use some help with unit testing the change.

@johlju johlju added the needs review The pull request needs a code review. label Mar 28, 2020
@johlju
Copy link
Member

johlju commented Mar 28, 2020

I won't have the bandwidth for a while, maybe you can look at the unit test and se how the property PasswordExpirationEnabled is tested and do something similar? 🙂

@johlju johlju changed the title [#1474] SqlServerLogin: Set default database SqlServerLogin: Set default database Apr 2, 2020
johlju
johlju requested changes Apr 2, 2020
View reviewed changes
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.

@bozho Awesome work with the unit test, and code looks great! 😃 Just minor review comments. 🙂

Reviewed 7 of 8 files at r1, 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @bozho)

a discussion (no related file):
Can you add an example for setting default database? Add link to the example in the resource documentation in the README.md too.

CHANGELOG.md, line 13 at r3 (raw file):

.)

Move the full stop . after the parentheses.

tests/Integration/MSFT_SqlServerLogin.config.ps1, line 106 at r3 (raw file):

SqlDatabase 'DefaultDb_Test'

Either add a configuration that removes this database again at the end, or update the integration documentation saying that the integration tests leaves the database, see here https://github.com/dsccommunity/SqlServerDsc/blob/master/tests/Integration/README.md#sqlserverlogin.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 194 at r3 (raw file):

"master"

We should use single-quote '.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 198 at r3 (raw file):

"master"

We should use single-quote '.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 203 at r3 (raw file):

"master"

We should use single-quote '.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 209 at r3 (raw file):

"master"

We should use single-quote '.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 294 at r3 (raw file):

Should -Not -BeNullOrEmpty

This should test so we get back the correct value Should -Be 'master' (or what the correct database is in this test)

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 307 at r3 (raw file):

$result.DefaultDatabase | Should -Not -BeNullOrEmpty

This should test so we get back the correct value Should -Be 'master' (or what the correct database is in this test)

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 320 at r3 (raw file):

$result.DefaultDatabase | Should -Not -BeNullOrEmpty

This should test so we get back the correct value Should -Be 'master' (or what the correct database is in this test)

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 335 at r3 (raw file):

$result.DefaultDatabase | Should -Not -BeNullOrEmpty

This should test so we get back the correct value Should -Be 'master' (or what the correct database is in this test)

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 622 at r3 (raw file):

$testTargetResource_SqlLoginPresentWithDefaultDatabaseMaster_EnsurePresent.Add( 'Ensure', 'Present' )

Non-blocking. This and those similar rows could be written like this which might be easier to read. But good as-is too.

$testTargetResource_SqlLoginPresentWithDefaultDatabaseMaster_EnsurePresent[ 'Ensure'] = 'Present'

@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 Apr 2, 2020
bozho
bozho commented Apr 8, 2020
View reviewed changes
Copy link
Contributor Author

@bozho bozho 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: 7 of 10 files reviewed, 12 unresolved discussions (waiting on @johlju)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Can you add an example for setting default database? Add link to the example in the resource documentation in the README.md too.

Done.

CHANGELOG.md, line 13 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
.)

Move the full stop . after the parentheses.

Done.

tests/Integration/MSFT_SqlServerLogin.config.ps1, line 106 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
SqlDatabase 'DefaultDb_Test'

Either add a configuration that removes this database again at the end, or update the integration documentation saying that the integration tests leaves the database, see here https://github.com/dsccommunity/SqlServerDsc/blob/master/tests/Integration/README.md#sqlserverlogin.

How would I clean up at the end? Just add another Context block which runs a "cleanup" configuration?

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 194 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
"master"

We should use single-quote '.

Done.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 198 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
"master"

We should use single-quote '.

Done.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 203 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
"master"

We should use single-quote '.

Done.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 209 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
"master"

We should use single-quote '.

Done.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 294 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Should -Not -BeNullOrEmpty

This should test so we get back the correct value Should -Be 'master' (or what the correct database is in this test)

Done.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 307 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$result.DefaultDatabase | Should -Not -BeNullOrEmpty

This should test so we get back the correct value Should -Be 'master' (or what the correct database is in this test)

Done.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 320 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$result.DefaultDatabase | Should -Not -BeNullOrEmpty

This should test so we get back the correct value Should -Be 'master' (or what the correct database is in this test)

Done.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 335 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$result.DefaultDatabase | Should -Not -BeNullOrEmpty

This should test so we get back the correct value Should -Be 'master' (or what the correct database is in this test)

Done.

tests/Unit/MSFT_SqlServerLogin.Tests.ps1, line 622 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$testTargetResource_SqlLoginPresentWithDefaultDatabaseMaster_EnsurePresent.Add( 'Ensure', 'Present' )

Non-blocking. This and those similar rows could be written like this which might be easier to read. But good as-is too.

$testTargetResource_SqlLoginPresentWithDefaultDatabaseMaster_EnsurePresent[ 'Ensure'] = 'Present'

Done.

@johlju johlju added needs review The pull request needs a code review. 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 Apr 10, 2020
johlju
johlju requested changes Apr 10, 2020
View reviewed changes
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.

Reviewed 3 of 3 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @bozho)

tests/Integration/MSFT_SqlServerLogin.config.ps1, line 106 at r3 (raw file):

Previously, bozho (Marko Bozikovic) wrote…

How would I clean up at the end? Just add another Context block which runs a "cleanup" configuration?

Yes, exactly, but you only need the It-block that Sets. I usually skip running the tests for Get and Test for those cleanup configurations.

@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 Apr 10, 2020
@johlju
Copy link
Member

johlju commented Apr 10, 2020

@bozho great work on this! Just a fix for the integration tests and this can be merged 😃

bozho
bozho commented Apr 10, 2020
View reviewed changes
Copy link
Contributor Author

@bozho bozho 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: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @johlju)

tests/Integration/MSFT_SqlServerLogin.config.ps1, line 106 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Yes, exactly, but you only need the It-block that Sets. I usually skip running the tests for Get and Test for those cleanup configurations.

Done.

@johlju johlju added needs review The pull request needs a code review. 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 Apr 10, 2020
johlju
johlju approved these changes Apr 11, 2020
View reviewed changes
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 r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju johlju merged commit 65d9a13 into dsccommunity:master Apr 11, 2020
@bozho bozho deleted the issue-1474-sqllogin-default-db branch April 14, 2020 06:36
@johlju johlju removed the needs review The pull request needs a code review. label Apr 25, 2020
@bozho bozho restored the issue-1474-sqllogin-default-db branch March 10, 2021 11:40
@bozho bozho deleted the issue-1474-sqllogin-default-db branch March 10, 2021 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johlju johlju johlju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SqlServerLogin: Set default database
2 participants
@bozho @johlju