Skip to content

(#12357) Make facter_dot_d look in Puppet[:confdir]/facts.d #44

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions README_DEVELOPER.markdown
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
Puppet Specific Facts
=====================

Facter is meant to stand alone and apart from Puppet. However, Facter often
runs inside Puppet and all custom facts included in the stdlib module will
almost always be evaluated in the context of Puppet and Facter working
together.

Still, we don't want to write custom facts that blow up in the users face if
Puppet is not loaded in memory. This is often the case if the user run
`facter` without also supplying the `--puppet` flag.

Ah! But Jeff, the custom fact won't be in the `$LOAD_PATH` unless the user
supplies `--facter`! You might say...

Not (always) true I say! If the user happens to have a CWD of
`<modulepath>/stdlib/lib` then the facts will automatically be evaluated and
blow up.

In any event, it's pretty easy to write a fact that has no value if Puppet is
not loaded. Simply do it like this:

Facter.add(:node_vardir) do
setcode do
# This will be nil if Puppet is not available.
Facter::Util::PuppetSettings.with_puppet do
Puppet[:vardir]
end
end
end

The `Facter::Util::PuppetSettings.with_puppet` method accepts a block and
yields to it only if the Puppet library is loaded. If the Puppet library is
not loaded, then the method silently returns `nil` which Facter interprets as
an undefined fact value. The net effect is that the fact won't be set.
10 changes: 10 additions & 0 deletions lib/facter/facter_dot_d.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
# The cache is stored in /tmp/facts_cache.yaml as a mode
# 600 file and will have the end result of not calling your
# fact scripts more often than is needed

require 'facter/util/puppet_settings'

class Facter::Util::DotD
require 'yaml'

Expand Down Expand Up @@ -182,3 +185,10 @@ def create

Facter::Util::DotD.new("/etc/facter/facts.d").create
Facter::Util::DotD.new("/etc/puppetlabs/facter/facts.d").create

# Windows has a different configuration directory that defaults to a vendor
# specific sub directory of the %COMMON_APPDATA% directory.
if Dir.const_defined? 'COMMON_APPDATA' then
windows_facts_dot_d = File.join(Dir::COMMON_APPDATA, 'PuppetLabs', 'facter', 'facts.d')
Facter::Util::DotD.new(windows_facts_dot_d).create
end
16 changes: 16 additions & 0 deletions lib/facter/puppet_vardir.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# This facter fact returns the value of the Puppet vardir setting for the node
# running puppet or puppet agent. The intent is to enable Puppet modules to
# automatically have insight into a place where they can place variable data,
# regardless of the node's platform.
#
# The value should be directly usable in a File resource path attribute.
require 'facter/util/puppet_settings'

Facter.add(:puppet_vardir) do
setcode do
# This will be nil if Puppet is not available.
Facter::Util::PuppetSettings.with_puppet do
Puppet[:vardir]
end
end
end
17 changes: 17 additions & 0 deletions lib/facter/util/puppet_settings.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
module Facter
module Util
module PuppetSettings
class << self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def self.with_puppet is generally preferable - because you can clearly identify each function individually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Thu, Mar 8, 2012 at 10:13 AM, Daniel Pittman <
[email protected]

wrote:

@@ -0,0 +1,17 @@
+module Facter

  • module Util
  • module PuppetSettings
  •  class << self
    

def self.with_puppet is generally preferable - because you can clearly
identify each function individually.

Got it, I'll keep this in mind.

For what it's worth, I was livid when I had to go learn about the
eigenclass when I was first getting started with Puppet a few years ago.
I'm surprised at myself for repeating this mistake of using class << self
with the intent of defining a class method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Thu, Mar 8, 2012 at 10:13 AM, Daniel Pittman
[email protected]
wrote:

@@ -0,0 +1,17 @@
+module Facter

  •  module Util
  •    module PuppetSettings
  •      class << self

def self.with_puppet is generally preferable - because you can clearly identify each function individually.

Should be addressed in #50

-Jeff

def with_puppet
begin
Module.const_get("Puppet")
rescue NameError
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a whole lot of magic, right here. It should have some comments to explain what it is all about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a case where it didn't seem like magic at all when I wrote
this. It's the third or fourth time I've done something like this so it
feels obvious to me now. If I put myself in the shoes I was wearing the
first time I drew this pattern it'd have been obvious that I needed to
comment this more clearly.

Will keep this in mind and add comments if I'm in this code again soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Thu, Mar 8, 2012 at 10:12 AM, Daniel Pittman
[email protected]
wrote:

@@ -0,0 +1,17 @@
+module Facter

  •  module Util
  •    module PuppetSettings
  •      class << self
  •        def with_puppet
  •          begin
  •            Module.const_get("Puppet")
  •          rescue NameError

This is a whole lot of magic, right here.  It should have some comments to explain what it is all about.

I found myself back in the code today and this comment should be
addressed in #50

-Jeff

nil
else
yield
end
end
end
end
end
end
35 changes: 35 additions & 0 deletions spec/unit/facter/util/puppet_settings_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
require 'spec_helper'
require 'facter/util/puppet_settings'

describe Facter::Util::PuppetSettings do

describe "#with_puppet" do
context "Without Puppet loaded" do
before(:each) do
Module.expects(:const_get).with("Puppet").raises(NameError)
end

it 'should be nil' do
subject.with_puppet { Puppet[:vardir] }.should be_nil
end
it 'should not yield to the block' do
Puppet.expects(:[]).never
subject.with_puppet { Puppet[:vardir] }.should be_nil
end
end
context "With Puppet loaded" do
module Puppet; end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to have lasting effects on the state of the running process. If order was randomized, and these tests run before the "without" tests, you are going to have failures. That sort of order dependency is not good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to have lasting effects on the state of the running process.

True.

If order was randomized, and these tests run before the "without" tests,
you are going to have failures. That sort of order dependency is not good.

Not true.

The way I'm mocking prevents these ordering issues because I'm explicitly
intercepting the call to Module.const_defined? in the without tests.
Therefore, it doesn't matter if the other test defined Puppet or didn't.

With this information, do you think it's still an issue that I'm doing
module Puppet; end in one of the test groups?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, that stubbing does fix things, and I missed it when I looked at the code. I am quite satisfied with this.

let(:vardir) { "/var/lib/puppet" }

before :each do
Puppet.expects(:[]).with(:vardir).returns vardir
end
it 'should yield to the block' do
subject.with_puppet { Puppet[:vardir] }
end
it 'should return the nodes vardir' do
subject.with_puppet { Puppet[:vardir] }.should eq vardir
end
end
end
end