search

dsccommunity / xPendingReboot

MIT License
31 stars 10 forks source link

CCM Client Utilities should be off by default #13

Open mgreenegit opened 6 years ago

mgreenegit commented 6 years ago

After testing, my general feedback is that the CCM Client Utilities SDK check should be off by default. That would return the resource to a "just works" state without having to explicitly turn off the capability. I am looking for feedback from others.

johlju commented 6 years ago

I feel that the current implementation of SkipCcmClientSDK is correct. If the configuration should not skip CcmClientSDK the parameter can be left out, and if if should skip the parameter is set to $true. If the default value should be $true then I think it would be better for the parameter to be named IncludeCcmClientSDK. That would be a breaking change. The default value for parameter SkipCcmClientSDK is $false. Changing it to to $true as the default value would be a breaking change

mgreenegit commented 6 years ago

The problem I see here is that not all machines have the Ccm Client SDK installed. I would expect the default behavior to work for all machines and then a switch to include a scenario for a subset of machines.

I would also like to do a rename to PendingRebootDSC so if we are going to make a breaking change, that would be the right time. Moving to the other version would be a purposeful decision.

johlju commented 6 years ago

@mgreenegit agree if that is the case, and to rename the parameter which would be more logical.

In regards to renaming the resource module, in issue #12 it is being discussed if we instead should move the resource. 🤔

PlagueHO commented 5 years ago

After starting the move of this resource over to ComptuerManagementDsc and reviewing the code, I agree with @mgreenegit - the SkipCcmClientSDK should default to true.

But I also noticed a larger problem that I'll also fix: Regardless if CcmClientSDK returns $true (assuming SkipCcmClientSDK is false), a reboot will never be triggered. E.g. the functionality is completely broken. There is no check in Test-TargetResrouce against the result of the CcmClientSDK.

I'll fix both of these issues during the migration across to ComputerManagementDsc.

PlagueHO commented 5 years ago

This has been fixed over in the new PendingReboot in ComputerManagementDsc. @gaelcolas - can you close this one?