dsccommunity / SqlServerDsc

This module contains DSC resources for deployment and configuration of Microsoft SQL Server.
MIT License
360 stars 227 forks source link

Test-IsNumericType: Function moved to DscResource.Common Module #1823

Closed hollanjs closed 1 year ago

hollanjs commented 1 year ago

Pull Request (PR) description

Moved Test-IsNumericType from a private function in SqlServerDsc to a public function in DscResource.Common Related Issue adding function to DscResource.Common: Issue #87

This Pull Request (PR) fixes the following issues

Task list


This change is Reviewable

codecov[bot] commented 1 year ago

Codecov Report

Merging #1823 (cdcdaf0) into main (92612af) will decrease coverage by 0%. The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #1823   +/-   ##
====================================
- Coverage    91%     91%   -1%     
====================================
  Files        82      81    -1     
  Lines      7433    7429    -4     
====================================
- Hits       6781    6777    -4     
  Misses      652     652           
Flag Coverage Ξ”
unit 91% <ΓΈ> (-1%) :arrow_down:
hollanjs commented 1 year ago

@johlju - I think the checks are only failing because Test-IsNumericType has not been merged into DscResource.Common:main yet. That merge is queued up and ready for review as well: DscResource.Common PR #88

If it does need any additional updates, please let me know!

johlju commented 1 year ago

I will release DscResource.Common momentarily, then I re-run the pipeline on this PR.

johlju commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
johlju commented 1 year ago

Good catch removing the private functions from the previous PR's change log entries in the Unreleased section - I would have totally forgot that πŸ™‚

johlju commented 1 year ago

There seems to be a bug in other parts of the module that fails the tests, that should be unrelated to this change. I can dig into it tomorrow.

hollanjs commented 1 year ago

@johlju let me know if there's anything else I can do on my end.

johlju commented 1 year ago

I don't know how the test passed before. But good thing this bug was caught now. πŸ™‚

The problem was on this line:

https://github.com/dsccommunity/SqlServerDsc/blob/92612af8de4fedca3cb1438e8456428e21a44375/source/Private/Invoke-SetupAction.ps1#L1289

This line calls Test-IsNumericType to determine if the value is numeric. If the value is an array then Test-IsNumericType would wrongly return $false for each value, so intead of returning a single $false for arrays it would return an array of $false or $true (one for each item), making the above line always evaluate to $true for arrays.

I proposed a fix in https://github.com/dsccommunity/DscResource.Common/pull/90.

johlju commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
johlju commented 1 year ago

@hollanjs thank you for this! πŸ™‡