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

Migrate Intune LAPS to new Settings Catalog Cmdlets #4743

Merged

Conversation

FabienTschanz
Copy link
Contributor

Pull Request (PR) description

This pull request does the following:

  • Fixes an issue in Test-M365DSCParameterState that prevents it from dealing with desired values that are $null
  • Fixes a couple of minor issues when dealing with setting catalog policies and values, especially the definitions
  • Migrates the Intune LAPS resource to the new cmdlets for improved stability

This Pull Request (PR) fixes the following issues

None.

@FabienTschanz FabienTschanz force-pushed the ref/migrate-laps-setting-catalog branch from 7f5dac0 to 06918f3 Compare June 7, 2024 09:00
@@ -19,9 +19,9 @@ class MSFT_IntuneAccountProtectionLocalAdministratorPasswordSolutionPolicy : OMI
[Write, Description("Configures which directory the local admin account password is backed up to. 0 - Disabled, 1 - Azure AD, 2 - AD"), ValueMap{"0", "1", "2"}, Values{"0", "1", "2"}] UInt32 BackupDirectory;
[Write, Description("Configures the maximum password age of the managed local administrator account for Azure AD. Minimum - 7, Maximum - 365")] UInt32 PasswordAgeDays_AAD;
[Write, Description("Configures the maximum password age of the managed local administrator account for Active Directory. Minimum - 1, Maximum - 365")] UInt32 PasswordAgeDays;
[Write, Description("Configures additional enforcement of maximum password age for the managed local administrator account.")] Boolean PasswordExpirationProtectionEnabled;
[Write, Description("Configures additional enforcement of maximum password age for the managed local administrator account."), ValueMap{"true", "false"}, Values{"true", "false"}] String PasswordExpirationProtectionEnabled;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we changing the type here, can't we cast to Boolean and keep the same type in the schema? Changing a type, even if there is a value map, could result in a breaking change.

Copy link
Contributor Author

@FabienTschanz FabienTschanz Jun 11, 2024

Choose a reason for hiding this comment

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

@NikCharlebois The reason for that is because Export-IntuneSettingCatalogPolicySettings returns the values for the corresponding property based on the definition of the property in the settings catalog schema. The definition for PasswordExpirationProtectionEnabled is a StringSettingValue with true and false. Leaving the data type like that in the schema without a manual cast to Boolean would result in an incorrect configuration that cannot be compiled to MOF again. But to minimize any custom configuration in the logic, I disregarded a cast to Boolean, because it would only make the logic far more complex than a schema change for practically the same values (but strings and not Boolean).

Your concerns were my concerns as well, that's why I checked with a previously exported configuration and applied that with the new logic. Previously, since booleans were exported, and booleans are either True or False, they will implicitly be casted to the appropriate string value, matching the specified ValueSet in the schema and parameter definition. From that side, there is no breaking change possible.

Example: $True as a string is True, and $False is False. Both of these strings match the ValueSet, since PowerShell is case-insensitive.

Real example of a configuration:
PasswordExpirationProtectionEnabled = $True; will be compiled into PasswordExpirationProtectionEnabled = "true";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NikCharlebois Any news? I honestly don't expect any disruption because the types are compatible with each other. But if you absolutely want me to leave the type, I can do that too, but when we want to align the resources, we should stick to a generalized approach wherever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NikCharlebois Can you please review the PR or tell me what to do? I have other PRs that are somewhat depending on the decision if we merge this one like it is or not. Thank you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is a breaking change, we need to follow the breaking change policy.

https://microsoft365dsc.com/concepts/breaking-changes/

This means that this PR cannot be merged until the first release of October to ensure minimal disruption on the customers.

Copy link
Contributor Author

@FabienTschanz FabienTschanz Jun 26, 2024

Choose a reason for hiding this comment

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

@NikCharlebois I disagree. The handling of the type (strings with true and false vs boolean) is identical, as proven by the example I gave earlier. But if you absolutely want it that way, I can change the implementation to still use the boolean type. Do you want me to do it that way?

Or I can prove it to you with some real examples that there is no disruption with the updated MOF definition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, but there are other more complex scenarios we always need to account for. Lots of customers are taking monthly snapshots (export) of their environment and running a discrepancy report over time. This would now start generating drifts where at some point it was equal to $false then later became "false"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NikCharlebois I reverted the breaking changes and added a small workaround that works with the string values behind the scenes. There shouldn't be any more issues now.

@NikCharlebois NikCharlebois merged commit 52c8ecf into microsoft:Dev Jun 26, 2024
2 checks passed
@FabienTschanz FabienTschanz deleted the ref/migrate-laps-setting-catalog branch June 26, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants