-
Notifications
You must be signed in to change notification settings - Fork 96
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
NetAdapterBinding: Use of State & CurrentState Properties is non-idiomatic #486
Comments
Agree that this is non-idiomatic. I agree with your solutions (also notice we need to correct the 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 But, what would happen if the Just adding one further comment: The reason for the Mixed state is so that wild card |
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). |
Probably something that the DSC resource spec should clarify: Should we allow I see two possibilities:
Any other solutions? |
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 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. |
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? |
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. |
Details of the scenario you tried and the problem that is occurring
The schema for the
NetAdapterBinding
Resource includes bothState
(writable enum) andCurrentState
(read-only enum) for the same information on the Resource:NetworkingDsc/source/DSCResources/DSC_NetAdapterBinding/DSC_NetAdapterBinding.schema.mof
Lines 6 to 7 in 8df30e4
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 bothState
andCurrentState
, though those values may be different becauseState
returns the value of the parameter passed to that function (defaults toEnabled
) whileCurrentState
returns the actual state of the adapter binding.NetworkingDsc/source/DSCResources/DSC_NetAdapterBinding/DSC_NetAdapterBinding.psm1
Lines 56 to 77 in 8df30e4
Test-TargetResource
does not utilize theGet-TargetResource
function but instead reimplements the check and returns a boolean result.Suggested solution to the issue
I suggest:
CurrentState
property entirely from the schemaState
parameter fromGet-TargetResource
Get-TargetResource
to return the current state of the adapter binding forState
Version of the DSC module that was used ('dev' if using current dev branch)
master
The text was updated successfully, but these errors were encountered: