Issue#20

[jcwalker]

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Fixes #20

Task list

• [ x] 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).
• [ x] Resource documentation added/updated in README.md in resource folder.
• [ x] Resource parameter descriptions added/updated in schema.mof
  and comment-based help.
• [x ] Comment-based help added/updated.
• [ x] Localization strings added/updated in all localization files as appropriate.
• [x ] Examples appropriately added/updated.
• [ x] Unit tests added/updated. See DSC Resource Testing Guidelines.
• [ x] Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• [x ] New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[codecov]

Codecov Report

Merging #21 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@        Coverage Diff         @@
##           dev   #21    +/-   ##
==================================
+ Coverage   90%   91%   +<1%     
==================================
  Files        5     5            
  Lines      233   246    +13     
==================================
+ Hits       211   224    +13     
  Misses      22    22

[PlagueHO]

Thanks @jcwalker - very much appreciated! I'll review this week.

[nehrua]

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

---

Tests/Unit/DSR_ReplaceText.Tests.ps1, line 63 at r2 (raw file):

"@

$script:testFileExpectedTextContentNewKey = @"

Tab should be added.

---

Tests/Unit/TestFile.txt, line 6 at r4 (raw file):

Setting.Two='TestText'
Setting3.Test=Value4
Setting.NotExist='TestText'

This is a file generated by the unit tests. You probably didn't mean to include this with the PR?

[winthrop28]

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

[PlagueHO]

Great stuff @jcwalker

I've been thinking about this and I think it might be worth adding a new parameter to the resource that enables the new behaviour of allowing the string to be added to the end of the file. E.g. Boolean AllowAppend.

Otherwise anyone using this as a templating system will end up being broken. Does that make sense?

Reviewed 1 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jcwalker)

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 249 at r4 (raw file):

    if ($results.Count -eq 0)
    {
        # No matches found - already in state

This comment needs to be corrected to match actual behavior.

I think though we need a boolean switch as mentioned above to disable appending a missing replace string. We should default the behavior to disabling this AllowAppend to $false to try and avoid a breaking change.

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 358 at r4 (raw file):

        The text to replace the text identifed by the RegEx.
#>
function Add-ConfigurationEntry

Can we add some unit tests for this function?

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 371 at r4 (raw file):

    )

    $stringBuilder = New-Object System.Text.StringBuilder

Can you use named parameter (e.g. New-Object -TypeName System.Text.StringBuilder)

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 377 at r4 (raw file):

    foreach ($line in $fileContentArray)
    {
        $null = $stringBuilder.AppendLine($line.Trim())

Is it risky to trim the line here? E.g. if the number of spaces at the beginning or end of the line is important? This would result in other unintended changes being made to the text file - but only when appending text.

---

Tests/Unit/DSR_ReplaceText.Tests.ps1, line 220 at r4 (raw file):

            }

            Context 'File exists and search text can not be found'{

Can you add a space before the {?

---

Tests/Unit/DSR_ReplaceText.Tests.ps1, line 403 at r4 (raw file):

                }

                It 'Should return true' {

This 'It' message needs to be corrected.

---

Tests/Unit/DSR_ReplaceText.Tests.ps1, line 600 at r4 (raw file):

            }

            Context 'File exists and search text cannot be found' {

There aren't any tests in here 😁

---

Tests/Unit/TestFile.txt, line 6 at r4 (raw file):

Previously, nehrua (Nehru Ali) wrote…

This is a file generated by the unit tests. You probably didn't mean to include this with the PR?

Good catch @nehrua 😁 Ideally this wouldn't have been created if the Mocks had prevented the real call from working.

[jcwalker]

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 358 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we add some unit tests for this function?

I was thinking how this should be done since there is no logic in the function. Do I just test for that it returns a sting? Or do I test that actual output of the function?

[jcwalker]

Reviewable status: 2 of 4 files reviewed, 9 unresolved discussions (waiting on @PlagueHO, @nehrua, @winthrop28, and @jcwalker)

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 249 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This comment needs to be corrected to match actual behavior.

I think though we need a boolean switch as mentioned above to disable appending a missing replace string. We should default the behavior to disabling this AllowAppend to $false to try and avoid a breaking change.

I think I understand. I'll add the code and you can tell me if I'm on the right track ;)

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 371 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use named parameter (e.g. New-Object -TypeName System.Text.StringBuilder)

Done

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 377 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is it risky to trim the line here? E.g. if the number of spaces at the beginning or end of the line is important? This would result in other unintended changes being made to the text file - but only when appending text.

Done.

Removed the trim.

---

Tests/Unit/DSR_ReplaceText.Tests.ps1, line 63 at r2 (raw file):

Previously, nehrua (Nehru Ali) wrote…

Tab should be added.

Since this is a here string it can't have tabs.

---

Tests/Unit/DSR_ReplaceText.Tests.ps1, line 220 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a space before the {?

Done.
Good catch

---

Tests/Unit/DSR_ReplaceText.Tests.ps1, line 403 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This 'It' message needs to be corrected.

Done.

---

Tests/Unit/DSR_ReplaceText.Tests.ps1, line 600 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

There aren't any tests in here 😁

🤦

---

Tests/Unit/TestFile.txt, line 6 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Good catch @nehrua 😁 Ideally this wouldn't have been created if the Mocks had prevented the real call from working.

Done.

[jcwalker]

---

Tests/Unit/DSR_ReplaceText.Tests.ps1, line 600 at r4 (raw file):

Previously, jcwalker (Jason Walker) wrote…

🤦

Looking at the test I added the tests for this scenario at the top of the describe block

Done.

[jcwalker]

Reviewable status: 1 of 6 files reviewed, 9 unresolved discussions (waiting on @PlagueHO, @nehrua, and @winthrop28)

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 249 at r4 (raw file):

Previously, jcwalker (Jason Walker) wrote…

I think I understand. I'll add the code and you can tell me if I'm on the right track ;)

OK I added a AllowAppend bool to the resource.

[nehrua]

Reviewed 4 of 5 files at r5.
Reviewable status: 5 of 6 files reviewed, 7 unresolved discussions (waiting on @nehrua, @winthrop28, and @PlagueHO)

[PlagueHO]

Sorry to take so long @jcwalker - I'll get onto this tomorrow night! It got a bit crazy at work. Just working through my backlog of other reviews as well 😁

[jcwalker]

@PlagueHO sounds good!

[PlagueHO]

Thanks @jcwalker - just some minor style tweaks and a question around the behavior of the append when used with Unix format files.

Reviewed 4 of 5 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @PlagueHO and @jcwalker)

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 358 at r4 (raw file):

Previously, jcwalker (Jason Walker) wrote…

I was thinking how this should be done since there is no logic in the function. Do I just test for that it returns a sting? Or do I test that actual output of the function?

Ideally you want to test an input with the function and ensure the output is expected. You'd want to test the common case and edge cases if any (e.g. when the strings are empty

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 106 at r5 (raw file):

        The secret text to replace the text identified by the RegEx.
        Only used when Type is set to 'Secret'.
#>

Can you add the description of the new parameter to this block (this is a requirement of High Quality Resource Module guidelines).

And in the Set* and Test*.

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 377 at r5 (raw file):

    .PARAMETER Text
        The text to replace the text identifed by the RegEx.

Should this read 'The text to append to the end of the FileContent.' - or something to that effect?

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 396 at r5 (raw file):

    $stringBuilder = New-Object -TypeName System.Text.StringBuilder

    $fileContentArray = $FileContent.Trim() -split '\n'

This actually seems like potentially destructive to any text that doesn't use Windows CRLF (e.g. Unix line endings). Wouldn't this code convert a Unix line ending file to a Windows CRLF - but only if the string was being appended?

Is it possible to detect the line ending format and automatically use that? This would also allow you to remove the expensive loop and just use:

$stringBuilder.Append($line)
$stringBuilder.Append($text)
$stringBuilder.Append($detectedNewLineFormat)

You could probably detect the new Line format by just searching for CRLF - if one exists then it is Windows. Then check for LF - if one exists then it is Unix format - if neither then default to CRLF (e.g. the file is either empty or has no new lines in it at all).

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.schema.mof, line 9 at r5 (raw file):

  [Write, Description("The text to replace the text identified by the RegEx. Only used when Type is set to 'Text'.")] String Text;
  [Write, Description("The secret text to replace the text identified by the RegEx. Only used when Type is set to 'Secret'."),EmbeddedInstance("MSFT_Credential")] String Secret;
  [Write, Description("Specifies to append text to the file being modified. Adds the ability to add a configuration entry")] Boolean AllowAppend;

Can you end in a full stop?

---

Tests/Unit/TestFile.txt, line 6 at r4 (raw file):

Previously, jcwalker (Jason Walker) wrote…

Done.

I think the file still needs to be removed from the commit.

[jcwalker]

Reviewable status: 3 of 6 files reviewed, 5 unresolved discussions (waiting on @nehrua, @PlagueHO, and @jcwalker)

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 106 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add the description of the new parameter to this block (this is a requirement of High Quality Resource Module guidelines).

And in the Set* and Test*.

Done.

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 377 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should this read 'The text to append to the end of the FileContent.' - or something to that effect?

Done.

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 396 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This actually seems like potentially destructive to any text that doesn't use Windows CRLF (e.g. Unix line endings). Wouldn't this code convert a Unix line ending file to a Windows CRLF - but only if the string was being appended?

Is it possible to detect the line ending format and automatically use that? This would also allow you to remove the expensive loop and just use:

$stringBuilder.Append($line)
$stringBuilder.Append($text)
$stringBuilder.Append($detectedNewLineFormat)

You could probably detect the new Line format by just searching for CRLF - if one exists then it is Windows. Then check for LF - if one exists then it is Unix format - if neither then default to CRLF (e.g. the file is either empty or has no new lines in it at all).

Thanks for pointing this out. I implemented your suggestion and the code looks much clearer (and works better) for it.

Done.

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.schema.mof, line 9 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you end in a full stop?

What do you mean by "full stop"?

---

Tests/Unit/TestFile.txt, line 6 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I think the file still needs to be removed from the commit.

#FacePalm

[jcwalker]

Reviewable status: 3 of 6 files reviewed, 5 unresolved discussions (waiting on @nehrua and @PlagueHO)

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.psm1, line 358 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Ideally you want to test an input with the function and ensure the output is expected. You'd want to test the common case and edge cases if any (e.g. when the strings are empty

Done.

[PlagueHO]

Just the one tiny thing and then merge! Great stuff!

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

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.schema.mof, line 9 at r5 (raw file):

Previously, jcwalker (Jason Walker) wrote…

What do you mean by "full stop"?

Sorry - realized that wasn't at all clear. I meant put a full stop at the end of the description. Sorry about that!

[jcwalker]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @PlagueHO and @nehrua)

---

DSCResources/DSR_ReplaceText/DSR_ReplaceText.schema.mof, line 9 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Sorry - realized that wasn't at all clear. I meant put a full stop at the end of the description. Sorry about that!

Done.

[nehrua]

@PlagueHO just checking in on the status of this PR. I believe I've resolved/acknowledge my comments. Are there any unresolved comments you require be addressed before merging?

[PlagueHO]

Awesome stuff! Will merge now and release to PS Gallery.

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