-
Notifications
You must be signed in to change notification settings - Fork 498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(FACT-1277) Ensure Puppet external facts are loaded #1227
Conversation
The external facts path needs to be registered before doing anything to resolve facts - such as call `Facter.add`. Otherwise that path will not be used to resolve facts. Register `pluginfactdest` before adding core Puppet facts.
👍 |
makes sense, just waiting for checks. |
Passed. |
@@ -18,14 +18,14 @@ static const char load_puppet[] = | |||
" $LOAD_PATH << Puppet[:libdir]\n" | |||
"end\n" | |||
"Facter.reset\n" | |||
"Facter.search_external([Puppet[:pluginfactdest]])\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Puppet's facter
terminus calls both Facter.search
and Facter.search_external
. The former includes custom facts from all modules in the environment. The latter includes external facts from all modules in the environment, and the external facts in pluginfactdest
.
This commit is only adding external fact search path for pluginfactdest
. Do we care about omitting custom and external facts from modules? For example facter -p
will return different facts than puppet apply
. I think we made explicit decision to skip those, but I don't remember the details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we didn't do it this way to keep it consistent with what Facter 2.x was doing to support -p
, although given that we originally didn't plan on supporting -p
that should be revisited, but IMO that's for a later release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We find pluginsynced custom facts, but not custom facts for modules in codedir. This was a decision carried forward from 2.x. This commit doesn't change that, it just rearranges code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, puppet 3.x includes facts from 5 sources:
- non-pluginsynced external facts (
/etc/facter/facts.d
) - module external (
/etc/puppet/modules/foo/facts.d/
) and custom (/etc/puppet/modules/foo/lib/facter
) facts - pluginsynced external (
/var/lib/puppet/facts.d/
) and custom (/var/lib/puppet/lib/facter
) facts
facter 2.x only reports on
- non-pluginsynced external facts (
/etc/facter/facts.d
) - custom (
/etc/puppet/modules/foo/lib/facter
) facts
$ sudo bundle exec puppet facts find . | jq .values | grep myfact | sort
"myfact_external": "true",
"myfact_module_custom": "true",
"myfact_module_external": "true",
"myfact_pluginsync_custom": "true",
"myfact_pluginsync_external": "true",
$ sudo bundle exec facter -p | grep myfact | sort
myfact_external => true
myfact_pluginsync_custom => true
Also both puppet 3.x and facter 2.x include puppetversion
:
$ sudo bundle exec facter -p | grep puppetversion
puppetversion => 3.8.4
$ sudo bundle exec puppet facts find . | jq .values | grep puppetversion
"puppetversion": "3.8.4",
We fixed facter (in 3.x series) to include pluginsynced external facts, so both puppet and facter return the same set of fact excluding custom and external facts from modules:
# /opt/puppetlabs/bin/puppet facts find | grep myfact
"myfact_external": "true",
"myfact_module_custom": true,
"myfact_module_external": "true",
"myfact_pluginsync_custom": true,
"myfact_pluginsync_external": "true",
# /opt/puppetlabs/bin/facter -p | grep myfact
myfact_external => true
myfact_pluginsync_custom => true
myfact_pluginsync_external => true
And without this PR, facter -p
omits puppetversion:
# /opt/puppetlabs/bin/facter -p | grep puppetversion
# /opt/puppetlabs/bin/puppet facts find | grep puppetversion
"puppetversion": "4.3.0",
So 👍
(FACT-1277) Ensure Puppet external facts are loaded
The external facts path needs to be registered before doing anything to
resolve facts - such as call
Facter.add
. Otherwise that path will notbe used to resolve facts. Register
pluginfactdest
before adding corePuppet facts.