ADDomainController: Ensure Property should be renamed or reimplemented

[michaeltlombardi]

Details of the scenario you tried and the problem that is occurring

The schema for the ADDomainController Resource includes the Ensure property as a read-only string:

ActiveDirectoryDsc/source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.schema.mof

Line 13 in 9346a1d

[Read, Description("Returns the state of the Domain Controller.")] String Ensure;

This is counter to the expectation and idiomatic implementation of the Ensure property across DSC Resources, which is taken to be the enforceable state of a Resource.

Furthermore, the actual implementation of Get-TargetResource for ADDomainController returns $true or $false for Ensure, which is also non-idiomatic (I would expect Present or Absent):

ActiveDirectoryDsc/source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1

Line 96 in 9346a1d

Ensure = $true

ActiveDirectoryDsc/source/DSCResources/MSFT_ADDomainController/MSFT_ADDomainController.psm1

Line 118 in 9346a1d

Ensure = $false

Suggested solution to the issue

I suggest either renaming the read-only Ensure property to something like IsPromoted (with a Boolean type) or reworking the Resource entirely such that it becomes an ensurable resource and the Ensure property is idiomatic.

Version of the DSC module that was used

• Latest code on main

[gaelcolas]

Agreed on all accounts.
Given when this resource was originally written, I guess the reason it's still this way is to minimize breaking changes and the lack of time to refactor.

That said, you're right and I'll let the maintainers of @dsccommunity/activedirectorydsc to comment on feasibility and preferred approach.

[johlju]

Yes it should use Ensure property as any other DSC resource, meaning using the two values Present and Absent. If I recall correctly the current implementation was a workaround to fix a bug and not make a breaking change.

This issue is related to issue #251 which suggest that the ADDomainController should be able to demote a domain controller.

[johlju]

We are happy to review a PR that implements this breaking change, preferably that also fixes issue #251. 🙂

[michaeltlombardi]

We are happy to review a PR that implements this breaking change, preferably that also fixes issue #251. 🙂

I probably can't commit to implementing the PR myself (I'm hip-deep in the Puppet-DSC integration work myself, we found this via a customer raising a bug) at this time, unfortunately.

[johlju]

No worries @michaeltlombardi. Hopefully someone in the community can pick this up.