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

SqlAudit: New resources #1499

Merged
merged 71 commits into from
Aug 13, 2022
Merged

Conversation

johlju
Copy link
Member

@johlju johlju commented Apr 16, 2020

Pull Request (PR) description

  • SqlServerDsc
    • Add new resource SqlServerAudit.
    • The following classes were added to the module:
      • SqlResourceBase - class that can be inherited by class-based resource and
        provides default DSC properties and method for get a [Server]-object.
    • The following public functions were added to the module (see comment-based
      help for more information):
      • Invoke-SqlDscQuery
      • Get-SqlDscAudit
      • New-SqlDscAudit
      • Set-SqlDscAudit
      • Remove-SqlDscAudit
      • Enable-SqlDscAudit
      • Disable-SqlDscAudit
  • Get-DscProperty
    • Added parameter ExcludeName to exclude property names from being returned.
  • SqlPermission
    • Fix comment-based help.
  • ConvertTo-Reason
    • Fix to handle $null values on Windows PowerShell.
    • If the property name contain the word 'Path' the value will be parsed to
      replace backslash or slashes at the end of the string, e.g. '/mypath/'
      will become '/mypath'.
  • ResourceBase
    • Now handles Ensure correctly from derived GetCurrentState(). But
      requires that the GetCurrentState() only return key property if object
      is present, and does not return key property if object is absent.
      Optionally the resource's derived GetCurrentState() can handle Ensure
      itself.

This PR continues the work that was made in the closed PR #1433. Big thanks to @Fiander for your initial work on this! 🙂

This Pull Request (PR) fixes the following issues

Fixes #1132

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

@johlju johlju added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Apr 16, 2020
@Fiander
Copy link
Contributor

Fiander commented Apr 16, 2020

great :-)
It was not abanded, but you got the most resent working version.
Im struggling very much with the unit tests, I can only conclude that for those i'm not experienced yet. so it is for everybodys best intrest I you can help

so... thank you very much.

@johlju
Copy link
Member Author

johlju commented Apr 18, 2020

@Fiander I have been there too, not knowing how to write tests, but it is not that difficult once you get the hang of it and you start to see the value with them. Now I write unit tests for everything in PowerShell.

@stale
Copy link

stale bot commented May 2, 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 May 2, 2020
@quillypowers
Copy link

I'd definitely like to use these methods if this gains more traction.

@johlju
Copy link
Member Author

johlju commented Jul 31, 2020

I thought I would had time to write the tests for these by now. Still on the todo but far down. If someone can write unit and integration tests for these resources we can merge them

@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 Jan 24, 2021
@github-actions
Copy link

github-actions bot commented Feb 8, 2021

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 Feb 8, 2021
@github-actions github-actions bot removed the abandoned The pull request has been abandoned. label Jul 28, 2022
@johlju johlju force-pushed the f/add-resource-SqlServerAudit branch from 2a7d3f9 to cca8cf9 Compare August 3, 2022 15:04
@johlju
Copy link
Member Author

johlju commented Aug 3, 2022

After spending a long time converting this project to Pester 5 I'm finally able to continue with this.
We should rename these two resource to SqlAudit and SqlAuditSpecification to follow the current naming schema, then I will push unit tests.

@johlju johlju changed the title SqlServerAudit, SqlServerAuditSpecification: New resource SqlAudit, SqlAuditSpecification: New resources Aug 3, 2022
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #1499 (2707167) into main (1fe1dce) will increase coverage by 1%.
The diff coverage is 99%.

Impacted file tree graph

@@          Coverage Diff           @@
##           main   #1499     +/-   ##
======================================
+ Coverage    86%     88%     +1%     
======================================
  Files        36      69     +33     
  Lines      6185    7244   +1059     
======================================
+ Hits       5349    6403   +1054     
- Misses      836     841      +5     
Flag Coverage Δ
unit 88% <99%> (+1%) ⬆️
Impacted Files Coverage Δ
source/Classes/020.SqlDatabasePermission.ps1 99% <ø> (ø)
source/Classes/020.SqlPermission.ps1 99% <ø> (ø)
source/Classes/011.SqlResourceBase.ps1 90% <90%> (ø)
source/Public/Get-SqlDscAudit.ps1 91% <91%> (ø)
source/Classes/010.ResourceBase.ps1 100% <100%> (ø)
source/Classes/020.SqlAudit.ps1 100% <100%> (ø)
source/Private/ConvertTo-Reason.ps1 100% <100%> (ø)
source/Private/Get-DscProperty.ps1 100% <100%> (ø)
source/Public/Disable-SqlDscAudit.ps1 100% <100%> (ø)
source/Public/Enable-SqlDscAudit.ps1 100% <100%> (ø)
... and 28 more

@johlju johlju changed the title SqlAudit, SqlAuditSpecification: New resources SqlAudit: New resources Aug 9, 2022
@johlju johlju marked this pull request as ready for review August 10, 2022 07:32
@johlju johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Aug 10, 2022
@johlju
Copy link
Member Author

johlju commented Aug 10, 2022

This PR is now ready for review if someone like to dig into it. SqlAudit was refactored entirely.

I also moved out the SqlServerAuditSpecification to a separate PR. I looked at underlaying SMO classes and it looks like it should be possible to pass one or more [AuditSpecificationDetail]. So I think it also need to be refactored the same way as SqlAudit was, but have a complex type that holds the information for one or more [AuditSpecificationDetail] . Anyone knows more about this then please let me know in PR #1779.

For SqlAudit I would like to add a a couple of evaluations of the passed parameters before Set-SqlDscAudit, and also handle removal an re-creating of the audit if it should be switched between File and Log or vice-verse. I might just create issues for this to be handled in other PR's.

Copy link
Member Author

@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 1 of 25 files at r1, 47 of 56 files at r2, 11 of 11 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion


source/en-US/SqlAudit.strings.psd1 line 17 at r3 (raw file):

    EvaluateServerAudit = Evaluate the current audit '{0}' on the instance '{1}'. (SA0004)
    MaximumFileSizeValueInvalid = The maximum file size must be set to a value of 0 or a value between 2 and 2147483647. (SA0004)
    QueueDelayValueInvalid = The queue delay must be set to a value of 0 or a value between 1000 and 2147483647.

We should have identifiers on the localized strings.

Copy link
Member Author

@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: 58 of 59 files reviewed, all discussions resolved (waiting on @johlju)


source/en-US/SqlAudit.strings.psd1 line 17 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should have identifiers on the localized strings.

Done

Copy link
Member Author

@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, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @johlju)

@johlju johlju merged commit d741244 into dsccommunity:main Aug 13, 2022
@johlju johlju deleted the f/add-resource-SqlServerAudit branch August 13, 2022 06:24
@johlju johlju removed the needs review The pull request needs a code review. label Aug 13, 2022
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.

New resource proposal: Server and database audit
3 participants