Fix String value issues in grub_config #46
Conversation
* Ensure that Boolean values are converted to Strings * Ensure the String values have quotes around them if not already present Fixes voxpupuli#44, voxpupuli#45
|
@raphink Up for a review? |
| @@ -66,6 +66,12 @@ def value | |||
| end | |||
|
|
|||
| def value=(newval) | |||
| if newval.is_a?(String) | |||
| unless %w[' "].include?(newval[0].chr) | |||
| newval = %Q("#{newval}") | |||
There was a problem hiding this comment.
I don't understand what you're doing here
There was a problem hiding this comment.
@raphink Ensuring that we don't quote strings that already start with quotes
There was a problem hiding this comment.
Oh I see, %Q("") adds literal quotes while interpreting #{newval}.
There was a problem hiding this comment.
iirc, there's a a quote_it function in augeasproviders_core, which is used in the shellvar module.
| end | ||
|
|
||
| def insync?(is) | ||
| if is.is_a?(String) && should.is_a?(String) |
There was a problem hiding this comment.
At some point, we should probably rewrite augeasproviders with the new Resource API to natively support data types.
There was a problem hiding this comment.
I'd be curious to see how that works out. From what I've seen, easy things are easy and advanced things are nigh impossible (might be missing something though).
There was a problem hiding this comment.
That was my feeling as well. @DavidS assured me otherwise though…
There was a problem hiding this comment.
looking over the grub_config type and provider, the following things can't be handled by the Resource API:
- the autorequire computation at https://github.com/hercules-team/augeasproviders_grub/blob/402ee21e8a590ed2c4e0c3981e35c7a03d268c61/lib/puppet/type/grub_config.rb#L63-L85 , which is more validation logic
- multiple providers for the same type (do they really have exactly the same attributes?)
- run
mkconfigafter all modifications: https://github.com/hercules-team/augeasproviders_grub/blob/402ee21e8a590ed2c4e0c3981e35c7a03d268c61/lib/puppet/provider/grub_config/grub2.rb#L82
There was a problem hiding this comment.
The stuff we do in https://github.com/hercules-team/augeasproviders_core/blob/master/lib/puppet/provider/augeasprovider/default.rb for ex, such as the handler sharing in the Puppet session.
instances such as https://github.com/hercules-team/augeasproviders_ssh/blob/master/lib/puppet/provider/sshd_config/augeas.rb#L113
probably other things, too. @trevor-vaughan do you have things in mind?
There was a problem hiding this comment.
The instances method seems to be fine - that's a regular get, collecting and computing all the resources.
I didn't fully read through the handler sharing, but you should be able to share any instances you need across providers by directly reaching into other ruby namespaces. No need to load something into the catalog for that.
|
Hello. I'm wondering when this will be merged to master and pushed to the forge? Thank you Tom |
present
Fixes #44, #45