xRegistry: Fixed issue unable to create registry value with data zero. (Fixes #276) #281

Amedama96 · 2016-11-27T17:11:17Z

xRegistry
Modified the logic to delete the first '0x' of the string.

This change is

msftclas · 2016-11-27T17:11:20Z

Hi @Amedama96, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

Amedama96 · 2016-11-29T14:14:24Z

@kwirkykat
I have already signed CLA 2 days ago.
Should I need to do it once again ?

kwirkykat · 2016-11-29T18:51:03Z

@Amedama96 CLA bot is having issues. I'm going to close the PR and then re-open it to try to get it to cooperate.

msftclas · 2016-11-29T18:51:29Z

Hi @Amedama96, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

TravisEz13 · 2016-11-29T22:23:06Z

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

DSCResources/MSFT_xRegistryResource/MSFT_xRegistryResource.psm1, line 1134 at r1 (raw file):

            {
                $retVal = $null
                if ($Data[0].StartsWith('0x'))

If we repeat this code 3 times, the code should be a function, with tests.

Comments from Reviewable

lfshr · 2016-11-30T13:58:01Z

Comments withdrawn.

Amedama96 · 2016-11-30T14:51:36Z

Hi @LiamFisher92
I appreciate your comment !
and sorry, I think that has no problem for the logic of parsing string to bytes array.

This is the code for parsing. (prefix '0x' has removed already)

for ($i = 0 ; $i -lt ($val.Length-1) ; $i = $i+2)
{
    $byteArray += [System.Byte]::Parse($val.Substring($i, 2), 'HexNumber')
}

[System.Byte]::Parse('00', 'HexNumber') give you byte (0).
[System.Byte]::Parse('0F', 'HexNumber') give you byte (15).
[System.Byte]::Parse('10', 'HexNumber') give you byte (16).

These results are correct, aren't you?
Please tell me if I am misunderstanding.

lfshr · 2016-11-30T15:23:54Z

I beg your pardon. Int32.TryParse with [System.Globalization.NumberStyles]::HexNumber handles this.
Clearly need another coffee.

Amedama96 · 2016-11-30T17:13:29Z

@TravisEz13
Thanks review.
Where and How should I write the test code?
It seems that there is no place to write test code of helper function.
You say I should write unit tests in the "MSFT_xRegistryResource.Tests.ps1" something like below?

It 'Should create a new binary registry key value with data 0' {
    $valueName = 'TestValue'
    $valueData = '0x00'
    $valueType = 'Binary'
...

Amedama96 · 2016-11-30T17:46:32Z

Sadly, I found two additional related issues.
I will try to fix these issues next weekend. please wait a few days.

Issue A

If you specify the ValueData with prefix '0x', Test-TargetResource always return $false. even if the reg value is desired.

xRegistry RegBinary_PrefixIssue
{
    Ensure = "Present"
    Key = "HKEY_LOCAL_MACHINE\SYSTEM\Tests"
    ValueName = "TestValue"
    ValueData = "0x0F"    # No problem happend if you set ValueData="0F", 
    ValueType = "Binary"
}

Issue B

When the current reg binary value is '0000'.
you specify the ValueData = '00' (not '0x00'), Test passes unexpectedly.

* xPackage fixes and updates * Replaces Out-Null with $null assignments * Resolves README.md merge conflict

Add a function 'Remove-PrefixString' and replace redundant codes.

Fix When the ValueData starts with '0x', Test-TargetResource always returned $false

When the current reg binary value is '0000' and ValueData is '00', Test-TargetResource returned wrong comparison result

Amedama96 · 2016-12-03T08:05:47Z

I have resolved @TravisEz13 review and fixed additional issues.

Did My PR mixed with @iainbrighton 's another commit ?
I'm so sorry. I don't know how to fix it. 😓

TravisEz13 · 2016-12-06T19:37:18Z

Tests/Unit/MSFT_xPackageResource.Tests.ps1

@@ -433,7 +435,7 @@ try
                    $msiUrl = "$baseUrl" + "package.msi"
                    New-MockFileServer -FilePath $script:msiLocation -Https

-                    # Test pipe connection as testing server readiness
+                    # Test pipe connection as testing server reasdiness


Please fix misspelling

TravisEz13 · 2016-12-06T19:43:53Z

DSCResources/MSFT_xPackageResource/MSFT_xPackageResource.psm1

@@ -1279,11 +1286,12 @@ function Set-TargetResource
                $startInfo.Arguments += ' /log "{0}"' -f $LogPath
            }

-            $startInfo.Arguments += " /quiet"
+            $startInfo.Arguments += " /quiet /norestart"


Nice catch.

TravisEz13 · 2016-12-06T19:46:27Z

DSCResources/MSFT_xPackageResource/MSFT_xPackageResource.psm1

                {
-                    $process.BeginOutputReadLine()


functional regression of the logstream functionality (but looking at the code I'm not 100% sure it worked)

TravisEz13 · 2016-12-06T19:50:07Z

DSCResources/MSFT_xPackageResource/MSFT_xPackageResource.psm1

+function Register-PInvoke
+{
+    $programSource = @'
+        using System;


Did you get this code from another resource? If so, please add a comment about where you got it from, please?

iainbrighton · 2016-12-06T21:58:40Z

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

DSCResources/MSFT_xPackageResource/MSFT_xPackageResource.psm1, line 1289 at r4 (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

Nice catch.

Thanks! Not sure how the xPackage changes ended up in this PR as they were all merged in #278 ?

DSCResources/MSFT_xPackageResource/MSFT_xPackageResource.psm1, line 1727 at r4 (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

Did you get this code from another resource? If so, please add a comment about where you got it from, please?

This comes from older versions of the xPackage resource!

Comments from Reviewable

TravisEz13 · 2016-12-06T23:32:53Z

Reviewed 2 of 2 files at r1.
Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

DSCResources/MSFT_xPackageResource/MSFT_xPackageResource.psm1, line 1727 at r4 (raw file):

Previously, iainbrighton (Iain Brighton) wrote…

This comes from older versions of the xPackage resource!

Please add a comment to the code of where the code from. And probably file an issue that we should have a common module to do this.

Comments from Reviewable

Amedama96 · 2016-12-07T15:10:27Z

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

DSCResources/MSFT_xRegistryResource/MSFT_xRegistryResource.psm1, line 1134 at r1 (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

If we repeat this code 3 times, the code should be a function, with tests.

Added helper function 'Remove-PrefixString'

Comments from Reviewable

TravisEz13 · 2016-12-07T19:29:27Z

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

Comments from Reviewable

Amedama96 · 2016-12-18T04:57:32Z

Those xRegistry resource issues fixed and merged #299 by @kwirkykat
Thanks all members and
I apologize for any inconvenience caused by lack of knowledge about github.

Amedama96 added 2 commits November 28, 2016 01:39

fix issue dsccommunity#276

f8607a2

Update the Release Notes

c048715

msftclas added the cla-required label Nov 27, 2016

kwirkykat added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Nov 29, 2016

kwirkykat closed this Nov 29, 2016

kwirkykat removed the cla-required label Nov 29, 2016

kwirkykat reopened this Nov 29, 2016

kwirkykat removed the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Nov 29, 2016

msftclas added the cla-already-signed label Nov 29, 2016

kwirkykat added the needs review The pull request needs a code review. label Nov 29, 2016

TravisEz13 added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Nov 29, 2016

iainbrighton and others added 5 commits December 3, 2016 16:48

xPackage fixes and updates (dsccommunity#278)

8f4424a

* xPackage fixes and updates * Replaces Out-Null with $null assignments * Resolves README.md merge conflict

Add helper function for refactoring

dd96c34

Add a function 'Remove-PrefixString' and replace redundant codes.

fix ValueData comapre issue

6deaa8e

Fix When the ValueData starts with '0x', Test-TargetResource always returned $false

Fix hex-string format issue

b85c7dd

When the current reg binary value is '0000' and ValueData is '00', Test-TargetResource returned wrong comparison result

Update the Release Notes

db01c65

TravisEz13 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 Dec 6, 2016

TravisEz13 suggested changes Dec 6, 2016

View reviewed changes

Amedama96 closed this Dec 18, 2016

vors removed the needs review The pull request needs a code review. label Dec 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xRegistry: Fixed issue unable to create registry value with data zero. (Fixes #276) #281

xRegistry: Fixed issue unable to create registry value with data zero. (Fixes #276) #281

Amedama96 commented Nov 27, 2016 •

edited by nanalakshmanan

Loading

msftclas commented Nov 27, 2016

Amedama96 commented Nov 29, 2016

kwirkykat commented Nov 29, 2016

msftclas commented Nov 29, 2016

TravisEz13 commented Nov 29, 2016

lfshr commented Nov 30, 2016 •

edited

Loading

Amedama96 commented Nov 30, 2016 •

edited

Loading

lfshr commented Nov 30, 2016

Amedama96 commented Nov 30, 2016

Amedama96 commented Nov 30, 2016 •

edited

Loading

Amedama96 commented Dec 3, 2016

TravisEz13 Dec 6, 2016

TravisEz13 Dec 6, 2016

TravisEz13 Dec 6, 2016

TravisEz13 Dec 6, 2016

iainbrighton commented Dec 6, 2016

TravisEz13 commented Dec 6, 2016

Amedama96 commented Dec 7, 2016

TravisEz13 commented Dec 7, 2016

Amedama96 commented Dec 18, 2016

xRegistry: Fixed issue unable to create registry value with data zero. (Fixes #276) #281

xRegistry: Fixed issue unable to create registry value with data zero. (Fixes #276) #281

Conversation

Amedama96 commented Nov 27, 2016 • edited by nanalakshmanan Loading

msftclas commented Nov 27, 2016

Amedama96 commented Nov 29, 2016

kwirkykat commented Nov 29, 2016

msftclas commented Nov 29, 2016

TravisEz13 commented Nov 29, 2016

lfshr commented Nov 30, 2016 • edited Loading

Amedama96 commented Nov 30, 2016 • edited Loading

lfshr commented Nov 30, 2016

Amedama96 commented Nov 30, 2016

Amedama96 commented Nov 30, 2016 • edited Loading

Issue A

Issue B

Amedama96 commented Dec 3, 2016

TravisEz13 Dec 6, 2016

Choose a reason for hiding this comment

TravisEz13 Dec 6, 2016

Choose a reason for hiding this comment

TravisEz13 Dec 6, 2016

Choose a reason for hiding this comment

TravisEz13 Dec 6, 2016

Choose a reason for hiding this comment

iainbrighton commented Dec 6, 2016

TravisEz13 commented Dec 6, 2016

Amedama96 commented Dec 7, 2016

TravisEz13 commented Dec 7, 2016

Amedama96 commented Dec 18, 2016

Amedama96 commented Nov 27, 2016 •

edited by nanalakshmanan

Loading

lfshr commented Nov 30, 2016 •

edited

Loading

Amedama96 commented Nov 30, 2016 •

edited

Loading

Amedama96 commented Nov 30, 2016 •

edited

Loading