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

Rename _ensure to _exist #202

Closed
SteveL-MSFT opened this issue Sep 23, 2023 · 7 comments · Fixed by #206
Closed

Rename _ensure to _exist #202

SteveL-MSFT opened this issue Sep 23, 2023 · 7 comments · Fixed by #206
Assignees
Labels
Issue-Enhancement The issue is a feature or idea Need-Review

Comments

@SteveL-MSFT
Copy link
Member

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

@SteveL-MSFT SteveL-MSFT added Issue-Enhancement The issue is a feature or idea Need-Review labels Sep 23, 2023
@ThomasNieto
Copy link

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

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

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

@SteveL-MSFT SteveL-MSFT self-assigned this Sep 26, 2023
@SteveL-MSFT SteveL-MSFT changed the title Rename _ensure to _absent Rename _ensure to _exist Sep 26, 2023
@ThomasNieto
Copy link

How does this impact existing resources using ensure?

@michaeltlombardi
Copy link
Collaborator

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

ghost commented Feb 7, 2024

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

ghost commented Feb 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Enhancement The issue is a feature or idea Need-Review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants