NetAdapterBinding: Use of State & CurrentState Properties is non-idiomatic

[michaeltlombardi]

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

The schema for the NetAdapterBinding Resource includes both State (writable enum) and CurrentState (read-only enum) for the same information on the Resource:

https://github.com/dsccommunity/NetworkingDsc/blob/8df30e4d16b058fd02fd8305f99e0d14dba7ec81/source/DSCResources/DSC_NetAdapterBinding/DSC_NetAdapterBinding.schema.mof#L6-L7

This runs counter to the idiomatic design of a DSC Resource which presumes that any property of a Resource which is stateful and ensurable have a single writable property for it and that this property be returned in Get-TargetResource.

The current implementation of Get-TargetResource returns both State and CurrentState, though those values may be different because State returns the value of the parameter passed to that function (defaults to Enabled) while CurrentState returns the actual state of the adapter binding.

https://github.com/dsccommunity/NetworkingDsc/blob/8df30e4d16b058fd02fd8305f99e0d14dba7ec81/source/DSCResources/DSC_NetAdapterBinding/DSC_NetAdapterBinding.psm1#L56-L77

Test-TargetResource does not utilize the Get-TargetResource function but instead reimplements the check and returns a boolean result.

Suggested solution to the issue

I suggest:

1. Removing the CurrentState property entirely from the schema
2. Removing the State parameter from Get-TargetResource
3. Reworking Get-TargetResource to return the current state of the adapter binding for State

Version of the DSC module that was used ('dev' if using current dev branch)

• Latest code on master

[PlagueHO]

Agree that this is non-idiomatic.

I agree with your solutions (also notice we need to correct the dev to master - although I'll be renaming this to main once more of the PRs are through).

The only problem I think we'll run into is that we will need to drop the 'Mixed' value because it is not available MOF for State. However, that may not be a problem because technically we shouldn't pass the State parameter to Get-TargetResource at all.

But, what would happen if the State returns a value that is not actually supported by the Set/Test? Will that be a problem? It feels like it might be a potential issue for some use cases - but can't confirm.

Just adding one further comment: The reason for the Mixed state is so that wild card InterfaceAlias could be specified: https://github.com/dsccommunity/NetworkingDsc/pull/155

[michaeltlombardi]

This is a really good point and worth considering for the next iteration of DSC explicitly: are there values which can exist for an enum but not be specified?

I don't know what would happen from a Puppet integration perspective either, actually. The use case in #155 makes some sense but I wonder if it's not convenience at the cost of granular accuracy (especially if it prevents fixing this).

[PlagueHO]

Probably something that the DSC resource spec should clarify: Should we allow Get-TargetResource to return values in parameters that aren't specified in the MOF? @johlju, @gaelcolas - do you have any thoughts on this one?

I see two possibilities:

1. Deprecate wildcard support (not totally against this as it does feel like a convenience and we don't allow this universally) - this would of course be a breaking change.
2. Apply your solution above and accept/document that Get-TargetResource state may return a value not specified in the MOF.

Any other solutions?

[johlju]

My opinion is that if there is ValidateSet in a parameter (ValueMap in the schema) then Get should not return another value. If there could be another value then in the future maybe such properties should be able to return an Unknown value. But for now I think it should not be able to return a value not in a ValueMap.

I agree with the solution in the issue description.

To remove wildcard support sound like it will simplify the resource as well. But I don't mind having wildcard support if it serves a purpose. Although it can lead to ping-pong behavior when a resource could be added twice with different wildcards in the same configuration.

[PlagueHO]

A good point about the ping-pong behavior @johlju.

So, the question really is:

Do we deprecate the wildcard support? I'm not against this, I'm just thinking about whether or not this feature is widely used. We don't actually include an example showing it.

Perhaps we leave this open for discussion for a bit before making a decision?

[johlju]

I say remove it. Those that use can always (an should already have) pin an older version to use the functionality until they can migrate. But if it possible to deprecate it while fixing this issue then that works too.