Markus Roberts <[email protected]> writes:
>> > Fix is_virtual fact to return strings rather than bools.
>>
>> Wouldn't a better way to achieve this be this patch?
>>
>> diff --git a/lib/facter/util/fact.rb b/lib/facter/util/fact.rb index
>> e78ed97..e01833b 100644 --- a/lib/facter/util/fact.rb +++
>> b/lib/facter/util/fact.rb @@ -77,6 +77,9 @@ class Facter::Util::Fact
>> break tmp unless tmp.nil? or tmp == "" }
>>
>> + # Fact values must necessarily be strings, enforce that. +
>> �...@value = @value.to_s + unless foundsuits
>>
>> Facter.debug "Found no suitable resolves of %s for %s" %
>> [[email protected],
>> @name] end
>
> Yes but...
>
> 1) I'd still suggest my suggested simplification on the original code (but
> your approach makes it off topic).
> 2)
ENOCOMMENT? (I agree that your suggested code is much, much nicer.)
>> Otherwise the uninitiated might make the sad and painful discovery of this
>> themselves by returning false and having to learn that only '' is actually
>> "not true" as far as puppet is concerned.
>
> If I'm reading this correctly neither patch will work, as both result in
> "false" rather than "".
Uh-huh. I would rather that facter returned rich types, ideally through
JSON[1], since that gives us a solid and language-independent way to express a
rich set of basic data types without over-much complexity.
However, at least now there is more clarity, and adding a warning on
conversion would improve that - I would have known facter was unhappy with my
boolean false sooner.
(...and a real patch deserves that warning, I think, since this is a hard
requirement and being too liberal in what we accept here leads to problems
for external developers later, IMO)
> 3) Looking at the surrounding (gawd-awful) code I'm not convinced this is
> exactly the right spot for it...but that inject-with-next-unless-and-break
> pattern makes my head hurt. It looks as if this change would sometimes
> stop the "value for %s is still nil" branch from firing. Maybe it
> should be done right before the "return @value"?
I don't know the patch is entirely correct; it was an RFC more than a
serious "I actually bothered to check facter still works" submission.
So, yeah, that would need more attention than I gave it.
Daniel
Footnotes:
[1] ...with enforcement of the UTF-8 encoding requirement for the text. :)
--
✣ Daniel Pittman ✉ [email protected] ☎ +61 401 155 707
♽ made with 100 percent post-consumer electrons
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
