BREAKING CHANGE: PowerPlan: Add IsActive read-only property #193

TraGicCode · 2018-10-03T23:30:40Z

Pull Request (PR) description

Added Read-Only property call IsActive.

This Pull Request (PR) fixes the following issues

Fixes #171

Task list

Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md in the resource folder.
Resource parameter descriptions added/updated in 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 Resource Testing Guidelines.
Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

Closes #171

…ing added.

…is not used as any parameters for the 3 dsc powershell functions (test, get, set).

…ve Read-Only Property.

TraGicCode · 2018-10-03T23:31:15Z

Hey @johlju ,

Are you able to review this. I believe i did everything correct in regards to implementing a read-only property.

Thanks!

PlagueHO

Just some minor tweaks. Thank you for submitting this @TraGicCode !!!

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TraGicCode)

CHANGELOG.md, line 11 at r1 (raw file):

- Added .VSCode settings for applying DSC PSSA rules - fixes [Issue #189](/~https://github.com/PowerShell/ComputerManagementDsc/issues/189).
- PowerPlan:
  - Added IsActive Read-Only Property. Fixes [Issue #171](/~https://github.com/PowerShell/ComputerManagementDsc/issues/171)

Can you change . to - for consistency and in a full stop?

TraGicCode · 2018-10-04T11:56:22Z

Hey @PlagueHO,

I want to remove the verbose messaging in the get target resource that says whether or not the power plan is active or not since it’s a bit redundant since now get target resource returns that information. Do you have any issues with this?

TraGicCode · 2018-10-04T11:58:24Z

I’m not at a computer now but I forgot this might still be helpful if the test and set methods are calling this function and you want to know what’s happening during a dsc run. In the context of the get function it’s redundant.

TraGicCode · 2018-10-04T14:18:21Z

CHANGELOG.md, line 11 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you change . to - for consistency and in a full stop?

I think this is Done. You might need to point to a line as an example if i didn't get this correct.

TraGicCode

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @PlagueHO)

CHANGELOG.md, line 11 at r1 (raw file):

Previously, TraGicCode (Michael Fyffe) wrote…

I think this is Done. You might need to point to a line as an example if i didn't get this correct.

Done.

codecov-io · 2018-10-04T14:21:22Z

Codecov Report

Merging #193 into dev will decrease coverage by <1%.
The diff coverage is 100%.

@@        Coverage Diff         @@
##           dev   #193   +/-   ##
==================================
- Coverage   90%    90%   -1%     
==================================
  Files        7      7           
  Lines      739    735    -4     
==================================
- Hits       671    667    -4     
  Misses      68     68

johlju · 2018-10-07T12:43:08Z

I want to remove the verbose messaging in the get target resource that says whether or not the power plan is active or not since it’s a bit redundant since now get target resource returns that information. Do you have any issues with this?

Two reason to keep it. First, there is a PSSA rule that will fail if there is no Write-Verbose in the Get-function. Also, the verbose message is for debug purpose when running the configuration with verbose messages, to show what the resource are returning, or running the Get-DscConfiguration where only the Get-function is called.
So I suggest we keep the verbose messages.

johlju

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @PlagueHO and @TraGicCode)

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 84 at r2 (raw file):

Name             = $Name

This does no longer return $null if the plan is not active. I think we should mention that change in the CHANGELOG.md? If users have logic is depending on this returning $null, that logic will fail. So would be good to mentioned that?

johlju · 2018-10-07T12:52:03Z

@PlagueHO @TraGicCode as per my review comment, should we consider this as a breaking change? I'm leaning towards not.

TraGicCode · 2018-10-08T05:33:22Z

@PlagueHO @TraGicCode as per my review comment, should we consider this as a breaking change? I'm leaning towards not.

I'm leaning towards it being a breaking change based on the comment you made shown below

This does no longer return $null if the plan is not active. I think we should mention that change in the CHANGELOG.md? If users have logic is depending on this returning $null, that logic will fail. So would be good to mentioned that?

While the contract has not changed the behavior has which can break/cause issues when people upgrade.

…re returned as a name set to null.

TraGicCode

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @johlju and @PlagueHO)

Modules/ComputerManagementDsc/DSCResources/MSFT_PowerPlan/MSFT_PowerPlan.psm1, line 84 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Name             = $Name
This does no longer return $null if the plan is not active. I think we should mention that change in the CHANGELOG.md? If users have logic is depending on this returning $null, that logic will fail. So would be good to mentioned that?

Done.

PlagueHO

@TraGicCode, @johlju - Unfortunately I think this might have because Puppet and Chef users both use the Get-TargetResource function directly and this change does break the contract I believe because of how $Name is now used.

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johlju and @TraGicCode)

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

- PowerPlan:
  - Added IsActive Read-Only Property - Fixes [Issue #171](/~https://github.com/PowerShell/ComputerManagementDsc/issues/171).
  - InActive power plans are no longer returned with their Name set to null.  Now, the name is always returned and the Read-Only property

Can you reduce the length of this line to below the max line length and also remove the double space after the full stop?

TraGicCode

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @PlagueHO and @johlju)

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you reduce the length of this line to below the max line length and also remove the double space after the full stop?

Done.

johlju

Approving my review comments - although @PlagueHO has review comments still not resolved.

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

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

Previously, TraGicCode (Michael Fyffe) wrote…

Done.

The tests still fail on this row. https://ci.appveyor.com/project/PowerShell/computermanagementdsc/builds/19332642?fullLog=true#L3813

TraGicCode · 2018-10-10T03:02:44Z

Thanks @johlju as always!

PlagueHO

Hi @TraGicCode - there are still some test failures in the linelength https://ci.appveyor.com/project/PowerShell/computermanagementdsc/builds/19332642?fullLog=true#L3815

Reviewable status: complete! all files reviewed, all discussions resolved

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

Previously, johlju (Johan Ljunggren) wrote…

The tests still fail on this row. https://ci.appveyor.com/project/PowerShell/computermanagementdsc/builds/19332642?fullLog=true#L3813

This line is still too long I think because the tests are failing.

TraGicCode

Done.

Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @johlju)

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This line is still too long I think because the tests are failing.

Done.

PlagueHO

Once @johlju completes his review I'll merge. Thanks again @TraGicCode

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

johlju

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

PlagueHO · 2018-10-11T06:08:06Z

Doh! With all this talk about BREAKING CHANGE we actually forgot to mark this as BREAKING CHANGE in the changelog. I'll quickly submit a PR.

johlju · 2018-10-11T06:18:44Z

@PlagueHO Yeah, we totally missed that 😃 Approved the other PR, it's ready for merge.

PlagueHO · 2018-10-11T06:28:28Z

Thanks @johlju - Yeah, I've been a bit distracted with work stuff the last couple of weeks so trying to just keep up with as many PR's as possible in what ever spare time I have. But just not getting much done right now 😢 Might have some spare time this weekend though.

TraGicCode · 2018-10-11T12:53:46Z

Thanks @PlagueHO and @johlju ,

As always great job on your modules and keeping things rolling.

johlju · 2018-10-11T14:01:03Z

@PlagueHO I haven’t much time now either. Gonna try to get something done this weekend too.

@TraGicCode thank you! Also thank you for your contributions.

TraGicCode added 4 commits October 3, 2018 22:44

PowerPlan: Adding IsActive Read-Only Property

dfa4758

Closes #171

Update Changelog to contain info about IsActive Read-Only Property be…

e6feb22

…ing added.

Read-Only properties must be in the schema.mof even if the parameter …

90f024b

…is not used as any parameters for the 3 dsc powershell functions (test, get, set).

Updated unit tests to properly handle the changes made for the IsActi…

1f7ca08

…ve Read-Only Property.

PlagueHO added the needs review The pull request needs a code review. label Oct 4, 2018

PlagueHO requested changes Oct 4, 2018

View reviewed changes

Attempting to fix pull-request review recommendation.

a39c1a4

TraGicCode commented Oct 4, 2018

View reviewed changes

johlju requested changes Oct 7, 2018

View reviewed changes

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 Oct 7, 2018

Added note to changelog that powerplans that are inactive no longer a…

16a59ed

…re returned as a name set to null.

TraGicCode commented Oct 8, 2018

View reviewed changes

PlagueHO requested changes Oct 8, 2018

View reviewed changes

Reduce line length based on pr.

0b035ed

TraGicCode commented Oct 8, 2018

View reviewed changes

TraGicCode changed the title ~~PowerPlan: Add IsActive read-only property~~ BREAKING CHANGE: PowerPlan: Add IsActive read-only property Oct 8, 2018

johlju approved these changes Oct 8, 2018

View reviewed changes

PlagueHO requested changes Oct 10, 2018

View reviewed changes

Fix line length markdown violation.

a4d7213

TraGicCode commented Oct 10, 2018

View reviewed changes

PlagueHO approved these changes Oct 10, 2018

View reviewed changes

johlju approved these changes Oct 10, 2018

View reviewed changes

PlagueHO merged commit 99c25ff into dsccommunity:dev Oct 11, 2018

johlju removed the ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. label Oct 11, 2018

PlagueHO mentioned this pull request Oct 11, 2018

Add BREAKING CHANGE to CHANGELOG.MD - Fixes #171 #194

Merged

9 tasks

TraGicCode deleted the GH-171-add-is-active-read-only-property branch October 11, 2018 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING CHANGE: PowerPlan: Add IsActive read-only property #193

BREAKING CHANGE: PowerPlan: Add IsActive read-only property #193

TraGicCode commented Oct 3, 2018 •

edited by joeyaiello

Loading

TraGicCode commented Oct 3, 2018

PlagueHO left a comment

TraGicCode commented Oct 4, 2018

TraGicCode commented Oct 4, 2018 •

edited

Loading

TraGicCode commented Oct 4, 2018

TraGicCode left a comment

codecov-io commented Oct 4, 2018

johlju commented Oct 7, 2018

johlju left a comment

johlju commented Oct 7, 2018

TraGicCode commented Oct 8, 2018 •

edited

Loading

TraGicCode left a comment

PlagueHO left a comment

TraGicCode left a comment

johlju left a comment

TraGicCode commented Oct 10, 2018

PlagueHO left a comment

TraGicCode left a comment

PlagueHO left a comment

johlju left a comment

PlagueHO commented Oct 11, 2018

johlju commented Oct 11, 2018

PlagueHO commented Oct 11, 2018

TraGicCode commented Oct 11, 2018

johlju commented Oct 11, 2018

BREAKING CHANGE: PowerPlan: Add IsActive read-only property #193

BREAKING CHANGE: PowerPlan: Add IsActive read-only property #193

Conversation

TraGicCode commented Oct 3, 2018 • edited by joeyaiello Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

TraGicCode commented Oct 3, 2018

PlagueHO left a comment

Choose a reason for hiding this comment

TraGicCode commented Oct 4, 2018

TraGicCode commented Oct 4, 2018 • edited Loading

TraGicCode commented Oct 4, 2018

TraGicCode left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 4, 2018

Codecov Report

johlju commented Oct 7, 2018

johlju left a comment

Choose a reason for hiding this comment

johlju commented Oct 7, 2018

TraGicCode commented Oct 8, 2018 • edited Loading

TraGicCode left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

TraGicCode left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

TraGicCode commented Oct 10, 2018

PlagueHO left a comment

Choose a reason for hiding this comment

TraGicCode left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

PlagueHO commented Oct 11, 2018

johlju commented Oct 11, 2018

PlagueHO commented Oct 11, 2018

TraGicCode commented Oct 11, 2018

johlju commented Oct 11, 2018

TraGicCode commented Oct 3, 2018 •

edited by joeyaiello

Loading

TraGicCode commented Oct 4, 2018 •

edited

Loading

TraGicCode commented Oct 8, 2018 •

edited

Loading