-
Notifications
You must be signed in to change notification settings - Fork 618
PPTenantSettings: EnableDesktopFlowDataPolicyManagement property type… #6741
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
base: Dev
Are you sure you want to change the base?
Conversation
… change String -> Boolean
|
Appreciate the fix. Unfortunately because this changes the type of the parameter, it is considered a breaking change. Boolean parameters are difficult because any non-null string is considered as a true-ish value, except for the number 0 which resolves to $false. Edit: To not block this change, what you could do is a |
|
I'm not sure to understand what you mean by using from the LCM because the of the exception mechanism in the Utils module |
|
@1Dimitri If I do an export of the configuration, will the property One addition: You missed the parameter in the Test-TargetResource function. It needs to be updated there as well. I will check the changes again this evening, with your modifications, and get back to you. |
FabienTschanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small things to fix. One is the type in the Test-TargetResource function and the other is the version of the CIM class in the schema file. Otherwise, it looks good to me.
| [Parameter()] | ||
| [System.String] | ||
| $EnableDesktopFlowDataPolicyManagement, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| [Parameter()] | |
| [System.Boolean] | |
| $EnableDesktopFlowDataPolicyManagement, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit late to the party, but yes the property is exported with "False" or "True" instead of $False or $True.
Thanks for the review.
Indeed I forgot the Test-Resource when I back ported the modification from my corporate test environment, and I did not think about bumping the schema version (is there a documentation about the revision number scheme somewhere for that -bug, add/removal of property, ... -?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really - just bump it up by one for the last number. As long as the number is increased, it doesn‘t really matter.
| @@ -1 +1 @@ | |||
| [ClassVersion("1.0.0.0"), FriendlyName("PPTenantSettings")] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| [ClassVersion("1.0.0.1"), FriendlyName("PPTenantSettings")] |
… change String -> Boolean
Pull Request (PR) description
In the mof file and psm1 the EnableDesktopFlowDataPolicyManagement property is defined as string whereas it is a boolean.
This leads to warnings or error with Set-TargetResource.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).