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

Trim the line before splitting parts in HostsFile #527

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

cdhunt
Copy link

@cdhunt cdhunt commented May 31, 2024

Pull Request (PR) description

Trim the line before splitting parts in Get-HostEntry.

This Pull Request (PR) fixes the following issues

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).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, 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 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

@PlagueHO PlagueHO self-requested a review June 1, 2024 00:23
@PlagueHO PlagueHO added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jun 1, 2024
@PlagueHO
Copy link
Member

PlagueHO commented Jun 1, 2024

The build pipeline is failing because the pipeline files haven't been updated in sometime and using a Linux build agent (which fails). I'll update the pipeline to the latest version so that you can rebase.

@cdhunt cdhunt changed the title Trim the line before splitting parts Trim the line before splitting parts in HostsFile Jun 3, 2024
@cdhunt
Copy link
Author

cdhunt commented Jun 7, 2024

@PlagueHO If you point me to an example, I can probably help.

@johlju
Copy link
Member

johlju commented Jun 7, 2024

Updated pipeline files has been merged thanks to @PlagueHO. This PR can be rebased.

@PlagueHO
Copy link
Member

PlagueHO commented Jun 7, 2024

Sorry @cdhunt - the pipeline fix was already waiting for review (#529). But there will be other DSC resource repos that need similar updates... so if you're inclined and have the time, we're always happy to have help doing the chores 😀

I've completed:

  • NetworkingDsc
  • ComputerManagementDsc
  • StorageDsc

PR needs review:

  • DFSDsc

Still need to complete:

  • xPSDesiredStateConfiguration
  • ADCertificateServicesDsc
  • CertificateDsc
  • iSCSIDsc
  • FileContentDsc
  • WSManDsc
  • FSRMDsc

@johlju - I'll probably sit down and do some of these tonight. Will raise issues and assign them to myself as I go.

@cdhunt

This comment was marked as outdated.

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97%. Comparing base (e8b5edc) to head (43ce5ae).

Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #527   +/-   ##
===================================
  Coverage    97%    97%           
===================================
  Files        28     28           
  Lines      2078   2078           
===================================
  Hits       2028   2028           
  Misses       50     50           
Files Coverage Δ
...urce/DSCResources/DSC_HostsFile/DSC_HostsFile.psm1 97% <100%> (ø)

@cdhunt cdhunt marked this pull request as ready for review June 10, 2024 19:26
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.

Thanks for submitting @cdhunt.

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @cdhunt)


CHANGELOG.md line 13 at r2 (raw file):

  - Renamed NetworkingDSc to NetworkingDsc in CHANGELOG.md - fixes [Issue #513](https://github.com/dsccommunity/NetworkingDsc/issues/513).
- HostsFile
  - Fix bad return data when line contains leading spaces - fixes [Issue #526](https://github.com/dsccommunity/NetworkingDsc/issues/526)

Nit: end with full stop :D


tests/Unit/DSC_HostsFile.Tests.ps1 line 303 at r2 (raw file):

        Context 'When a host entry has leading spaces' {

Nit: can remove blank line :)


tests/Unit/DSC_HostsFile.Tests.ps1 line 324 at r2 (raw file):

                (Get-TargetResource @testParams).Ensure | Should -Be 'Present'
            }
        }

What would happen if there was whitepace at the beginning of the Hostname parameter passed in the config? Edgecase definitely. E.g.

HostsFile Bad {
HostName = ' whitespace.com'
IPAddress = '192.168.1.1'
}

Would this result in a flapping config? As Get-TargetResource would return the trimmed value, but the parameter would still have untrimmed whitespace. Maybe add a test to check this?

E.g.

        $testParams = @{
            HostName  = ' www.leadingwhitespace.com'
            IPAddress = '127.0.0.1'
            Verbose   = $true
        }

@cdhunt
Copy link
Author

cdhunt commented Jun 11, 2024

What do you think the desired/expected behavior would be? Should it support intentional whitespace or enforce an exact format? You could use File.Contents if you want custom formatting.

@cdhunt
Copy link
Author

cdhunt commented Jun 11, 2024

@PlagueHO Can you rerun the failed integration test job?

@johlju
Copy link
Member

johlju commented Jun 11, 2024

@PlagueHO Can you rerun the failed integration test job?

I helped with that.

@cdhunt
Copy link
Author

cdhunt commented Jun 12, 2024

I've added additional tests which I believe cover at least one edge case and better cover the scenario I'm trying to fix.

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.

Nice! Thank you for adding that integration test.

Reviewed 1 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cdhunt)


tests/Unit/DSC_HostsFile.Tests.ps1 line 327 at r4 (raw file):

                $testParams = @{
                    HostName  = 'www.contoso.com'
                    IPAddress = '192.168.0.156'

What I was really thinking here, was if you created another unit test where the IPAddress = ' 192.168.0.156' and the current hosts was:
'# A mocked example of a host file - this line is a comment',
'',
'127.0.0.1 localhost',
'127.0.0.1 www.anotherexample.com',
" 192.168.0.156 $($testParams.HostName)",
'127.0.0.5 anotherexample.com',
''
Would the Test-TargetResource return True?

In theory this should because it's in state.


tests/Unit/DSC_HostsFile.Tests.ps1 line 348 at r4 (raw file):

                It 'Should return false from the test method' {
                    Test-TargetResource @testParams | Should -Be $false

Sorry, missed this - can change to Should -BeFalse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author response The pull request is waiting for the author to respond to comments in the pull request. waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author.
Projects
None yet
3 participants