(#4561) Support structured facts #1578

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

dalen commented Apr 3, 2013

Add the option stringify_facts which defaults to true. If it is turned
on the current behaviour with turning facts into strings is kept. If it
is false facts are kept in the format they were in which can also be
booleans, numerics, arrays and hashes.

CLA Signed by dalen on 2012-07-17 21:00:00 -0700

lib/puppet/defaults.rb
+ :stringify_facts => {
+ :default => true,
+ :type => :boolean,
+ :desc => "Transform fact values to strings. Means you can't have arrays or hashes as fact values.",
@adrienthebo

adrienthebo Apr 3, 2013

Contributor

This should indicate that the values will be flattened with #to_s

Contributor

adrienthebo commented Apr 3, 2013

To copy the conversation we had in IRC, I would still like to see some sort of input validation if we're not stringifying values. Stringifying facts served to smash data into a regular and fairly safe format, and when we remove that I worry that we're exposing ourselves to potential exploits.

Contributor

adrienthebo commented Apr 3, 2013

We're also getting spec failures; it just looks like a test needs to be updated since we're logging deprecation warnings.

  1) Puppet::Application::Facts should return facts if a key is given to find
     Failure/Error: @logs.should be_empty
       expected empty? to return true, got false
     # ./spec/unit/application/facts_spec.rb:26:in `block (2 levels) in '
Contributor

dalen commented Apr 4, 2013

I've updated this now and fixed all the issues identified so far.

Contributor

adrienthebo commented Apr 8, 2013

My initial impression is that this should probably work as-is, and addresses my concerns. However, structured facts is the topic of ARM 5 and I don't think that we have the final behavior nailed down. I'll need to talk to @ahpook and @zaphod42 about this and see where they want to take structured facts. I'll update this when I have more information.

Contributor

dalen commented Apr 8, 2013

yes, to me it seems ARM-5 mostly discusses various solutions to issue #11915, rather than the structured facts issue.

I do think there is more work to be done in facter regarding this though, like better output of structured facts in the standard output (they already work fine with --json or --yaml). And also it would be good if one fact could extend another fact.

Then of course PuppetDB will have to support them as well.

But that is all stuff in facter & puppetdb, not in puppet.

Contributor

jeffmccune commented Apr 10, 2013

@adrienthebo @dalen Where are we at with this pull request? I reviewed this today and the next actions aren't super clear to me. @dalen could you let me know if this change set is ready for review and merge, or if there's additional discussion that needs to happen with @ahpook and @zaphod42 ?

-Jeff

(#4561) Support structured facts
Add the option stringify_facts which defaults to true. If it is turned
on the current behaviour with turning facts into strings is kept.
If it is false facts are sanitized and only converted to strings if they
aren't booleans, numerics, arrays and hashes. It does this sanitizing in
a nested manner for arrays and hashes.
Contributor

dalen commented Apr 11, 2013

Updated the spec for sanitize splitting it into several tests as requested by @zaphod42.

Contributor

jeffmccune commented Apr 12, 2013

@dalen Thanks for pushing the amended commit, I'm planning to review this and figure out what the next steps are by the COB PT today.

Contributor

jeffmccune commented Apr 12, 2013

@dalen Actually, I spoke too soon, I don't think I'm going to have this updated by the end of today, sorry. I'll follow up as soon as possible, hopefully Monday.

Contributor

jeffmccune commented Apr 23, 2013

@dalen This looks good, and is backwards compatible, however, it does deprecate stringified facts. @ahpook do you think it's OK to deprecate the default fact stringification behavior of Puppet in the next minor or major release of Puppet?

Contributor

ahpook commented Apr 23, 2013

💯

Contributor

adrienthebo commented Apr 24, 2013

👍 for merging this, @jeffmccune wanna pull the trigger on this tomorrow then?

Contributor

jeffmccune commented Apr 24, 2013

Yeah, lets merge this tomorrow morning assuming everything stays green over
night.

Contributor

ahpook commented Apr 24, 2013

Excellent. Need to bring arm-5 to conclusion to do the Facter end of this, and tie up the deprecation / migration path.

Contributor

dalen commented Apr 24, 2013

Facter already supports structured facts, so with this patch it already works (sans puppetdb).

But I think there are other related things in facter that could be good. For example merging of hash facts so one fact can extend another fact.

Contributor

jeffmccune commented Apr 25, 2013

summary: Merged into master as ca0da45. This should be released in Puppet 3.3.0. Thanks again for the contribution! As Erik mentioned, Facter already supports structured facts, so with this patch it already works (sans puppetdb). But I think there are other related things in facter that could be good. For example merging of hash facts so one fact can extend another fact.

-Jeff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment