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

SqlMaxDop Fix for #396 #1644

Merged
merged 3 commits into from
Dec 10, 2020
Merged

SqlMaxDop Fix for #396 #1644

merged 3 commits into from
Dec 10, 2020

Conversation

Fiander
Copy link
Contributor

@Fiander Fiander commented Dec 7, 2020

Pull Request (PR) description

Added three return values for Get-TargetResource and tests

This Pull Request (PR) fixes the following issues

Task list

  • [X ] 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

@codecov
Copy link

codecov bot commented Dec 7, 2020

Codecov Report

Merging #1644 (f46ecf8) into master (f5724c7) will increase coverage by 0%.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1644   +/-   ##
======================================
  Coverage      98%     98%           
======================================
  Files          38      38           
  Lines        6177    6179    +2     
======================================
+ Hits         6085    6087    +2     
  Misses         92      92           
Flag Coverage Δ
unit 98% <100%> (+<1%) ⬆️

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

Impacted Files Coverage Δ
...urce/DSCResources/DSC_SqlMaxDop/DSC_SqlMaxDop.psm1 100% <100%> (ø)

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

This looks good, just a tiny change in the CHANGELOG.md.

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


CHANGELOG.md, line 7 at r2 (raw file):

## [Unreleased]

Before the entry we need to add header ### Fixed or ### Changed depending on what suits best (see https://keepachangelog.com/en/1.0.0/). Also a blank rows around the header. See previous log entries.


source/DSCResources/DSC_SqlMaxDop/DSC_SqlMaxDop.psm1, line 55 at r2 (raw file):

Ensure

Non-blocking, just thoughts. This resource shouldn't have had a Ensure parameter in the first place, since it does not create an object. Realized now that it have an Ensure property. Technical debt... . 🙂

@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 Dec 9, 2020
@Fiander
Copy link
Contributor Author

Fiander commented Dec 9, 2020

Personally i like this ensure. It sets the maxdop to the server default when set to Absent.
No need to figure out what the setting should be when you dont want to set maxdop anymore.

removing it would als make it a breaking change

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

@johlju johlju merged commit fdc7608 into dsccommunity:master Dec 10, 2020
@johlju johlju changed the title SqlServerMaxDop Fix for #396 SqlServerMaxDop: Fix for #396 Dec 10, 2020
@johlju johlju changed the title SqlServerMaxDop: Fix for #396 SqlMaxDop Fix for #396 Dec 10, 2020
@johlju johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Dec 10, 2020
Fiander added a commit to Fiander/SqlServerDsc that referenced this pull request Dec 11, 2020
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.

SqlServerMaxDop: Should return values for Ensure and DynamicAlloc when Get-DscConfiguration is called
2 participants