Open hungtran84 opened 2 years ago
We should also add this text to the description in the schema mof since the schema MOF is used to generate the wiki documentation.
Already added to schema MOF file (if I'm not wrong)
If the current state already has a different endpoint, should not Test-function return $false so that Set-function kan set the correct endpoint? When the endpoint is part of the configuration and the value is in desired state should Test-function return $true?
I tried to remove the default value of Endpoint and set the mock value but no luck. Actually, I don't know much about DSC Unit test and struggle on trousbleshooting/resolving the failed test case.
@johlju could you help to solve it? Thank you in advanced
Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.
/AzurePipelines run
Thank you for your suggestion. I'll be working on it soonest.
On Sun, Jul 31, 2022, 20:49 Johan Ljunggren @.***> wrote:
@.**** requested changes on this pull request.
Reviewable https://reviewable.io/reviews/dsccommunity/failoverclusterdsc/279 status: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @hungtran84 https://github.com/hungtran84 and @johlju https://github.com/johlju)
source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 line 256 at r2 https://reviewable.io/reviews/dsccommunity/failoverclusterdsc/279#-N8JVVpu8C69Be0MkFj1:-N8JVVpu8C69Be0MkFj2:b5r8pkw (raw file https://github.com/dsccommunity/failoverclusterdsc/blob/2147a57d86ea8a7d7d441a117dadb249f1eba50b/source/DSCResources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1#L256):
$getGetTargetResourceResult = Get-TargetResource -IsSingleInstance $IsSingleInstance $testTargetResourceReturnValue = $false
This should only do the evaluation if the parameters was assigned. So maybe we have tu reverse it to something like this (this will also fix Resource):
$testTargetResourceReturnValue = $true if ($getGetTargetResourceResult.Type -ne $Type) { $testTargetResourceReturnValue = $false } if ($PSBoundParameters.ContainsKey('Resource') -and $getGetTargetResourceResult.Resource -ne $Resource) { $testTargetResourceReturnValue = $false } if ($PSBoundParameters.ContainsKey('Endpoint') -and $getGetTargetResourceResult.Endpoint -ne $Endpoint) { $testTargetResourceReturnValue = $false }
Suggestion:
$testTargetResourceReturnValue = $true if ($getGetTargetResourceResult.Type -ne $Type) { $testTargetResourceReturnValue = $false } if ($PSBoundParameters.ContainsKey('Resource') -and $getGetTargetResourceResult.Resource -ne $Resource) { $testTargetResourceReturnValue = $false } if ($PSBoundParameters.ContainsKey('Endpoint') -and $getGetTargetResourceResult.Endpoint -ne $Endpoint) { $testTargetResourceReturnValue = $false }
— Reply to this email directly, view it on GitHub https://github.com/dsccommunity/FailoverClusterDsc/pull/279#pullrequestreview-1056570634, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHGGMR7M7QRAQU5MEICRIRDVWZ75PANCNFSM52C4OAEA . You are receiving this because you were mentioned.Message ID: @.***>
@hungtran84 let me know when you done above changes and I take a look again.
Merging #279 (7ec21f6) into main (faa9aa3) will not change coverage. The diff coverage is
100%
.
@@ Coverage Diff @@
## main #279 +/- ##
===================================
Coverage 100% 100%
===================================
Files 8 8
Lines 611 619 +8
===================================
+ Hits 611 619 +8
Impacted Files | Coverage Δ | |
---|---|---|
...Resources/DSC_ClusterQuorum/DSC_ClusterQuorum.psm1 | 100% <100%> (ø) |
@johlju It seems that all the tests passed, could you help to review my PR again?
Great work! I will review as soon as I have a chance.
Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.
Pull Request (PR) description
This PR is created for adopting Azure Government cloud where the resource endpoint is required.
This Pull Request (PR) fixes the following issues
Task list
This change is