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

Test-SqlDscIsRole: New command #2060

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

Test-SqlDscIsRole: New command #2060

wants to merge 3 commits into from
+151 −3

Conversation

IAMJDA
Copy link

@IAMJDA IAMJDA commented Feb 23, 2025 •
edited by johlju
Loading

Pull Request (PR) description

Added a public command that returns whether the server role exists.
Implemented new Test-SqlDscIsRole for testing whether a server principal is a Role. Fixed documentation in Test-SqlDscIsLogin

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

@IAMJDA IAMJDA requested review from johlju and a team as code owners February 23, 2025 16:13
Jens Damm added 2 commits February 23, 2025 17:06
@codecov Codecov
Copy link

codecov bot commented Feb 23, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94%. Comparing base (85ea1ed) to head (242e99b).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #2060   +/-   ##
====================================
  Coverage    94%     94%           
====================================
  Files        94      95    +1     
  Lines      7930    7930           
====================================
+ Hits       7500    7504    +4     
+ Misses      430     426    -4
Flag Coverage Δ
unit 94% <100%> (+<1%) ⬆️
Files with missing lines Coverage Δ
source/Public/Test-SqlDscIsLogin.ps1 100% <ø> (ø)
source/Public/Test-SqlDscIsRole.ps1 100% <100%> (ø)

... and 4 files with indirect coverage changes

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

johlju commented Feb 28, 2025 •
edited
Loading

Due to some external changes (improvements) to HQRM tests there seems to be some unrelated tests that are failing. I will look into it as soon as I can.

johlju
johlju requested changes Mar 1, 2025
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 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @IAMJDA)

source/Public/Test-SqlDscIsRole.ps1 line 19 at r2 (raw file):

    .EXAMPLE
        $serverInstance = Connect-SqlDscDatabaseEngine
        Test-SqlDscIsDatabaseRole -ServerObject $serverInstance -Name 'MyPrincipal'

Should be Test-SqlDscIsRole.

Suggestion:

Test-SqlDscIsRole

CHANGELOG.md line 8 at r2 (raw file):

## [Unreleased]
- SqlServerDsc
  - Added Test-SqlDscIsRole to be used like Test-SqlDscIsLogin but tests for a server role as principal

The entry should be under the section ### Added (see Keep a changelog).

Code quote:

- SqlServerDsc
  - Added Test-SqlDscIsRole to be used like Test-SqlDscIsLogin but tests for a server role as principal

@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 Mar 1, 2025
@johlju johlju changed the title Implemented new Test-SqlDscIsRole for testing whether a server principal is a Role. Fixed documentation in Test-SqlDscIsLogin Test-SqlDscIsRole: New command Mar 1, 2025
@johlju johlju mentioned this pull request Mar 1, 2025
Open
@johlju
Copy link
Member

johlju commented Mar 8, 2025

@IAMJDA are still on this?

@IAMJDA
Copy link
Author

IAMJDA commented Mar 8, 2025

@IAMJDA are still on this?

Yes, just need to find some free time - properly this month :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johlju johlju johlju requested changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test-SqlDscIsRole: New command proposal
2 participants
@IAMJDA @johlju