Fix problem introduced in #346 and simplification of create #349
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It was a plain parameter (not property) with no effect. Now it's an property that actually backs the subscription json key. Its default was also strange: the symbol :name. Now it's the value of the name property (I suppose this was the original intention).
Using Puppet::Property::Boolean does not work correctly. It munges the
value to (boolean) true and false. This causes an unfortunate problem.
Once a property is true, it's not possible to change it back to false.
This is because on ResourceHarness::sync_if_needed() the property value
is not even checked to be in sync of if the should value is false:
if param.should && !param.safe_insync?(current_value)
This should probably read `if !param.should.nil?`. But since it doesn't,
this commit introduces a new superclass for the boolean properties that
normalizes to :true/:false instead. To avoid extra complexity in the
provider, wich has to work with actual booleans, the superclass also
overrides `set` and `retrieve` to shield the provider from having to do
converisons :true/:false => true/false and back.
This creates an additional problem, though. The should value retrieved
in the provider through `resource[:boolean_property]` will still be
:true/:false. Because the `create` methods in providers are littered
with statements such as
self.<property name> = resource[:<property name>]
the setters in the providers will still be called with :true/:false.
This will be addressed in the next commit.
The new `create` implemented in a module will call `Property::set(value)` rather than the provider setters directly. This is to allow `Property::set` to do extra magic before delegating back to the provider, as is done in `PuppetX::Sensu::BooleanProperty`.
|
Wow. Nice fix. I'll try to review on the weekend - @jlambert121 ping too. |
jamtur01
added a commit
that referenced
this pull request
May 10, 2015
Fix problem introduced in #346 and simplification of create
|
Awesome thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Look at the individual commit messages for more information.