PowerShell / DSC

This repo is for the DSC v3 project
MIT License
196 stars 26 forks source link

Rename `_ensure` to `_exist` #202

Closed SteveL-MSFT closed 11 months ago

SteveL-MSFT commented 11 months ago

Summary of the new feature / enhancement

_ensure is really a directive and not a property. This makes it inconsistent when a resource receives _ensure and also needs to return it. Resources should only return properties that pertain to the object. When a resource returns:

foo: bar
_ensure: present

Does it mean that the resource requested is present or is it just passing through the initial request?

To remove this ambiguity, I would propose since _ensure only has absent and present as valid values to rename to _absent which is true or false. In this case, if the request wants to ensure the instance is absent, then it would set _absent = true and the resource (if it supports _absent) always returns an _absent property setting it to true or false.

In the case that _absent is missing from the request or the object, it defaults to false.

Proposed technical implementation details (optional)

No response

ThomasNieto commented 11 months ago

I believe there were DSC resources that had non-standard values and maybe some in the Microsoft resource or examples.

As for the naming, would it be better to have present as the name with true or false instead of a double negative?

michaeltlombardi commented 11 months ago

_ensure is really a directive and not a property.

This depends on perspective - it's a property of the instance-as-a-whole, not a setting of the instance, if you follow me. Most configuration management tools that use this idiom use it as a configurable property. The expectation is that the resource implementation returns Absent and null-values (or in the case of JSON, none of those values are in the blob) when the instance doesn't exist.

This is often used by tools for reporting messages about whether the operation was to create, update, or remove an instance - Puppet and Chef both do this iirc.

In that regard, I think the current implementation of _ensure as a manageable property is correct and reuses a common idiom.

I'm not against replacing it with a boolean property instead, but I would recommend we call it something like exist to be even more explicit and move further away from the idiom.

Something like:

$id:         <HOST>/<PREFIX>/<VERSION>/resource/properties/exist.json
title:       Instance Existance
description: Indicates whether the DSC Resource instance should exist.
type:        boolean
default:     true
SteveL-MSFT commented 11 months ago

Based on internal discussion, we decided to change this well-known property to _exist as a boolean. Default value is true if not specified.

ThomasNieto commented 11 months ago

How does this impact existing resources using ensure?

michaeltlombardi commented 11 months ago

How does this impact existing resources using ensure? ~ @ThomasNieto

It doesn't affect existing PSDSC resources at all. They still work as implemented. They don't participate in any semantics DSC or higher order tools may build on _exist, but they aren't at a particular disadvantage.

After there's support in PSDSC for the DSCv3 semantics, maintainers can update their resources to participate in those semantics.

This change does require existing command-based DSC Resources (like those in the samples site) to update if they want to participate in these semantics.

ghost commented 7 months ago

To be or not to be is not the question, but whether you want to be or not to be? I think desired state is being confused with current state. For a Get method, the output is the static current state. I can consider it a directive to recreate my current configuration. It should look the same but is not the same. (Generating current state sounds useful. Return it as a Json?) "Present" or "Absent" is more obvious to me, but I could get used to _exists.

Ensure, Exists, or any other name for describing this does not fit with other properties. The properties that select the object are also special. If I think of a SQL Server table, a key and columns would be expected. I would not expect a IsDeleted column unless doing soft deletes so that deletes can be recovered or propagated. (It can be hard to detect a record that's not there when it was yesterday. Joining for missing records is expensive.) IsDeleted is a property of the record. A desired state of IsDeleted 0 or 1 works for me too, btw.

The only directive I see is that of calling Set-TargetResource. It's a simple directive - make it so. The property set, including Ensure, is not a directive.

I would have also preferred there be an option to use only one set of static properties for get, test, and set. Any "extra", non-key properties that cause errors when calling Get-TargetResource can be dynamically striped prior to the call. It might seem odd to have both extra properties and _exists $false, but it's less configuration.

After thought - it's not possible to drop the Ensure (by any name) completely. If only the properties required to find the object are present, it does not mean the object does not exist.

Test is another animal comparing current and desired states. IsDeleted will be special and cause me to pause and think. It will always be handled special because delete and insert are not updates.

ghost commented 7 months ago

With an older module like SqlServerDsc, will it use the Ensure value to populate _exists?