Changed logic to Evaluate Ensure

[NikCharlebois]

Pull Request (PR) description

• Modified 100+ Test-TargetResource logic to prevent returning $false when the Ensure parameter doesn't match the desired value. While this could introduce a very small performance gain in some cases, it resulted in a lot of drifts being detected without proper logging. All evaluation, including of the Ensure property, is now being handled by the Test-M365DSCParameterState function.

This Pull Request (PR) fixes the following issues

• N/A

[FabienTschanz]

@NikCharlebois Just FYI, a couple resources are still left with the formatting. Making sure we catch all of them 💪

[FabienTschanz]

@NikCharlebois The resource IntuneDeviceCompliancePolicyAndroid is the last one (although marked as resolved, it's not yet fixed).

[FabienTschanz]

@NikCharlebois Quick question, why was the $Script:exportedInstance updated to plural $Script:exportedInstances? Any specific reason for that? In the Export-TargetResource loop, we're assigning the current loop item to this variable, so the plural would indicate there are multiple instances of it (like an array). But instead it's simply the current item. Wouldn't it make more sense to just leave it be?

[NikCharlebois]

@NikCharlebois Quick question, why was the $Script:exportedInstance updated to plural $Script:exportedInstances? Any specific reason for that? In the Export-TargetResource loop, we're assigning the current loop item to this variable, so the plural would indicate there are multiple instances of it (like an array). But instead it's simply the current item. Wouldn't it make more sense to just leave it be?

There were a lot of confusion in the resources where some places where using the plural, others weren't. This was causing the performance improvements that this was meant to introduce to be completely skipped.

[FabienTschanz]

@NikCharlebois But now I think we might have a problem for a couple of resources that were already using $Script:exportedInstances to assign all of the exported resources to it, e.g. EXOManagementRole:

Microsoft365DSC/Modules/Microsoft365DSC/DSCResources/MSFT_EXOManagementRole/MSFT_EXOManagementRole.psm1

Lines 377 to 397 in fdfb259

	[array] $Script:exportedInstances = Get-ManagementRole | Where-Object -FilterScript { $_.Parent -ne $null }
	$dscContent = [System.Text.StringBuilder]::New()
	if ($Script:exportedInstances.Length -eq 0)
	{
	Write-Host $Global:M365DSCEmojiGreenCheckMark
	}
	else
	{
	Write-Host "`r`n" -NoNewline
	}
	$i = 1
	foreach ($ManagementRole in $Script:exportedInstances)
	{
	if ($null -ne $Global:M365DSCExportResourceInstancesCount)
	{
	$Global:M365DSCExportResourceInstancesCount++
	}
	Write-Host " |---[$i/$($Script:exportedInstances.Count)] $($ManagementRole.Name)" -NoNewline

Here, $Script:exportedInstances gets all of the current roles assigned to it, and $Script:exportedInstance contains just the single element of the loop. If you now rename the singular to the plural and leave it be, I don't know what will happen. After the first element of exportedInstances is processed, it gets assigned the current loop item, and if there would have been a second item, I suspect it would be skipped since foreach ($ManagementRole in $Script:exportedInstances) always looks at the loop object (which now only contains a single item). Does that make any sense? If there is a better name, feel free to change that, but I believe that renaming it to its plural counterpart will cause issues with resources that use it for looping through all the instances and not just reference the current item.

[NikCharlebois]

@NikCharlebois But now I think we might have a problem for a couple of resources that were already using $Script:exportedInstances to assign all of the exported resources to it, e.g. EXOManagementRole:

Microsoft365DSC/Modules/Microsoft365DSC/DSCResources/MSFT_EXOManagementRole/MSFT_EXOManagementRole.psm1

Lines 377 to 397 in fdfb259

	[array] $Script:exportedInstances = Get-ManagementRole | Where-Object -FilterScript { $_.Parent -ne $null }
	$dscContent = [System.Text.StringBuilder]::New()
	if ($Script:exportedInstances.Length -eq 0)
	{
	Write-Host $Global:M365DSCEmojiGreenCheckMark
	}
	else
	{
	Write-Host "`r`n" -NoNewline
	}
	$i = 1
	foreach ($ManagementRole in $Script:exportedInstances)
	{
	if ($null -ne $Global:M365DSCExportResourceInstancesCount)
	{
	$Global:M365DSCExportResourceInstancesCount++
	}
	Write-Host " |---[$i/$($Script:exportedInstances.Count)] $($ManagementRole.Name)" -NoNewline

Here, $Script:exportedInstances gets all of the current roles assigned to it, and $Script:exportedInstance contains just the single element of the loop. If you now rename the singular to the plural and leave it be, I don't know what will happen. After the first element of exportedInstances is processed, it gets assigned the current loop item, and if there would have been a second item, I suspect it would be skipped since foreach ($ManagementRole in $Script:exportedInstances) always looks at the loop object (which now only contains a single item). Does that make any sense? If there is a better name, feel free to change that, but I believe that renaming it to its plural counterpart will cause issues with resources that use it for looping through all the instances and not just reference the current item.

Rolling back for now as this is not a top priority

[NikCharlebois]

@ricmestre if you have a few minutes, could you please verify this change since it touches 100+ resources. In the past, we would have a separate check in Test-TargetResource for some resources to see if the Ensure property differed (e.g, the resource didn't exist and it should have). This made for a faster evaluation in some cases, but it never actually logged any drifts in the logs. We've modify this logic so that all parameter evaluation, including the Ensure parameter, is always performed by the Test-M365DSCParameterState function instead.

[ricmestre]

@NikCharlebois I just found out this issue #5666, I need it fixed before testing your diff.

[NikCharlebois]

@NikCharlebois I just found out this issue #5666, I need it fixed before testing your diff.

Looks like #5666 is fixed. Let me know once you've had a chance to look at the current PR. Thanks

[ricmestre]

Looks like "everything" works, meaning the PR is ok to be merged but while running my tests I found other issues, I'll raise separate issues for them.