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

Remove Id from PSBoundParameters in Test-TargetResource #3893

Merged
merged 15 commits into from
Nov 29, 2023

Conversation

ricmestre
Copy link
Contributor

@ricmestre ricmestre commented Nov 14, 2023

Pull Request (PR) description

After deploy the resources below they always show up as not in desired state, this is because the Id parameter is not being removed from PSBoundParameters in Test-TargetResource, by removing it the issue is solved, except that the Autopilot ones also have a problem with Assignments property not being correctly set and therefore will not be completely fixed with this PR but I raised a separate issue for that.

IntuneDeviceConfigurationDeliveryOptimizationPolicyWindows10
IntuneDeviceConfigurationHealthMonitoringConfigurationPolicyWindows10
IntuneDeviceConfigurationIdentityProtectionPolicyWindows10
IntuneWindowsAutopilotDeploymentProfileAzureADHybridJoined
IntuneWindowsAutopilotDeploymentProfileAzureADJoined

EDIT: IntuneDeviceConfigurationEndpointProtectionPolicyWindows10 is affected by similar problem but not by not removing Id but CertificateThumbprint.
EDIT2: IntuneDeviceEnrollmentStatusPageWindows10 has both problems, it needs to have Id and all auth methods removed from Test, but also has problems with Assignments as reported on #3892

This Pull Request (PR) fixes the following issues

@ricmestre
Copy link
Contributor Author

@William-Francillette If you have time and this PR is accepted could you please check the DRG in order to have similar changes as I have here?

@William-Francillette
Copy link
Contributor

@NikCharlebois, @ykuijs @andikrueger
Is there any use cases in checking Id in Test-TargetResource? If not, we may update Test-M365DSCParameterState from M365DSCUtil (around line 614) and force skipping Id as we do for the credentials parameters

@andikrueger
Copy link
Collaborator

I just try to understand why to remove the ID from test-TargetResource. Usually we would compare the ID if it’s passed into the resource as a desired parameter.

If it is set, we should compare it.

E.g. someone wants to change the DisplayName (key) of an object. This would only work, if we monitor the ID as well.

Please correct me if I got the purpose wrong or the context of this change.

@ricmestre
Copy link
Contributor Author

@andikrueger If Id is not removed and is kept in the blueprint then when applying the policy to another tenant is not an issue but will complain that the resource is not in desired state because of course the Ids are different between tenants.

This is the same situation if Id is removed, now that it's not mandatory you can remove it therefore since the property is not there it will fallback to DisplayName and will just work, as long as the resource has been updated to do that, I know of some really old resources that if cannot be found by Id it won't fallback to DisplayName but that's another issue for another day.

@NikCharlebois
Copy link
Collaborator

@NikCharlebois, @ykuijs @andikrueger Is there any use cases in checking Id in Test-TargetResource? If not, we may update Test-M365DSCParameterState from M365DSCUtil (around line 614) and force skipping Id as we do for the credentials parameters

There are cases where even if the property is named Id (or Identity) it is represented by a friendly display name instead of a GUID. In these cases we need to keep ID as a param in the Test-TargetResource.

Copy link
Member

@ykuijs ykuijs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ykuijs ykuijs merged commit ed9cf9c into microsoft:Dev Nov 29, 2023
2 checks passed
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.

Intune: Some resources can be deployed but fail test on Test-TargetResource method
5 participants