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

Fix nested change detection in CIMInstance and resource comparison #4126

Merged
merged 9 commits into from
Feb 14, 2024

Conversation

FabienTschanz
Copy link
Contributor

@FabienTschanz FabienTschanz commented Jan 5, 2024

Pull Request (PR) description

This pull request addresses the following two issues (without GitHub issues) in the M365DSCReport submodule:

  • Nested change detection if an array contains multiple CIMInstances of a resource. Previously, even though the two compared resources were identical, some changes would be found because the module did not handle the array elements in the proper way to recognize if the destination array actually contained the element of the reference array.
  • Comparison of IntuneDeviceEnrollmentPlatformRestriction now uses the ResourceInstanceName instead of the DisplayName property to check if such a resource exists. Previously, it used the display name property, which led to an incorrect selection of the resource if e.g. a Android platform restriction policy was created in Intune. Such policies may contain multiple definitions (Android and AndroidForWork), which are saved as separate resources.

Additionally, an if condition (which was always true) was removed:

[string]$key = ''
$null -ne $key --> Always returns $true, because a String is never $null. 

This Pull Request (PR) fixes the following issues

@FabienTschanz
Copy link
Contributor Author

Any chance this will be merged anytime near in the future? Would be great to have the comparison work without error without having to copy the fix to the module directory on every update.

@andikrueger andikrueger linked an issue Feb 6, 2024 that may be closed by this pull request
@andikrueger
Copy link
Collaborator

@FabienTschanz Will this also resolve this issue here: #4291

@ricmestre
Copy link
Contributor

I didn't test it but it would be cool if it also solved this old one I raised #3472, and I've also seen a problem with SettingDefinitionId on IntuneSettingCatalogCustomPolicyWindows10 which I've not reported yet.

@FabienTschanz
Copy link
Contributor Author

@andikrueger I'll check and report back whether it fixes that issue as well.
@ricmestre Is the template still the same and reproducible? I can look into that on Friday, unfortunately not earlier.

@ricmestre
Copy link
Contributor

Unfortunately yes, this was never fixed.

@FabienTschanz
Copy link
Contributor Author

@andikrueger Nope, that issue wasn't fixed with my update. But I updated one condition to exclude resources from the comparison if an exact match was already found.

Given the following example:
If I compare my three resources from #4291 to the destination with only two, the two matching resources will be excluded after they've been matched against each other and only the last resource will be marked as "missing" (better said: configured differently, because they are in the same "bigger resource").

@ricmestre My proposed fix does indeed change the behavior of your case from #3472, but if I deploy such a blueprint and the ids aren't matching, it will at least show that all of those settings are different (better said: missing). Since there is no way to ignore properties from the inner resources (in your case the MSFT_IntuneGroupPolicyDefinitionValue), that unfortunately will always be the case. At least it shows that correctly and not in the weird way you were facing. So it's a semi-fix.

@ricmestre
Copy link
Contributor

OK so still not fixed for me :) I'll keep my issue opened, once this is in I'll update it with how the assertion gets done with your diff.

It's definetely better so I'm not against this, but this comparison engine really should be looked at, one of my biggest sells with our customers is having a monitoring tool that can assert the drifts between tenants against a single blueprint

@NikCharlebois NikCharlebois merged commit 9d039e4 into microsoft:Dev Feb 14, 2024
2 checks passed
@ykuijs
Copy link
Member

ykuijs commented Feb 14, 2024

This morning I submitted a PR for the DSCParser that fixes an issue I encountered at a customer with nested CIM Instances. This might be the same as this issue. So it might be fixed in todays release where the new version of the DSCParser is included.

@ricmestre
Copy link
Contributor

@ykuijs I also have a long standing issue raised here microsoft/DSCParser#34 that I also spoke with @NikCharlebois about it, unfortunately I don't have time right now to test your fix so I'll look at it today during evening or by tomorrow but I'd love for this to work since it would fix IntuneSettingCatalogCustomPolicyWindows10 which is VERY important to us.

@ykuijs
Copy link
Member

ykuijs commented Feb 14, 2024

Let me guess, it was the Settings\SettingsInstance\choiceSettingValue setting that broke down?

That is the exact issue I fixed for my customer!

@ykuijs
Copy link
Member

ykuijs commented Feb 14, 2024

Just checked the issue you are referring to and indeed, I worked on the exact same issue! Please test with todays release to see if the issue is fixed!

@FabienTschanz FabienTschanz deleted the fix-dsc-report branch April 26, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AADGroup Assigned Licenses Comparison Not Working As Intended
5 participants