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

Added cmdlet Compare-ResourcePropertyState #48

Merged

Conversation

johlju
Copy link
Member

@johlju johlju commented Jul 12, 2020

Pull Request (PR) description

  • Added cmdlet Compare-ResourcePropertyState and also introduces a new
    design pattern how to evaluate properties in both Test and Set - fixes
    issue #47.

This Pull Request (PR) fixes the following issues

- Fixes #47

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).
  • Documentation added/updated in README.md.
  • Comment-based help added/updated for all new/changed functions.
  • 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 requested a review from PlagueHO July 12, 2020 16:50
@johlju johlju added the needs review The pull request needs a code review. label Jul 12, 2020
Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @johlju)


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

## Added

- Added cmdlet `Compare-ResourcePropertyState` and also introduces a new

and -> that


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

- Added cmdlet `Compare-ResourcePropertyState` and also introduces a new
  design pattern how to evaluate properties in both _Test_ and _Set_ - fixes

how to -> to


README.md, line 153 at r1 (raw file):

a hashtable with the metadata from the comparison.

This introduces a new design pattern how to evaluate current and desired

Maybe: "This introduces a new design pattern that is used to evaluate current and desired state in a DSC resource."


README.md, line 155 at r1 (raw file):

This introduces a new design pattern how to evaluate current and desired
state in a DSC resource. This cmdlet is meant to be used in a DSC resource
from both the _Test_ and _Set_. The evaluation is made in _Set_ to make sure

from both Test and Set. (no need for 'the").


README.md, line 156 at r1 (raw file):

state in a DSC resource. This cmdlet is meant to be used in a DSC resource
from both the _Test_ and _Set_. The evaluation is made in _Set_ to make sure
to only change the properties that was not in desired state. Properties that

was not in desired state -> are not in the desired state


README.md, line 157 at r1 (raw file):

from both the _Test_ and _Set_. The evaluation is made in _Set_ to make sure
to only change the properties that was not in desired state. Properties that
was in desired state should not be changed again. This design pattern also

was in desired state -> are in the desired state


README.md, line 159 at r1 (raw file):

was in desired state should not be changed again. This design pattern also
handles when the cmdlet `Invoke-DscResource` is called with the method
`Set` which with this design pattern will evaluate the properties correctly.

Maybe add a comma after Set.


README.md, line 198 at r1 (raw file):

This examples call Compare-ResourcePropertyState with the current state

examples call -> example calls


README.md, line 220 at r1 (raw file):

This examples call Compare-ResourcePropertyState with the current state

examples call -> example calls


README.md, line 239 at r1 (raw file):

This examples call Compare-ResourcePropertyState with the current state

examples call -> example calls


README.md, line 259 at r1 (raw file):

This examples call Compare-ResourcePropertyState with the current state

examples call -> example calls


source/Private/Test-DscPropertyState.ps1, line 75 at r1 (raw file):

            foreach ($keyProperty in $Values.KeyProperties)
            {
                $currentCimInstance = $Values.CurrentValue |

Is this code right? Seems like we'd only end up with the last KeyProperty in $currentCimInstance - in which case we don't need the loop - just select the last $Values.KeyProperties.


source/Public/Compare-ResourcePropertyState.ps1, line 63 at r1 (raw file):

        [Parameter()]
        [System.String[]]

Should we add [ValidateNotNullOrEmpty()] because an empty array in here would result in undefined behavior?

@johlju johlju requested a review from PlagueHO July 17, 2020 14:32
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: 2 of 7 files reviewed, 13 unresolved discussions (waiting on @PlagueHO)


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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

and -> that

Done.


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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

how to -> to

Done.


README.md, line 153 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Maybe: "This introduces a new design pattern that is used to evaluate current and desired state in a DSC resource."

Done.


README.md, line 155 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

from both Test and Set. (no need for 'the").

Done.


README.md, line 156 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

was not in desired state -> are not in the desired state

Done.


README.md, line 157 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

was in desired state -> are in the desired state

Done.


README.md, line 159 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Maybe add a comma after Set.

Done.


README.md, line 198 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

examples call -> example calls

Done.


README.md, line 220 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

examples call -> example calls

Done.


README.md, line 239 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

examples call -> example calls

Done.


README.md, line 259 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

examples call -> example calls

Done.


source/Private/Test-DscPropertyState.ps1, line 75 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is this code right? Seems like we'd only end up with the last KeyProperty in $currentCimInstance - in which case we don't need the loop - just select the last $Values.KeyProperties.

Done. Great catch! I missed adding tests for two key properties. Corrected.


source/Public/Compare-ResourcePropertyState.ps1, line 63 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should we add [ValidateNotNullOrEmpty()] because an empty array in here would result in undefined behavior?

Done.

Copy link
Member

@PlagueHO PlagueHO 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 5 of 5 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@PlagueHO PlagueHO merged commit 7ed9c78 into dsccommunity:master Jul 18, 2020
@johlju johlju deleted the f/add-Compare-ResourcePropertyState branch July 18, 2020 08:44
@johlju johlju removed the needs review The pull request needs a code review. label Jul 22, 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.

2 participants