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

CCM Client Utilities should be off by default #13

Open
mgreenegit opened this issue Feb 25, 2018 · 5 comments
Open

CCM Client Utilities should be off by default #13

mgreenegit opened this issue Feb 25, 2018 · 5 comments
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community.

Comments

@mgreenegit
Copy link
Contributor

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
Copy link
Member

johlju commented May 3, 2018

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

@johlju johlju added enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community. breaking change When used on an issue, the issue has been determined to be a breaking change. labels May 3, 2018
@mgreenegit
Copy link
Contributor Author

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
Copy link
Member

johlju commented May 4, 2018

@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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community.
Projects
None yet
Development

No branches or pull requests

3 participants