(GH-225) Add support for custom insync #285
Conversation
|
I haven't added the new tests yet but it looks like existing tests are failing on finding the servername fact. 🤔 |
michaeltlombardi
changed the title
spec/fixtures/test_module/lib/puppet/provider/test_custom_insync/test_custom_insync.rb
Outdated
Show resolved
Hide resolved
|
One thing I noticed in trying this out (puppetlabs/opv@a7f60f2) is that the type needs to have a property to trigger an insync-check. I expect that to be surprising, confusing, easily missed and dangerous to folks using this feature. At least this change should have a check that refuses to have a type with custom_insync feature and no property. I would prefer it if this condition is detected that the rsapi injects a hidden property just to force the check being called. This needs a bit more thought and care around how to name that to avoid conflicts and more confusion, but I think it is doable. |
@DavidS from a developer writing a resource, is this more or less unexpected than an error message informing the developer that use of custom insync requires an attribute without the namevar, read_only, or parameter behaviors? I'm not sure this is a big deal (since we can just explain it in the docs) but I'm equally unsure if this behavior would be more surprising than raising a From a naming perspective, I think |
|
@michaeltlombardi on the risk of bikeshedding, |
|
I've given this a go with a provider I wrote last month. Works great! (Well, it would if the rundeck API wasn't so hopeless!) |
|
@alexjfisher, @DavidS I added a new handler for cases where you want to use custom insync on a resource that doesn't have any insyncable properties; it creates a new hidden attribute, There's still some work and noodling left to do with regard to reporting and visibility, I'm not sure how hidden we can make the property - as far as I can tell, if |
|
@joshcooper I think this is in a state for code review at this time. |
|
While investigating some of Josh's comments, I found a bug: If a provider is I think that resource api should call |
|
What is this PR blocked on? I'd like to get it merged real soon (today or early next week) so we can get the DSC work unblocked. Is the API question something that we can deploy temporarily as an undocumented private API and iterate on with the DSC modules? |
This commit does a few things:
1. Add `custom_insync` to the list of valid type features
2. Ensure that if the `custom_insync` feature flag is specified all
properties **except** the `ensure` property get the custom `insync?`,
which calls into the provider and falls back to the
`Puppet::Property.insync?` logic if the result is nil (not handled
specifically in the provider's `insync?` method).
3. Passes (some) information about the type and current context to the
property so that the provider's `insync?` method can be called with
the necessary context.
4. Defines a hidden property, `rsapi_custom_insync_trigger`, which gets
added to any type which has the `custom_insync` feature flag but does
not include an insyncable attribute. This allows a resource with only
parameters to still call `insync?` and test whether the system is in
the desired state or not. This attribute is ommitted from RSAPI
logging but still shows up in Puppet change logs when the resource is
out of sync, as there is no way to report a change without also
logging it. In these cases, the log will read a `Custom insync logic
determined that this resource is out of sync`
This enables:
- A developer can now specify the `custom_insync` feature for a type and
an `insync?` method in their provider; that method can know the
attribute being checked, the `is` and `should` values of the actual
*resource*, and the current context. Valid returns from the custom
`insync?` method are:
- `true` and `false`, which indicate that the resource is in/out of
sync; if `false`, Puppet will log the default change report message.
- A `String`, which indicates that the resource is out of sync and
the value of which Puppet will log as the change report message.
- `nil`, which indicates that no custom insync handling was required,
which then falls back on `Puppet::Property.insync?` to compare state
and write any change reporting messages as needed.
- Any other data type, which will emit a warning and be treated as out
of sync, using Puppet's default change report message.
- A developer can now specify a type without any properties and with the
`custom_insync` feature which will trigger the `insync?` method once
for `rsapi_custom_insync_trigger`, enabling types which has only
parameters, init, read_only, and/or namevar attributes to validate
whether or not a resource is in or out of the desired state.
|
@binford2k I have just cleaned up the commits in the PR in case we can/want to merge today and otherwise to limit the noise if further refactoring is required. Per prior conversations with you, I'm fine with merging as is and "soft launching" with any future changes in implementation to be made and merged after the fact. |
|
Also worth noting: this is a blocker for Puppet Native Testing as well. |
|
I'm not sure what "soft launch" means? The one outstanding issue from my perspective is the the API for the |
This is the part we were discussing. If I understand right, the API change won't require changes in existing code. We were proposing treating the change as an undocumented private API consumed only by the DSC modules. As we iterate on it, the pwshlib gem is the only consumer and @michaeltlombardi has committed to doing the work to account for API changes. I hate doing something so hacky, but we have a publicly messaged timeline and need to get this shipped ASAP |
|
@DavidS, @joshcooper - latest commit refactors the I think this addresses the last concerns? |
|
Wanted to add some context that Puppet < 3 had nearly this same problem, the About 2eeff57, since the API is brand new, why not make symbol |
|
@joshcooper that makes sense, I can fix that commit either to warn (keeping it in line with the |
This commit modifies the def_custom_insync? handling in the property to
expect that calling insync? against the provider will return both the
provider_insync_result and a change_message, defaulting to nil for both
if nothing is returned.
If only one value is returned, it is treated as the result and the message
is nil.
If the result is nil or the change message is nil or empty, change_to_s
is not modified and reports the output of Puppet::Property.change_to_s as
the change log if needed.
If the result is not nil and the change message has a value, the change
message is used for the change log if needed.
The method also does some guardrail handling for result values:
- If the result is nil, fall back on Puppet::Property.insync? to check
whether the property is in the desired state or not.
- If the result is nil and the attribute being checked is ensure,
validate the comparison by comparing the values as strings.
- If the result is a real boolean, return that
- If the result is anything else, raise an explanatory Puppet::DevError.
|
@joshcooper, @DavidS, updated the implementation to raise a |
This PR defines a custom
insync?method for properties of a Resource API Type; this method can only be added to properties, not parameters, and is only called for properties which are not namevars.Currently, it does a few things:
custom_insyncto the list of valid type featurescustom_insyncfeature is specified all properties except theensureproperty get the custominsync?, which calls into the provider before supering back to the propertyinsync?method as defined by Puppet.ensureis already using a custominsync?, which this PR leaves alone.insync?method and context can be accessed from that method in the provider.What this currently enables:
custom_insyncfeature for a type and aninsync?method in their provider; that method can know the attribute being checked, theisandshouldvalues of the resource, & the current context. If nothing is specified in this method, the comparison defaults to what Puppet currently uses; the same is true for any properties not especially handled inside of this method. Any comparison that returnsnilwill be supered back to the existinginsync?method defined inPuppet::Property.