search

dsccommunity / xPendingReboot

MIT License
31 stars 10 forks source link

Fix Registry Key Evaluation #15

Closed GavinEke closed 6 years ago

GavinEke commented 6 years ago

Changed all instances of hklm: and hklm:\ with HKLM:\ to be readable and consistent with other modules and documentation while also fixing Issue #8

This change is Reviewable

GavinEke commented 6 years ago

I noticed the failed tests relate to files that I have not changed. Should I be targeting the dev branch or the master branch?

johlju commented 6 years ago

@GavinEke You should target the dev branch. This repo haven't merged a PR for soon two years, so the test framework has evolved a lot, and unfortunately this repo don't have a community maintainer that kept the repo up to date, and @kwirkykat and @mbreakey3 are very busy with their regular job.

Looks like the last merged PR passed the test, but that must be false positive since the test are failing now. https://ci.appveyor.com/project/PowerShell/xpendingreboot/build/0.3.46.0

If you are up to resolve this some of these are easy to resolve.

  1. Remove any tab characters in the README.md. https://ci.appveyor.com/project/PowerShell/xpendingreboot/build/0.3.51.0#L318
  2. The file xPendingReboot.tests.ps1 must have a blank row at the end of the file. https://ci.appveyor.com/project/PowerShell/xpendingreboot/build/0.3.51.0#L325
  3. Pester recently release v4.0 with breaking changes. All Assert-VerifiableMocks need to change to Assert-VerifiableMock (without the 's' at the end). https://ci.appveyor.com/project/PowerShell/xpendingreboot/build/0.3.51.0#L553

These must have always failed (?), but reported as false positive way back in the last PR. I do not know what the problem is with these (haven't looked at the code).

  1. https://ci.appveyor.com/project/PowerShell/xpendingreboot/build/0.3.51.0#L556
  2. https://ci.appveyor.com/project/PowerShell/xpendingreboot/build/0.3.51.0#L561
GavinEke commented 6 years ago

Thanks @johlju I will have a look into resolving all the failed test tomorrow. I think I should be able to resolve most if not all.

GavinEke commented 6 years ago

Fixed tests.

johlju commented 6 years ago

Awesome work on this one! Thanks for fixing most PSSA rules! :smile:

I made a couple of minor review comments.

Reviewed 1 of 2 files at r2, 2 of 2 files at r3. Review status: all files reviewed at latest revision, 3 unresolved discussions.

README.md, line 16 at r3 (raw file):

DscResource.Kit

This must be a very old name. The link works, but this should be 'DscResources' instead, do you mind fixing this as well?

README.md, line 35 at r3 (raw file):

* **ComponentBasedServicing**: (Read-only) One of the locations that are examined by the resource.

Details are returned by Get-DSCConfiguration.

We should indent this row two spaces and the blank rows before and after should be removed. I think it will be rendered correctly then on the description of ComponentBasedServicing parameter.

Now it is rendered wrong I think: https://github.com/GavinEke/xPendingReboot/blob/2ec541e289bdbbde123f6842291815ddbf383400/README.md#xpendingreboot-1

DSCResources/MSFT_xPendingReboot/MSFT_xPendingReboot.psm1, line 78 at r3 (raw file):

    (
        [Parameter(Mandatory=$true)]
        [string]$Name,

Non-blocking, meaning that comment is not part of your change, you may leave as-is.

If we move variable name to the next row (under the type) it will align with the style guideline. Throughout.

Comments from Reviewable

GavinEke commented 6 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

README.md, line 35 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
We should indent this row two spaces and the blank rows before and after should be removed. I think it will be rendered correctly then on the description of `ComponentBasedServicing` parameter. Now it is rendered wrong I think: https://github.com/GavinEke/xPendingReboot/blob/2ec541e289bdbbde123f6842291815ddbf383400/README.md#xpendingreboot-1

I have decided the sentence is redundant and I have removed it.

Comments from Reviewable

GavinEke commented 6 years ago

Thanks for the review Johan. I have made the suggested changes.

Reviewed 1 of 2 files at r2, 2 of 2 files at r4. Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

GavinEke commented 6 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

README.md, line 16 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
> DscResource.Kit This must be a very old name. The link works, but this should be 'DscResources' instead, do you mind fixing this as well?

Done.

README.md, line 35 at r3 (raw file):

Previously, GavinEke (Gavin Eke) wrote…
I have decided the sentence is redundant and I have removed it.

Done.

Comments from Reviewable

johlju commented 6 years ago

Reviewed 2 of 2 files at r4. Review status: all files reviewed at latest revision, 3 unresolved discussions.

README.md, line 16 at r3 (raw file):

Previously, GavinEke (Gavin Eke) wrote…
Done.

LGTM. For some reason I couldn't resolve this one when I acknowledged.

README.md, line 35 at r3 (raw file):

Previously, GavinEke (Gavin Eke) wrote…
Done.

LGTM. For some reason I couldn't resolve this one when I acknowledged.

README.md, line 47 at r4 (raw file):

### Unreleased

* Converted appveyor.yml to install Pester from PSGallery instead of from Chocolatey.

Could you please add an entry to what was chnage in this PR?

Comments from Reviewable

GavinEke commented 6 years ago

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

README.md, line 47 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Could you please add an entry to what was chnage in this PR?

Done.

Comments from Reviewable

johlju commented 6 years ago
:lgtm:

@kwirkykat would you mind merging this change. @GavinEke was kind enough to help us clean up this repo so it passes the tests, so would be great to have this PR merged.

Reviewed 1 of 1 files at r5. Review status: all files reviewed at latest revision, 1 unresolved discussion.

README.md, line 47 at r4 (raw file):

Previously, GavinEke (Gavin Eke) wrote…
Done.

LGTM. This comment was not resolved when I acknowledge for some reason.

Comments from Reviewable

GamerLivingWill commented 6 years ago

Thank you @kwirkykat !