From 111a4b546dd1bcaab182d5c8ad694404c2c2f91c Mon Sep 17 00:00:00 2001
From: Ben Hughes <ben@puppetlabs.com>
Date: Fri, 1 Apr 2011 15:23:14 +1100
Subject: (#6857) Password disclosure when changing a user's password

Make the should_to_s and is_to_s functions to return a form of 'redacted'.

Rather than send the password hash to system logs in cases of failure or
running in --noop mode, just state whether it's the new or old hash. We're
already doing this with password changes that work, so this just brings it
inline with those, albeit via a slightly different pair of methods.
---
 ...rd-disclosure-when-changing-a-users-password.rb | 23 ++++++++++++++++++++++
 lib/puppet/type/user.rb                            |  8 ++++++++
 spec/unit/type/user_spec.rb                        |  8 ++++++++
 3 files changed, 39 insertions(+)
 create mode 100644 acceptance/tests/ticket_6857_password-disclosure-when-changing-a-users-password.rb

diff --git a/acceptance/tests/ticket_6857_password-disclosure-when-changing-a-users-password.rb b/acceptance/tests/ticket_6857_password-disclosure-when-changing-a-users-password.rb
new file mode 100644
index 000000000..f1e100c2e
--- /dev/null
+++ b/acceptance/tests/ticket_6857_password-disclosure-when-changing-a-users-password.rb
@@ -0,0 +1,23 @@
+test_name "#6857: redact password hashes when applying in noop mode"
+
+adduser_manifest = <<MANIFEST
+user { 'passwordtestuser':
+  ensure   => 'present',
+  password => 'apassword',
+}
+MANIFEST
+
+changepass_manifest = <<MANIFEST
+user { 'passwordtestuser':
+  ensure   => 'present',
+  password => 'newpassword',
+  noop     => true,
+}
+MANIFEST
+
+apply_manifest_on(agents, adduser_manifest )
+results = apply_manifest_on(agents, changepass_manifest )
+
+results.each do |result|
+  assert_match( /current_value \[old password hash redacted\], should be \[new password hash redacted\]/ , "#{result.stdout}" )
+end
diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb
index f74e4266f..8d04fdc30 100755
--- a/lib/puppet/type/user.rb
+++ b/lib/puppet/type/user.rb
@@ -165,6 +165,14 @@ module Puppet
           return "changed password"
         end
       end
+
+      def is_to_s( currentvalue )
+        return '[old password hash redacted]'
+      end
+      def should_to_s( newvalue )
+        return '[new password hash redacted]'
+      end
+
     end
 
     newproperty(:password_min_age, :required_features => :manages_password_age) do
diff --git a/spec/unit/type/user_spec.rb b/spec/unit/type/user_spec.rb
index 5a84af443..594802c6e 100755
--- a/spec/unit/type/user_spec.rb
+++ b/spec/unit/type/user_spec.rb
@@ -290,6 +290,14 @@ describe user do
       @password.change_to_s("other", "mypass").should_not be_include("mypass")
     end
 
+    it "should redact the password when displaying the old value" do
+      @password.is_to_s("currentpassword").should =~ /^\[old password hash redacted\]$/
+    end
+
+    it "should redact the password when displaying the new value" do
+      @password.should_to_s("newpassword").should =~ /^\[new password hash redacted\]$/
+    end
+
     it "should fail if a ':' is included in the password" do
       lambda { @password.should = "some:thing" }.should raise_error(Puppet::Error)
     end
-- 
cgit 


From f6882d6d5779883e931a6f558c06f631098011c5 Mon Sep 17 00:00:00 2001
From: nfagerlund <nick.fagerlund@gmail.com>
Date: Wed, 29 Jun 2011 16:50:46 -0700
Subject: (#8147) Change default reporturl to match newer Dashboard versions

Puppet's default reporturl setting was http://localhost:3000/reports, which has
been deprecated in Puppet Dashboard in favor of
http://localhost:3000/reports/upload. As Dashboard is the first-class
destination for the http report processor, this commit changes Puppet's default
to match what current versions of Dashboard expect.

Reviewed-By: Jacob Helwig <jacob@puppetlabs.com>
---
 lib/puppet/defaults.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
index 07442d0e9..2247634b3 100644
--- a/lib/puppet/defaults.rb
+++ b/lib/puppet/defaults.rb
@@ -466,7 +466,7 @@ module Puppet
       :desc => "The directory in which to store reports
         received from the client.  Each client gets a separate
         subdirectory."},
-    :reporturl => ["http://localhost:3000/reports",
+    :reporturl => ["http://localhost:3000/reports/upload",
       "The URL used by the http reports processor to send reports"],
     :fileserverconfig => ["$confdir/fileserver.conf", "Where the fileserver configuration is stored."],
     :strict_hostname_checking => [false, "Whether to only search for the complete
-- 
cgit 


From 98cd89bdd44a68439ee4d030acc91fd0dda42707 Mon Sep 17 00:00:00 2001
From: nfagerlund <nick.fagerlund@gmail.com>
Date: Wed, 29 Jun 2011 20:01:20 -0700
Subject: (#8147) Update test for default reporturl

Edit test to accomodate the default URL change made in commit
f6882d6d5779883e931a6f558c06f631098011c5.

Reviewed-By: Jacob Helwig <jacob@puppetlabs.com>
---
 spec/integration/defaults_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/integration/defaults_spec.rb b/spec/integration/defaults_spec.rb
index 9bec769ab..8cf0e3e7b 100755
--- a/spec/integration/defaults_spec.rb
+++ b/spec/integration/defaults_spec.rb
@@ -275,6 +275,6 @@ describe "Puppet defaults" do
 
   describe "reporturl" do
     subject { Puppet.settings[:reporturl] }
-    it { should == "http://localhost:3000/reports" }
+    it { should == "http://localhost:3000/reports/upload" }
   end
 end
-- 
cgit 


From 5826f7399325c784a5e8e33c7fabc46e202334a8 Mon Sep 17 00:00:00 2001
From: Jeff McCune <jeff@puppetlabs.com>
Date: Tue, 21 Jun 2011 18:54:35 -0700
Subject: (#8032) Add containment to create_resources

Without this change native resource types declared using the
create_resources() function are not contained within the class scope of
the function call.  As a result, resources were "floating off" in the
graph, disconnected from the rest of the relationship edges.

With this change, the scope is preserved and native resources are
contained by the class the function call is executed from.

Reviewed-by: Dan Bode <dan@puppetlabs.com>
---
 lib/puppet/parser/functions/create_resources.rb | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/puppet/parser/functions/create_resources.rb b/lib/puppet/parser/functions/create_resources.rb
index 430f110b4..3b8bb3543 100644
--- a/lib/puppet/parser/functions/create_resources.rb
+++ b/lib/puppet/parser/functions/create_resources.rb
@@ -27,15 +27,16 @@ Takes two parameters:
   args[1].each do |title, params|
     raise ArgumentError, 'params should not contain title' if(params['title'])
     case type_of_resource
-    when :type
-      res = resource.hash2resource(params.merge(:title => title))
-      catalog.add_resource(res)
-    when :define
+    # JJM The only difference between a type and a define is the call to instantiate_resource
+    # for a defined type.
+    when :type, :define
       p_resource = Puppet::Parser::Resource.new(type_name, title, :scope => self, :source => resource)
       params.merge(:name => title).each do |k,v|
         p_resource.set_parameter(k,v)
       end
-      resource.instantiate_resource(self, p_resource)
+      if type_of_resource == :define then
+        resource.instantiate_resource(self, p_resource)
+      end
       compiler.add_resource(self, p_resource)
     when :class
       klass = find_hostclass(title)
-- 
cgit 


From ae3ef423c03b7ef27f975dadfb67bf77ca481503 Mon Sep 17 00:00:00 2001
From: Dan Bode <dan@puppetlabs.com>
Date: Wed, 6 Jul 2011 21:59:05 -0700
Subject: (#7699) - Help should only show options once

puppet help was reprinting every option once
for every alias that is had.

This fix involves only storing the option.name
in the @options instance var for both face and
actions options. The @options_hash still
maintains the list of options and aliases as its
keys.

Reviewed-by: Daniel Pittman (puppet-dev list)
---
 lib/puppet/interface/action.rb         | 3 ++-
 lib/puppet/interface/option_manager.rb | 3 ++-
 spec/unit/interface/action_spec.rb     | 9 +++++++++
 spec/unit/interface_spec.rb            | 8 ++++++++
 4 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index 185302b07..fe77a9658 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -227,8 +227,9 @@ WRAPPER
       end
     end
 
+    @options << option.name
+
     option.aliases.each do |name|
-      @options << name
       @options_hash[name] = option
     end
 
diff --git a/lib/puppet/interface/option_manager.rb b/lib/puppet/interface/option_manager.rb
index 326a91d92..a1f300e8e 100644
--- a/lib/puppet/interface/option_manager.rb
+++ b/lib/puppet/interface/option_manager.rb
@@ -26,8 +26,9 @@ module Puppet::Interface::OptionManager
       end
     end
 
+    @options << option.name
+
     option.aliases.each do |name|
-      @options << name
       @options_hash[name] = option
     end
 
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index cf8d61d51..3e1bd0d4c 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -171,6 +171,15 @@ describe Puppet::Interface::Action do
       face.get_action(:foo).options.should =~ [:bar]
     end
 
+    it "should only list options and not aliases" do
+      face = Puppet::Interface.new(:action_level_options, '0.0.1') do
+        action :foo do
+          option "--bar", "-b", "--foo-bar"
+        end
+      end
+      face.get_action(:foo).options.should =~ [:bar]
+    end
+
     describe "with both face and action options" do
       let :face do
         Puppet::Interface.new(:action_level_options, '0.0.1') do
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index 8bbbcc857..4cb1f8743 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -174,6 +174,14 @@ describe Puppet::Interface do
       face.get_action(:foo).options.should =~ [:quux]
       face.get_action(:bar).options.should =~ [:quux]
     end
+
+    it "should only list options and not aliases" do
+      face = subject.new(:face_options, '0.0.1') do
+        option "--bar", "-b", "--foo-bar"
+      end
+      face.options.should =~ [:bar]
+    end
+
   end
 
   describe "with inherited options" do
-- 
cgit 


From 1feccc3f2db29d112308a55032e7f93ca44b45aa Mon Sep 17 00:00:00 2001
From: Jeff McCune <jeff@puppetlabs.com>
Date: Sun, 10 Jul 2011 13:51:31 -0700
Subject: Revert "Merge branch
 'ticket/2.7.x/7699_fix_help_listing_options_twice' into 2.7.x"

This reverts commit b7ee0258ab40478329c20177eda9b250f27ede18, reversing
changes made to 8fe2e555ac3d57f5b6503ffe1a5466db8d6e190a.
---
 lib/puppet/interface/action.rb         | 3 +--
 lib/puppet/interface/option_manager.rb | 3 +--
 spec/unit/interface/action_spec.rb     | 9 ---------
 spec/unit/interface_spec.rb            | 8 --------
 4 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index fe77a9658..185302b07 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -227,9 +227,8 @@ WRAPPER
       end
     end
 
-    @options << option.name
-
     option.aliases.each do |name|
+      @options << name
       @options_hash[name] = option
     end
 
diff --git a/lib/puppet/interface/option_manager.rb b/lib/puppet/interface/option_manager.rb
index a1f300e8e..326a91d92 100644
--- a/lib/puppet/interface/option_manager.rb
+++ b/lib/puppet/interface/option_manager.rb
@@ -26,9 +26,8 @@ module Puppet::Interface::OptionManager
       end
     end
 
-    @options << option.name
-
     option.aliases.each do |name|
+      @options << name
       @options_hash[name] = option
     end
 
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index 3e1bd0d4c..cf8d61d51 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -171,15 +171,6 @@ describe Puppet::Interface::Action do
       face.get_action(:foo).options.should =~ [:bar]
     end
 
-    it "should only list options and not aliases" do
-      face = Puppet::Interface.new(:action_level_options, '0.0.1') do
-        action :foo do
-          option "--bar", "-b", "--foo-bar"
-        end
-      end
-      face.get_action(:foo).options.should =~ [:bar]
-    end
-
     describe "with both face and action options" do
       let :face do
         Puppet::Interface.new(:action_level_options, '0.0.1') do
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index 4cb1f8743..8bbbcc857 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -174,14 +174,6 @@ describe Puppet::Interface do
       face.get_action(:foo).options.should =~ [:quux]
       face.get_action(:bar).options.should =~ [:quux]
     end
-
-    it "should only list options and not aliases" do
-      face = subject.new(:face_options, '0.0.1') do
-        option "--bar", "-b", "--foo-bar"
-      end
-      face.options.should =~ [:bar]
-    end
-
   end
 
   describe "with inherited options" do
-- 
cgit 


From 45b3908e03734388b6c699ffbc4223f43b44a1d5 Mon Sep 17 00:00:00 2001
From: Matt Robinson <matt@puppetlabs.com>
Date: Tue, 12 Jul 2011 15:47:21 -0700
Subject: (#4142) Fix module check not to fail when empty metadata.json

Even though the puppet module tool was fixed to generate the required
metadata attributes when it packages modules, it still creates an empty
metadata.json file that gets checked into everybody's module repos.
This causes the module to be unusable straight from a git clone since
puppet was requiring all the required metadata attributes just with the
presence of that file, and resulting in the error:

    No source module metadata provided for mcollective at

This change makes it so that if you have an empty metadata.json (like
the moduletool generates), puppet doesn't consider it to have metadata.
If you have ANY metadata attributes in that file, it will still check to
make sure all the required attributes are present.

The work around up to this point has just been to delete the
metadata.json file in git cloned modules.

This also fixed the tests around this to actually run, since previously
the tests depended on the a json feature, which we didn't have.  We do,
however, have a pson feature.

Reviewed-by: Nick Lewis <nick@puppetlabs.com>
---
 lib/puppet/module.rb     |  5 ++++-
 spec/unit/module_spec.rb | 47 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/lib/puppet/module.rb b/lib/puppet/module.rb
index 059591ed8..00468df96 100644
--- a/lib/puppet/module.rb
+++ b/lib/puppet/module.rb
@@ -42,7 +42,10 @@ class Puppet::Module
   def has_metadata?
     return false unless metadata_file
 
-    FileTest.exist?(metadata_file)
+    return false unless FileTest.exist?(metadata_file)
+
+    metadata = PSON.parse File.read(metadata_file)
+    return metadata.is_a?(Hash) && !metadata.keys.empty?
   end
 
   def initialize(name, environment = nil)
diff --git a/spec/unit/module_spec.rb b/spec/unit/module_spec.rb
index 8d38657f9..822c76296 100755
--- a/spec/unit/module_spec.rb
+++ b/spec/unit/module_spec.rb
@@ -505,12 +505,38 @@ describe Puppet::Module do
     mod.metadata_file.should == mod.metadata_file
   end
 
-  it "should know if it has a metadata file" do
+  it "should have metadata if it has a metadata file and its data is not empty" do
     FileTest.expects(:exist?).with(@module.metadata_file).returns true
+    File.stubs(:read).with(@module.metadata_file).returns "{\"foo\" : \"bar\"}"
 
     @module.should be_has_metadata
   end
 
+  it "should have metadata if it has a metadata file and its data is not empty" do
+    FileTest.expects(:exist?).with(@module.metadata_file).returns true
+    File.stubs(:read).with(@module.metadata_file).returns "{\"foo\" : \"bar\"}"
+
+    @module.should be_has_metadata
+  end
+
+  it "should not have metadata if has a metadata file and its data is empty" do
+    FileTest.expects(:exist?).with(@module.metadata_file).returns true
+    File.stubs(:read).with(@module.metadata_file).returns "/*
++-----------------------------------------------------------------------+
+|                                                                       |
+|                    ==> DO NOT EDIT THIS FILE! <==                     |
+|                                                                       |
+|   You should edit the `Modulefile` and run `puppet-module build`      |
+|   to generate the `metadata.json` file for your releases.             |
+|                                                                       |
++-----------------------------------------------------------------------+
+*/
+
+{}"
+
+    @module.should_not be_has_metadata
+  end
+
   it "should know if it is missing a metadata file" do
     FileTest.expects(:exist?).with(@module.metadata_file).returns false
 
@@ -528,16 +554,16 @@ describe Puppet::Module do
     Puppet::Module.new("yay")
   end
 
-  describe "when loading the medatada file", :if => Puppet.features.json? do
+  describe "when loading the medatada file", :if => Puppet.features.pson? do
     before do
       @data = {
-        :license => "GPL2",
-        :author => "luke",
-        :version => "1.0",
-        :source => "http://foo/",
+        :license       => "GPL2",
+        :author        => "luke",
+        :version       => "1.0",
+        :source        => "http://foo/",
         :puppetversion => "0.25"
       }
-      @text = @data.to_json
+      @text = @data.to_pson
 
       @module = Puppet::Module.new("foo")
       @module.stubs(:metadata_file).returns "/my/file"
@@ -552,9 +578,12 @@ describe Puppet::Module do
 
       it "should fail if #{attr} is not present in the metadata file" do
         @data.delete(attr.to_sym)
-        @text = @data.to_json
+        @text = @data.to_pson
         File.stubs(:read).with("/my/file").returns @text
-        lambda { @module.load_metadata }.should raise_error(Puppet::Module::MissingMetadata)
+        lambda { @module.load_metadata }.should raise_error(
+          Puppet::Module::MissingMetadata,
+          "No #{attr} module metadata provided for foo"
+        )
       end
     end
 
-- 
cgit 


From 4a2f22c00d1348aaa136f7269d2b5c65bcb54dc5 Mon Sep 17 00:00:00 2001
From: Dominic Maraglia <dominic@puppetlabs.com>
Date: Tue, 12 Jul 2011 17:34:22 -0700
Subject: (maint) Fix platform dection for RHEL

Detecting supported platform used incorrect string for RHEL.
Changed string from 'redhat' to 'rhel'
---
 .../resource/service/ticket_4123_should_list_all_running_redhat.rb     | 3 +--
 .../tests/resource/service/ticket_4124_should_list_all_disabled.rb     | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/acceptance/tests/resource/service/ticket_4123_should_list_all_running_redhat.rb b/acceptance/tests/resource/service/ticket_4123_should_list_all_running_redhat.rb
index 127e943a9..9f0fdc5c7 100755
--- a/acceptance/tests/resource/service/ticket_4123_should_list_all_running_redhat.rb
+++ b/acceptance/tests/resource/service/ticket_4123_should_list_all_running_redhat.rb
@@ -4,8 +4,7 @@ step "Validate services running agreement ralsh vs. OS service count"
 # ticket_4123_should_list_all_running_redhat.sh
 
 hosts.each do |host|
-  if host['platform'].include?('centos') or host['platform'].include?('redhat')
-    puts "XXX #{host['platform']}"
+  if host['platform'].include?('centos') or host['platform'].include?('rhel')
     run_script_on(host,'acceptance-tests/tests/resource/service/ticket_4123_should_list_all_running_redhat.sh')
   else
     skip_test "Test not supported on this plaform" 
diff --git a/acceptance/tests/resource/service/ticket_4124_should_list_all_disabled.rb b/acceptance/tests/resource/service/ticket_4124_should_list_all_disabled.rb
index db96ad91c..13ad5ceac 100755
--- a/acceptance/tests/resource/service/ticket_4124_should_list_all_disabled.rb
+++ b/acceptance/tests/resource/service/ticket_4124_should_list_all_disabled.rb
@@ -4,7 +4,7 @@ step "Validate disabled services agreement ralsh vs. OS service count"
 # ticket_4124_should_list_all_disabled.sh
 
 hosts.each do |host|
-  unless host['platform'].include? 'centos' or host['platform'].include? 'redhat'
+  unless host['platform'].include? 'centos' or host['platform'].include? 'rhel'
     skip_test "Test not supported on this plaform"
    else
     run_script_on(host,'acceptance-tests/tests/resource/service/ticket_4124_should_list_all_disabled.sh')
-- 
cgit 


From b82f29c61aa84a12fc208487e4b049cb24702261 Mon Sep 17 00:00:00 2001
From: Dan Bode <dan@puppetlabs.com>
Date: Wed, 13 Jul 2011 15:38:32 -0700
Subject: (#7699) Help command should only list options once

The problem was caused by the fact that the
options method returns a list of options that
treated the aliases as seperate options.

The fix is to only maintain a list of options
and not add all aliases to the options list.
---
 lib/puppet/interface/action.rb                        |  3 ++-
 lib/puppet/interface/option_manager.rb                |  3 ++-
 spec/shared_behaviours/things_that_declare_options.rb |  2 +-
 spec/unit/interface/action_spec.rb                    | 10 ++++++++++
 spec/unit/interface_spec.rb                           |  8 ++++++++
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index 185302b07..fe77a9658 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -227,8 +227,9 @@ WRAPPER
       end
     end
 
+    @options << option.name
+
     option.aliases.each do |name|
-      @options << name
       @options_hash[name] = option
     end
 
diff --git a/lib/puppet/interface/option_manager.rb b/lib/puppet/interface/option_manager.rb
index 326a91d92..a1f300e8e 100644
--- a/lib/puppet/interface/option_manager.rb
+++ b/lib/puppet/interface/option_manager.rb
@@ -26,8 +26,9 @@ module Puppet::Interface::OptionManager
       end
     end
 
+    @options << option.name
+
     option.aliases.each do |name|
-      @options << name
       @options_hash[name] = option
     end
 
diff --git a/spec/shared_behaviours/things_that_declare_options.rb b/spec/shared_behaviours/things_that_declare_options.rb
index ebf1b2071..19bba66c1 100755
--- a/spec/shared_behaviours/things_that_declare_options.rb
+++ b/spec/shared_behaviours/things_that_declare_options.rb
@@ -43,7 +43,7 @@ shared_examples_for "things that declare options" do
       option "-f"
       option "--baz"
     end
-    thing.options.should == [:foo, :bar, :b, :q, :quux, :f, :baz]
+    thing.options.should == [:foo, :bar, :quux, :f, :baz]
   end
 
   it "should detect conflicts in long options" do
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index cf8d61d51..23216e7b4 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -171,6 +171,16 @@ describe Puppet::Interface::Action do
       face.get_action(:foo).options.should =~ [:bar]
     end
 
+    it "should only list options and not aliases" do
+      face = Puppet::Interface.new(:action_level_options, '0.0.1') do
+        action :foo do
+          when_invoked do |options| true end
+          option "--bar", "-b", "--foo-bar"
+        end
+      end
+      face.get_action(:foo).options.should =~ [:bar]
+    end
+
     describe "with both face and action options" do
       let :face do
         Puppet::Interface.new(:action_level_options, '0.0.1') do
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index 8bbbcc857..4cb1f8743 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -174,6 +174,14 @@ describe Puppet::Interface do
       face.get_action(:foo).options.should =~ [:quux]
       face.get_action(:bar).options.should =~ [:quux]
     end
+
+    it "should only list options and not aliases" do
+      face = subject.new(:face_options, '0.0.1') do
+        option "--bar", "-b", "--foo-bar"
+      end
+      face.options.should =~ [:bar]
+    end
+
   end
 
   describe "with inherited options" do
-- 
cgit 


From b268fb3d4cca79bdce0adc7da8b4d47f20769521 Mon Sep 17 00:00:00 2001
From: Max Martin <max@puppetlabs.com>
Date: Wed, 6 Jul 2011 15:04:27 -0700
Subject: (#7144) Update Settings#writesub to convert mode to Fixnum

Settings#writesub was not checking the type of the mode value passed in
from the defaults, causing it to pass a string for mode to File.open,
leading to failures. This commit resolves that issue.

Paired-with: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/util.rb              |  1 -
 lib/puppet/util/settings.rb     |  2 +-
 spec/unit/util/settings_spec.rb | 11 +++++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index d06f44808..34c6ec1ed 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -29,7 +29,6 @@ module Util
     end
   end
 
-  
   def self.synchronize_on(x,type)
     sync_object,users = 0,1
     begin
diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index f243b8691..4559e9af3 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -721,7 +721,7 @@ if @config.include?(:run_mode)
     end
 
     Puppet::Util::SUIDManager.asuser(*chown) do
-      mode = obj.mode || 0640
+      mode = obj.mode ? obj.mode.to_i : 0640
       args << "w" if args.empty?
 
       args << mode
diff --git a/spec/unit/util/settings_spec.rb b/spec/unit/util/settings_spec.rb
index 07b712c08..888de156b 100755
--- a/spec/unit/util/settings_spec.rb
+++ b/spec/unit/util/settings_spec.rb
@@ -1,6 +1,7 @@
 #!/usr/bin/env ruby
 
 require File.dirname(__FILE__) + '/../../spec_helper'
+require 'ostruct'
 
 describe Puppet::Util::Settings do
   describe "when specifying defaults" do
@@ -1105,4 +1106,14 @@ describe Puppet::Util::Settings do
 
     it "should cache the result"
   end
+
+  describe "#writesub" do
+    it "should only pass valid arguments to File.open" do
+      settings = Puppet::Util::Settings.new
+      settings.stubs(:get_config_file_default).with(:privatekeydir).returns(OpenStruct.new(:mode => "750"))
+
+      File.expects(:open).with("/path/to/keydir", "w", 750).returns true
+      settings.writesub(:privatekeydir, "/path/to/keydir")
+    end
+  end
 end
-- 
cgit 


From c4848d25f51372b9c8fc0e5259b7d58fd23ebb44 Mon Sep 17 00:00:00 2001
From: Matt Robinson <matt@puppetlabs.com>
Date: Thu, 14 Jul 2011 14:13:31 -0700
Subject: maint: Fix documentation link for fileserver configuration

The link to the filesever configuration page linked to the wiki, which
links back to the docs site.  Short circuiting that do just link to
where you want to go.

Reviewed-by: Nick Lewis <nick@puppetlabs.com>
---
 lib/puppet/type/file/source.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index 76c646baf..49e885865 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -42,7 +42,7 @@ module Puppet
       on the local host, whereas `agent` will connect to the
       puppet server that it received the manifest from.
 
-      See the [fileserver configuration documentation](http://projects.puppetlabs.com/projects/puppet/wiki/File_Serving_Configuration) for information on how to configure
+      See the [fileserver configuration documentation](http://docs.puppetlabs.com/guides/file_serving.html) for information on how to configure
       and use file services within Puppet.
 
       If you specify multiple file sources for a file, then the first
-- 
cgit 


From a109c908f6d46d0b3a8bcd203b661d150eabb1ba Mon Sep 17 00:00:00 2001
From: Dominic Maraglia <dominic@puppetlabs.com>
Date: Thu, 14 Jul 2011 15:01:13 -0700
Subject: (maint) Cleanup and strengthen acceptance tests

Converted plain regex checks to use Test::Unit::Accertions.
Adding more verbose output in the case of failure to speeed
debugging.
---
 acceptance/tests/jeff_append_to_array.rb            |  9 +++++----
 ..._file_should_create_a_file_and_report_the_md5.rb |  9 +++++----
 acceptance/tests/puppet_apply_basics.rb             |  8 +++++---
 .../tests/puppet_apply_should_show_a_notice.rb      |  8 +++++---
 ...uppet_kick_with_hostnames_on_the_command_line.rb |  8 +++++---
 .../tests/ticket_4059_ralsh_can_change_settings.rb  |  7 ++++---
 ..._should_not_create_a_user_that_already_exists.rb |  6 ++++--
 .../tests/ticket_4233_resource_with_a_newline.rb    |  7 +++++--
 ...source_fail_when_name_defined_instead_of_path.rb |  6 ++++--
 ...ch_when_function_call_used_in_an_if_statement.rb |  7 ++++---
 ...r_should_recognize_OEL_operatingsystemrelease.rb |  2 +-
 ...t_4293_define_and_use_a_define_within_a_class.rb |  7 ++++---
 .../tests/ticket_5477_master_not_dectect_sitepp.rb  |  6 ++++--
 .../tests/ticket_6541_invalid_filebucket_files.rb   | 21 ++++++++++++---------
 acceptance/tests/ticket_6734_6256_5530_5503.rb      |  2 +-
 15 files changed, 68 insertions(+), 45 deletions(-)

diff --git a/acceptance/tests/jeff_append_to_array.rb b/acceptance/tests/jeff_append_to_array.rb
index 415d59fe8..20f43665e 100644
--- a/acceptance/tests/jeff_append_to_array.rb
+++ b/acceptance/tests/jeff_append_to_array.rb
@@ -12,8 +12,9 @@ manifest = %q{
     include parent::child
 }
 
-apply_manifest_on(agents, manifest) do
-  stdout =~ /notice: parent array element/ or fail_test("parent missing")
-  stdout =~ /notice: child array element/  or fail_test("child missing")
+agents.each do |host|
+  apply_manifest_on(host, manifest) do
+    assert_match(/notice: parent array element/, stdout, "#{host}: parent missing")
+    assert_match(/notice: child array element/, stdout, "#{host}: child missing")
+  end
 end
-
diff --git a/acceptance/tests/puppet_apply_a_file_should_create_a_file_and_report_the_md5.rb b/acceptance/tests/puppet_apply_a_file_should_create_a_file_and_report_the_md5.rb
index abb06fbaf..44338520a 100644
--- a/acceptance/tests/puppet_apply_a_file_should_create_a_file_and_report_the_md5.rb
+++ b/acceptance/tests/puppet_apply_a_file_should_create_a_file_and_report_the_md5.rb
@@ -6,10 +6,11 @@ manifest = "file{'#{file}': content => 'test'}"
 step "clean up #{file} for testing"
 on agents, "rm -f #{file}"
 
-step "run the manifest and verify MD5 was printed"
-apply_manifest_on(agents, manifest) do
-    fail_test "didn't find the content MD5 on output" unless
-        stdout.include? "defined content as '{md5}098f6bcd4621d373cade4e832627b4f6'"
+step "Run the manifest and verify MD5 was printed"
+agents.each do |host|
+  apply_manifest_on(host, manifest) do
+    assert_match(/defined content as '{md5}098f6bcd4621d373cade4e832627b4f6'/, stdout, "#{host}: didn't find the content MD5 on output")
+  end
 end
 
 step "clean up #{file} after testing"
diff --git a/acceptance/tests/puppet_apply_basics.rb b/acceptance/tests/puppet_apply_basics.rb
index bbbdefc15..23c57d84d 100644
--- a/acceptance/tests/puppet_apply_basics.rb
+++ b/acceptance/tests/puppet_apply_basics.rb
@@ -5,11 +5,13 @@
 test_name "Trivial puppet tests"
 
 step "check that puppet apply displays notices"
-apply_manifest_on(agents, "notice 'Hello World'") do
-  stdout =~ /notice:.*Hello World/ or fail_test("missing notice!")
+agents.each do |host|
+  apply_manifest_on(host, "notice 'Hello World'") do
+    assert_match(/notice:.*Hello World/, stdout, "#{host}: missing notice!")
+  end
 end
 
 step "verify help displays something for puppet master"
 on master, puppet_master("--help") do
-  stdout =~ /puppet master/ or fail_test("improper help output")
+  assert_match(/puppet master/, stdout, "improper help output")
 end
diff --git a/acceptance/tests/puppet_apply_should_show_a_notice.rb b/acceptance/tests/puppet_apply_should_show_a_notice.rb
index af6f41ca7..757d29bbf 100644
--- a/acceptance/tests/puppet_apply_should_show_a_notice.rb
+++ b/acceptance/tests/puppet_apply_should_show_a_notice.rb
@@ -1,5 +1,7 @@
 test_name "puppet apply should show a notice"
-apply_manifest_on(agents, "notice 'Hello World'") do
-    fail_test "the notice didn't show" unless
-        stdout =~ /notice: .*: Hello World/
+
+agents.each do |host|
+  apply_manifest_on(host, "notice 'Hello World'") do
+    assert_match(/notice: .*: Hello World/, stdout, "#{host}: the notice didn't show")
+  end
 end
diff --git a/acceptance/tests/ticket_3172_puppet_kick_with_hostnames_on_the_command_line.rb b/acceptance/tests/ticket_3172_puppet_kick_with_hostnames_on_the_command_line.rb
index 436ce29fe..24dd7256b 100644
--- a/acceptance/tests/ticket_3172_puppet_kick_with_hostnames_on_the_command_line.rb
+++ b/acceptance/tests/ticket_3172_puppet_kick_with_hostnames_on_the_command_line.rb
@@ -2,6 +2,8 @@ test_name "#3172: puppet kick with hostnames on the command line"
 step "verify that we trigger our host"
 
 target = 'working.example.org'
-on(agents, puppet_kick(target), :acceptable_exit_codes => [3]) {
-  fail_test "didn't trigger #{target}" unless stdout.include? "Triggering #{target}"
-}
+agents.each do |host|
+  on(host, puppet_kick(target), :acceptable_exit_codes => [3]) {
+    assert_match(/Triggering #{target}/, stdout, "didn't trigger #{target} on #{host}" )
+  }
+end
diff --git a/acceptance/tests/ticket_4059_ralsh_can_change_settings.rb b/acceptance/tests/ticket_4059_ralsh_can_change_settings.rb
index c97bbdbe6..83f5899f2 100644
--- a/acceptance/tests/ticket_4059_ralsh_can_change_settings.rb
+++ b/acceptance/tests/ticket_4059_ralsh_can_change_settings.rb
@@ -11,9 +11,10 @@ on(agents, puppet_resource(content)) do
   stdout.index('Host[example.com]/ensure: created') or
     fail_test("missing notice about host record creation")
 end
-on(agents, "cat #{target}") do
-  stdout =~ /^127\.0\.0\.1\s+example\.com/ or
-    fail_test("missing host record in #{target}")
+agents.each do |host|
+  on(host, "cat #{target}") do
+    assert_match(/^127\.0\.0\.1\s+example\.com/, stdout, "missing host record in #{target} on #{host}")
+  end
 end
 
 step "cleanup at the end of the test"
diff --git a/acceptance/tests/ticket_4110_puppet_apply_should_not_create_a_user_that_already_exists.rb b/acceptance/tests/ticket_4110_puppet_apply_should_not_create_a_user_that_already_exists.rb
index 9704250d9..147857362 100644
--- a/acceptance/tests/ticket_4110_puppet_apply_should_not_create_a_user_that_already_exists.rb
+++ b/acceptance/tests/ticket_4110_puppet_apply_should_not_create_a_user_that_already_exists.rb
@@ -1,5 +1,7 @@
 test_name "#4110: puppet apply should not create a user that already exists"
 
-apply_manifest_on(agents, "user { 'root': ensure => 'present' }") do
-  fail_test("we tried to create root on this host") if stdout =~ /created/
+agents.each do |host|
+  apply_manifest_on(host, "user { 'root': ensure => 'present' }") do
+     assert_no_match(/created/, stdout, "we tried to create root on #{host}" )
+  end
 end
diff --git a/acceptance/tests/ticket_4233_resource_with_a_newline.rb b/acceptance/tests/ticket_4233_resource_with_a_newline.rb
index 7bb7dc3c3..11924b550 100644
--- a/acceptance/tests/ticket_4233_resource_with_a_newline.rb
+++ b/acceptance/tests/ticket_4233_resource_with_a_newline.rb
@@ -8,6 +8,9 @@ test_name "#4233: resource with a newline"
 # and 2.6.0 final to not return an error line.
 # Look for the line in the output and fail the test
 # if we find it.
-apply_manifest_on(agents, 'exec { \'/bin/echo -e "\nHello World\n"\': }') do
-  stdout =~ /err:/ and fail_test("error report in output, sorry")
+
+agents.each do |host|
+  apply_manifest_on(host, 'exec { \'/bin/echo -e "\nHello World\n"\': }') do
+    assert_no_match(/err:/, stdout, "error report in output on #{host}")
+  end
 end
diff --git a/acceptance/tests/ticket_4285_file_resource_fail_when_name_defined_instead_of_path.rb b/acceptance/tests/ticket_4285_file_resource_fail_when_name_defined_instead_of_path.rb
index d2297fbc4..c1cdb0b9d 100644
--- a/acceptance/tests/ticket_4285_file_resource_fail_when_name_defined_instead_of_path.rb
+++ b/acceptance/tests/ticket_4285_file_resource_fail_when_name_defined_instead_of_path.rb
@@ -12,6 +12,8 @@ manifest = %q{
   }
 }
 
-apply_manifest_on(agents, manifest) do
-    fail_test "found the bug report output" if stdout =~ /Cannot alias/
+agents.each do |host|
+  apply_manifest_on(host, manifest) do
+    assert_no_match(/Cannot alias/, stdout, "#{host}: found the bug report output")
+  end
 end
diff --git a/acceptance/tests/ticket_4287_undefined_method_evaluate_match_when_function_call_used_in_an_if_statement.rb b/acceptance/tests/ticket_4287_undefined_method_evaluate_match_when_function_call_used_in_an_if_statement.rb
index e9a17df36..f5a1c1685 100644
--- a/acceptance/tests/ticket_4287_undefined_method_evaluate_match_when_function_call_used_in_an_if_statement.rb
+++ b/acceptance/tests/ticket_4287_undefined_method_evaluate_match_when_function_call_used_in_an_if_statement.rb
@@ -7,7 +7,8 @@ manifest = %q{
   }
 }
 
-apply_manifest_on(agents, manifest) do
-    fail_test "didn't get the expected notice" unless
-        stdout.include? 'notice: No issue here...'
+agents.each do |host|
+  apply_manifest_on(host, manifest) do
+    assert_match(/notice: No issue here.../, stdout, "didn't get the expected notice on #{host}")
+  end
 end
diff --git a/acceptance/tests/ticket_4289_facter_should_recognize_OEL_operatingsystemrelease.rb b/acceptance/tests/ticket_4289_facter_should_recognize_OEL_operatingsystemrelease.rb
index 653fcb274..cacb7db65 100644
--- a/acceptance/tests/ticket_4289_facter_should_recognize_OEL_operatingsystemrelease.rb
+++ b/acceptance/tests/ticket_4289_facter_should_recognize_OEL_operatingsystemrelease.rb
@@ -16,6 +16,6 @@ agents.each do |host|
   if stdout =~ /oel/i then
     step "test operatingsystemrelease fact on OEL host #{host}"
     on host, facter("operatingsystemrelease")
-    stdout =~ /^\d\.\d$/ or fail_test "operatingsystemrelease not as expected"
+    assert_match(/^\d\.\d$/, stdout, "operatingsystemrelease not as expected on #{host}")
   end
 end
diff --git a/acceptance/tests/ticket_4293_define_and_use_a_define_within_a_class.rb b/acceptance/tests/ticket_4293_define_and_use_a_define_within_a_class.rb
index 830da99b4..aa42fd401 100644
--- a/acceptance/tests/ticket_4293_define_and_use_a_define_within_a_class.rb
+++ b/acceptance/tests/ticket_4293_define_and_use_a_define_within_a_class.rb
@@ -16,7 +16,8 @@ class foo {
 include foo
 PP
 
-apply_manifest_on(agents, manifest) do
-  stdout =~ /notice.*?Foo::Do_notify.*?a_message_for_you/ or
-    fail_test("the notification didn't show up in stdout")
+agents.each do |host|
+  apply_manifest_on(host, manifest) do
+    assert_match(/notice.*?Foo::Do_notify.*?a_message_for_you/, stdout, "the notification didn't show up in stdout on #{host}")
+  end
 end
diff --git a/acceptance/tests/ticket_5477_master_not_dectect_sitepp.rb b/acceptance/tests/ticket_5477_master_not_dectect_sitepp.rb
index 3b0e21512..e57b268ef 100644
--- a/acceptance/tests/ticket_5477_master_not_dectect_sitepp.rb
+++ b/acceptance/tests/ticket_5477_master_not_dectect_sitepp.rb
@@ -24,7 +24,9 @@ with_master_running_on(master, "--manifest #{manifest_file} --certdnsnames=\"pup
   sleep 3
 
   step "Agent: puppet agent --test"
-  on agents, puppet_agent("--test"), :acceptable_exit_codes => [2] do
-    fail_test "Site.pp not detect at Master?" unless stdout.include? 'ticket_5477_notify'
+  agents.each do |host|
+    on(host, puppet_agent("--test"), :acceptable_exit_codes => [2]) do
+      assert_match(/ticket_5477_notify/, stdout, "#{host}: Site.pp not detected on Puppet Master")
+    end
   end
 end
diff --git a/acceptance/tests/ticket_6541_invalid_filebucket_files.rb b/acceptance/tests/ticket_6541_invalid_filebucket_files.rb
index 25bcff452..33b985b67 100644
--- a/acceptance/tests/ticket_6541_invalid_filebucket_files.rb
+++ b/acceptance/tests/ticket_6541_invalid_filebucket_files.rb
@@ -4,23 +4,26 @@ apply_manifest_on(agents, manifest)
 
 test_name "verify invalid hashes should not change the file"
 manifest = "file { '/tmp/6541': content => '{md5}notahash' }"
-apply_manifest_on(agents, manifest) do
-  fail_test "shouldn't have overwrote the file" if
-    stdout =~ /content changed/
+agents.each do |host|
+  apply_manifest_on(host, manifest) do
+    assert_no_match(/content changed/, stdout, "#{host}: shouldn't have overwrote the file")
+  end
 end
 
 test_name "verify valid but unbucketed hashes should not change the file"
 manifest = "file { '/tmp/6541': content => '{md5}13ad7345d56b566a4408ffdcd877bc78' }"
-apply_manifest_on(agents, manifest) do
-  fail_test "shouldn't have overwrote the file" if
-    stdout =~ /content changed/
+agents.each do |host|
+  apply_manifest_on(host, manifest) do
+    assert_no_match(/content changed/, stdout, "#{host}: shouldn't have overwrote the file")
+  end
 end
 
 on(agents, puppet_filebucket("backup -l /dev/null") )
 
 test_name "verify that an empty file can be retrieved from the filebucket"
 manifest = "file { '/tmp/6541': content => '{md5}d41d8cd98f00b204e9800998ecf8427e' }"
-apply_manifest_on(agents, manifest) do
-  fail_test "shouldn't have overwrote the file" unless
-    stdout =~ /content changed '\{md5\}552e21cd4cd9918678e3c1a0df491bc3' to '\{md5\}d41d8cd98f00b204e9800998ecf8427e'/
+agents.each do |host|
+  apply_manifest_on(host, manifest) do
+    assert_match(/content changed '\{md5\}552e21cd4cd9918678e3c1a0df491bc3' to '\{md5\}d41d8cd98f00b204e9800998ecf8427e'/, stdout, "#{host}: shouldn't have overwrote the file")
+  end
 end
diff --git a/acceptance/tests/ticket_6734_6256_5530_5503.rb b/acceptance/tests/ticket_6734_6256_5530_5503.rb
index 8f0155efb..a59ac13ab 100644
--- a/acceptance/tests/ticket_6734_6256_5530_5503.rb
+++ b/acceptance/tests/ticket_6734_6256_5530_5503.rb
@@ -9,6 +9,6 @@ with_master_running_on(master) do
 
   step "Check permissions on puppet/rrd/"
   on master, "ls -l /var/lib/puppet | grep rrd | awk '{print $3\" \"$4}'" do
-    fail_test "puppet/rrd does not exist/wrong permission" unless stdout.include? 'puppet puppet'
+    assert_match(/puppet puppet/, stdout, "puppet/rrd does not exist/wrong permissions")
   end
 end
-- 
cgit 


From d02000bf0060045eb27649ce2b433dcac34a7827 Mon Sep 17 00:00:00 2001
From: nfagerlund <nick.fagerlund@gmail.com>
Date: Fri, 15 Jul 2011 10:52:37 -0700
Subject: (#8401) Document that --detailed-exitcodes is a bitmask

The agent/apply/device man pages mentioned the 2 and 4 exit codes, but didn't
mention that they can combine to make 6 if there are both changes and failures.
This commit adds the missing information to all three man pages.

Reviewed-by: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/application/agent.rb  | 8 ++++----
 lib/puppet/application/apply.rb  | 7 ++++---
 lib/puppet/application/device.rb | 8 ++++----
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/lib/puppet/application/agent.rb b/lib/puppet/application/agent.rb
index f0442648b..ea7cbdfb5 100644
--- a/lib/puppet/application/agent.rb
+++ b/lib/puppet/application/agent.rb
@@ -187,10 +187,10 @@ configuration options can also be generated by running puppet agent with
   should always at least contain MD5, MD2, SHA1 and SHA256.
 
 * --detailed-exitcodes:
-  Provide transaction information via exit codes. If this is enabled, an
-  exit code of '2' means there were changes, and an exit code of '4'
-  means that there were failures during the transaction. This option
-  only makes sense in conjunction with --onetime.
+  Provide transaction information via exit codes. If this is enabled, an exit
+  code of '2' means there were changes, an exit code of '4' means there were
+  failures during the transaction, and an exit code of '6' means there were both
+  changes and failures.
 
 * --disable:
   Disable working on the local system. This puts a lock file in place,
diff --git a/lib/puppet/application/apply.rb b/lib/puppet/application/apply.rb
index 5562a9b09..200309b7d 100644
--- a/lib/puppet/application/apply.rb
+++ b/lib/puppet/application/apply.rb
@@ -82,9 +82,10 @@ configuration options can also be generated by running puppet with
   Enable full debugging.
 
 * --detailed-exitcodes:
-  Provide transaction information via exit codes. If this is enabled, an
-  exit code of '2' means there were changes, and an exit code of '4'
-  means that there were failures during the transaction.
+  Provide transaction information via exit codes. If this is enabled, an exit
+  code of '2' means there were changes, an exit code of '4' means there were
+  failures during the transaction, and an exit code of '6' means there were both
+  changes and failures.
 
 * --help:
   Print this help message
diff --git a/lib/puppet/application/device.rb b/lib/puppet/application/device.rb
index 3e2dec98c..977c5c023 100644
--- a/lib/puppet/application/device.rb
+++ b/lib/puppet/application/device.rb
@@ -113,10 +113,10 @@ parameter, so you can specify '--server <servername>' as an argument.
   Enable full debugging.
 
 * --detailed-exitcodes:
-  Provide transaction information via exit codes.  If this is enabled, an
-  exit code of '2' means there were changes, and an exit code of '4' means
-  that there were failures during the transaction. This option only makes
-  sense in conjunction with --onetime.
+  Provide transaction information via exit codes. If this is enabled, an exit
+  code of '2' means there were changes, an exit code of '4' means there were
+  failures during the transaction, and an exit code of '6' means there were both
+  changes and failures.
 
 * --help:
   Print this help message
-- 
cgit 


From 72abe6ce7192bba0b295a8a83483668d21050624 Mon Sep 17 00:00:00 2001
From: Pieter van de Bruggen <pieter@puppetlabs.com>
Date: Tue, 19 Jul 2011 10:55:26 -0700
Subject: (#7204) Consolidate Semantic Versioning code.

This introduces a class representing a semantic version, and
implementing a few of the most common uses of them: validation,
comparison, and finding the greatest available version matching
a range.  This refactoring also allows us to easily expand our
matching of version ranges in the future, which is a big plus.

Reviewed-By: Daniel Pittman
---
 lib/puppet/interface.rb                     |   5 +-
 lib/puppet/interface/face_collection.rb     |  49 +-------
 lib/semver.rb                               |  65 ++++++++++
 spec/unit/interface/face_collection_spec.rb |  40 +-----
 spec/unit/semver_spec.rb                    | 187 ++++++++++++++++++++++++++++
 5 files changed, 266 insertions(+), 80 deletions(-)
 create mode 100644 lib/semver.rb
 create mode 100644 spec/unit/semver_spec.rb

diff --git a/lib/puppet/interface.rb b/lib/puppet/interface.rb
index 6be8b6930..6c288f3c0 100644
--- a/lib/puppet/interface.rb
+++ b/lib/puppet/interface.rb
@@ -2,6 +2,7 @@ require 'puppet'
 require 'puppet/util/autoload'
 require 'puppet/interface/documentation'
 require 'prettyprint'
+require 'semver'
 
 class Puppet::Interface
   include FullDocs
@@ -84,12 +85,12 @@ class Puppet::Interface
   attr_reader :name, :version
 
   def initialize(name, version, &block)
-    unless Puppet::Interface::FaceCollection.validate_version(version)
+    unless SemVer.valid?(version)
       raise ArgumentError, "Cannot create face #{name.inspect} with invalid version number '#{version}'!"
     end
 
     @name    = Puppet::Interface::FaceCollection.underscorize(name)
-    @version = version
+    @version = SemVer.new(version)
 
     # The few bits of documentation we actually demand.  The default license
     # is a favour to our end users; if you happen to get that in a core face
diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index 12d3c56b1..4522824fd 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -1,8 +1,6 @@
 require 'puppet/interface'
 
 module Puppet::Interface::FaceCollection
-  SEMVER_VERSION = /^(\d+)\.(\d+)\.(\d+)([A-Za-z][0-9A-Za-z-]*|)$/
-
   @faces = Hash.new { |hash, key| hash[key] = {} }
 
   def self.faces
@@ -17,55 +15,18 @@ module Puppet::Interface::FaceCollection
     @faces.keys.select {|name| @faces[name].length > 0 }
   end
 
-  def self.validate_version(version)
-    !!(SEMVER_VERSION =~ version.to_s)
-  end
-
-  def self.semver_to_array(v)
-    parts = SEMVER_VERSION.match(v).to_a[1..4]
-    parts[0..2] = parts[0..2].map { |e| e.to_i }
-    parts
-  end
-
-  def self.cmp_semver(a, b)
-    a, b = [a, b].map do |x| semver_to_array(x) end
-
-    cmp = a[0..2] <=> b[0..2]
-    if cmp == 0
-      cmp = a[3] <=> b[3]
-      cmp = +1 if a[3].empty? && !b[3].empty?
-      cmp = -1 if b[3].empty? && !a[3].empty?
-    end
-    cmp
-  end
-
-  def self.prefix_match?(desired, target)
-    # Can't meaningfully do a prefix match with current on either side.
-    return false if desired == :current
-    return false if target  == :current
-
-    # REVISIT: Should probably fail if the matcher is not valid.
-    prefix = desired.split('.').map {|x| x =~ /^\d+$/ and x.to_i }
-    have   = semver_to_array(target)
-
-    while want = prefix.shift do
-      return false unless want == have.shift
-    end
-    return true
-  end
-
   def self.[](name, version)
     name = underscorize(name)
     get_face(name, version) or load_face(name, version)
   end
 
   # get face from memory, without loading.
-  def self.get_face(name, desired_version)
+  def self.get_face(name, pattern)
     return nil unless @faces.has_key? name
+    return @faces[name][:current] if pattern == :current
 
-    return @faces[name][:current] if desired_version == :current
-
-    found = @faces[name].keys.select {|v| prefix_match?(desired_version, v) }.sort.last
+    versions = @faces[name].keys - [ :current ]
+    found    = SemVer.find_matching(pattern, versions)
     return @faces[name][found]
   end
 
@@ -108,7 +69,7 @@ module Puppet::Interface::FaceCollection
         # versions here and return the last item in that set.
         #
         # --daniel 2011-04-06
-        latest_ver = @faces[name].keys.sort {|a, b| cmp_semver(a, b) }.last
+        latest_ver = @faces[name].keys.sort.last
         @faces[name][:current] = @faces[name][latest_ver]
       end
     rescue LoadError => e
diff --git a/lib/semver.rb b/lib/semver.rb
new file mode 100644
index 000000000..ef9435abd
--- /dev/null
+++ b/lib/semver.rb
@@ -0,0 +1,65 @@
+class SemVer
+  VERSION = /^v?(\d+)\.(\d+)\.(\d+)([A-Za-z][0-9A-Za-z-]*|)$/
+  SIMPLE_RANGE = /^v?(\d+|[xX])(?:\.(\d+|[xX])(?:\.(\d+|[xX]))?)?$/
+
+  include Comparable
+
+  def self.valid?(ver)
+    VERSION =~ ver
+  end
+
+  def self.find_matching(pattern, versions)
+    versions.select { |v| v.matched_by?("#{pattern}") }.sort.last
+  end
+
+  attr_reader :major, :minor, :tiny, :special
+
+  def initialize(ver)
+    unless SemVer.valid?(ver)
+      raise ArgumentError.new("Invalid version string '#{ver}'!")
+    end
+
+    @major, @minor, @tiny, @special = VERSION.match(ver).captures.map do |x|
+      # Because Kernel#Integer tries to interpret hex and octal strings, which
+      # we specifically do not want, and which cannot be overridden in 1.8.7.
+      Float(x).to_i rescue x
+    end
+  end
+
+  def <=>(other)
+    other = SemVer.new("#{other}") unless other.is_a? SemVer
+    return self.major <=> other.major unless self.major == other.major
+    return self.minor <=> other.minor unless self.minor == other.minor
+    return self.tiny  <=> other.tiny  unless self.tiny  == other.tiny
+
+    return 0  if self.special  == other.special
+    return 1  if self.special  == ''
+    return -1 if other.special == ''
+
+    return self.special <=> other.special
+  end
+
+  def matched_by?(pattern)
+    # For the time being, this is restricted to exact version matches and
+    # simple range patterns.  In the future, we should implement some or all of
+    # the comparison operators here:
+    # https://github.com/isaacs/node-semver/blob/d474801/semver.js#L340
+
+    case pattern
+    when SIMPLE_RANGE
+      pattern = SIMPLE_RANGE.match(pattern).captures
+      pattern[1] = @minor unless pattern[1] && pattern[1] !~ /x/i
+      pattern[2] = @tiny  unless pattern[2] && pattern[2] !~ /x/i
+      [@major, @minor, @tiny] == pattern.map { |x| x.to_i }
+    when VERSION
+      self == SemVer.new(pattern)
+    else
+      false
+    end
+  end
+
+  def inspect
+    "v#{@major}.#{@minor}.#{@tiny}#{@special}"
+  end
+  alias :to_s :inspect
+end
diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index 4ad8787c5..98887a778 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -25,39 +25,9 @@ describe Puppet::Interface::FaceCollection do
     @original_required.each {|f| $".push f unless $".include? f }
   end
 
-  describe "::prefix_match?" do
-    #   want     have
-    { ['1.0.0', '1.0.0'] => true,
-      ['1.0',   '1.0.0'] => true,
-      ['1',     '1.0.0'] => true,
-      ['1.0.0', '1.1.0'] => false,
-      ['1.0',   '1.1.0'] => false,
-      ['1',     '1.1.0'] => true,
-      ['1.0.1', '1.0.0'] => false,
-    }.each do |data, result|
-      it "should return #{result.inspect} for prefix_match?(#{data.join(', ')})" do
-        subject.prefix_match?(*data).should == result
-      end
-    end
-  end
-
-  describe "::validate_version" do
-    { '10.10.10'     => true,
-      '1.2.3.4'      => false,
-      '10.10.10beta' => true,
-      '10.10'        => false,
-      '123'          => false,
-      'v1.1.1'       => false,
-    }.each do |input, result|
-      it "should#{result ? '' : ' not'} permit #{input.inspect}" do
-        subject.validate_version(input).should(result ? be_true : be_false)
-      end
-    end
-  end
-
   describe "::[]" do
     before :each do
-      subject.instance_variable_get("@faces")[:foo]['0.0.1'] = 10
+      subject.instance_variable_get("@faces")[:foo][SemVer.new('0.0.1')] = 10
     end
 
     it "should return the face with the given name" do
@@ -75,13 +45,13 @@ describe Puppet::Interface::FaceCollection do
     end
 
     it "should return true if the face specified is registered" do
-      subject.instance_variable_get("@faces")[:foo]['0.0.1'] = 10
+      subject.instance_variable_get("@faces")[:foo][SemVer.new('0.0.1')] = 10
       subject["foo", '0.0.1'].should == 10
     end
 
     it "should attempt to require the face if it is not registered" do
       subject.expects(:require).with do |file|
-        subject.instance_variable_get("@faces")[:bar]['0.0.1'] = true
+        subject.instance_variable_get("@faces")[:bar][SemVer.new('0.0.1')] = true
         file == 'puppet/face/bar'
       end
       subject["bar", '0.0.1'].should be_true
@@ -131,7 +101,9 @@ describe Puppet::Interface::FaceCollection do
     it "should store the face by name" do
       face = Puppet::Face.new(:my_face, '0.0.1')
       subject.register(face)
-      subject.instance_variable_get("@faces").should == {:my_face => {'0.0.1' => face}}
+      subject.instance_variable_get("@faces").should == {
+        :my_face => { face.version => face }
+      }
     end
   end
 
diff --git a/spec/unit/semver_spec.rb b/spec/unit/semver_spec.rb
new file mode 100644
index 000000000..0e0457b6e
--- /dev/null
+++ b/spec/unit/semver_spec.rb
@@ -0,0 +1,187 @@
+require 'spec_helper'
+require 'semver'
+
+describe SemVer do
+  describe '::valid?' do
+    it 'should validate basic version strings' do
+      %w[ 0.0.0 999.999.999 v0.0.0 v999.999.999 ].each do |vstring|
+        SemVer.valid?(vstring).should be_true
+      end
+    end
+
+    it 'should validate special version strings' do
+      %w[ 0.0.0foo 999.999.999bar v0.0.0a v999.999.999beta ].each do |vstring|
+        SemVer.valid?(vstring).should be_true
+      end
+    end
+
+    it 'should fail to validate invalid version strings' do
+      %w[ nope 0.0foo 999.999 x0.0.0 z.z.z 1.2.3-beta 1.x.y ].each do |vstring|
+        SemVer.valid?(vstring).should be_false
+      end
+    end
+  end
+
+  describe '::find_matching' do
+    before :all do
+      @versions = %w[
+        0.0.1
+        0.0.2
+        1.0.0rc1
+        1.0.0rc2
+        1.0.0
+        1.0.1
+        1.1.0
+        1.1.1
+        1.1.2
+        1.1.3
+        1.1.4
+        1.2.0
+        1.2.1
+        2.0.0rc1
+      ].map { |v| SemVer.new(v) }
+    end
+
+    it 'should match exact versions by string' do
+      @versions.each do |version|
+        SemVer.find_matching(version, @versions).should == version
+      end
+    end
+
+    it 'should return nil if no versions match' do
+      %w[ 3.0.0 2.0.0rc2 1.0.0alpha ].each do |v|
+        SemVer.find_matching(v, @versions).should be_nil
+      end
+    end
+
+    it 'should find the greatest match for partial versions' do
+      SemVer.find_matching('1.0', @versions).should == 'v1.0.1'
+      SemVer.find_matching('1.1', @versions).should == 'v1.1.4'
+      SemVer.find_matching('1', @versions).should   == 'v1.2.1'
+      SemVer.find_matching('2', @versions).should   == 'v2.0.0rc1'
+      SemVer.find_matching('2.1', @versions).should == nil
+    end
+
+
+    it 'should find the greatest match for versions with placeholders' do
+      SemVer.find_matching('1.0.x', @versions).should == 'v1.0.1'
+      SemVer.find_matching('1.1.x', @versions).should == 'v1.1.4'
+      SemVer.find_matching('1.x', @versions).should   == 'v1.2.1'
+      SemVer.find_matching('1.x.x', @versions).should == 'v1.2.1'
+      SemVer.find_matching('2.x', @versions).should   == 'v2.0.0rc1'
+      SemVer.find_matching('2.x.x', @versions).should == 'v2.0.0rc1'
+      SemVer.find_matching('2.1.x', @versions).should == nil
+    end
+  end
+
+  describe 'instantiation' do
+    it 'should raise an exception when passed an invalid version string' do
+      expect { SemVer.new('invalidVersion') }.to raise_exception ArgumentError
+    end
+
+    it 'should populate the appropriate fields for a basic version string' do
+      version = SemVer.new('1.2.3')
+      version.major.should   == 1
+      version.minor.should   == 2
+      version.tiny.should    == 3
+      version.special.should == ''
+    end
+
+    it 'should populate the appropriate fields for a special version string' do
+      version = SemVer.new('3.4.5beta6')
+      version.major.should   == 3
+      version.minor.should   == 4
+      version.tiny.should    == 5
+      version.special.should == 'beta6'
+    end
+  end
+
+  describe '#matched_by?' do
+    subject { SemVer.new('v1.2.3beta') }
+
+    describe 'should match against' do
+      describe 'literal version strings' do
+        it { should be_matched_by('1.2.3beta') }
+
+        it { should_not be_matched_by('1.2.3alpha') }
+        it { should_not be_matched_by('1.2.4beta') }
+        it { should_not be_matched_by('1.3.3beta') }
+        it { should_not be_matched_by('2.2.3beta') }
+      end
+
+      describe 'partial version strings' do
+        it { should be_matched_by('1.2.3') }
+        it { should be_matched_by('1.2') }
+        it { should be_matched_by('1') }
+      end
+
+      describe 'version strings with placeholders' do
+        it { should be_matched_by('1.2.x') }
+        it { should be_matched_by('1.x.3') }
+        it { should be_matched_by('1.x.x') }
+        it { should be_matched_by('1.x') }
+      end
+    end
+  end
+
+  describe 'comparisons' do
+    describe 'against a string' do
+      it 'should just work' do
+        SemVer.new('1.2.3').should == '1.2.3'
+      end
+    end
+
+    describe 'against a symbol' do
+      it 'should just work' do
+        SemVer.new('1.2.3').should == :'1.2.3'
+      end
+    end
+
+    describe 'on a basic version (v1.2.3)' do
+      subject { SemVer.new('v1.2.3') }
+
+      it { should == SemVer.new('1.2.3') }
+
+      # Different major versions
+      it { should > SemVer.new('0.2.3') }
+      it { should < SemVer.new('2.2.3') }
+
+      # Different minor versions
+      it { should > SemVer.new('1.1.3') }
+      it { should < SemVer.new('1.3.3') }
+
+      # Different tiny versions
+      it { should > SemVer.new('1.2.2') }
+      it { should < SemVer.new('1.2.4') }
+
+      # Against special versions
+      it { should > SemVer.new('1.2.3beta') }
+      it { should < SemVer.new('1.2.4beta') }
+    end
+
+    describe 'on a special version (v1.2.3beta)' do
+      subject { SemVer.new('v1.2.3beta') }
+
+      it { should == SemVer.new('1.2.3beta') }
+
+      # Same version, final release
+      it { should < SemVer.new('1.2.3') }
+
+      # Different major versions
+      it { should > SemVer.new('0.2.3') }
+      it { should < SemVer.new('2.2.3') }
+
+      # Different minor versions
+      it { should > SemVer.new('1.1.3') }
+      it { should < SemVer.new('1.3.3') }
+
+      # Different tiny versions
+      it { should > SemVer.new('1.2.2') }
+      it { should < SemVer.new('1.2.4') }
+
+      # Against special versions
+      it { should > SemVer.new('1.2.3alpha') }
+      it { should < SemVer.new('1.2.3beta2') }
+    end
+  end
+end
-- 
cgit 


From cc311ad3dcb70547d7249e7aec9f545672c3e8e2 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Tue, 19 Jul 2011 16:45:26 -0700
Subject: maint: SSL::Inventory.serial should report missing names.

Our SSL inventory was able to find the serial number of a certificate by name,
but was incapable of living up to the contract it offered, that it would
actually report when a certificate was missing.

Now it returns `nil`, which is the same case as "no inventory", if the
certificate was not found, rather than accidentally returning the entire
inventory data as raw strings.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/ssl/inventory.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/puppet/ssl/inventory.rb b/lib/puppet/ssl/inventory.rb
index e094da100..c210fdc35 100644
--- a/lib/puppet/ssl/inventory.rb
+++ b/lib/puppet/ssl/inventory.rb
@@ -48,5 +48,7 @@ class Puppet::SSL::Inventory
 
       return Integer($1)
     end
+
+    return nil
   end
 end
-- 
cgit 


From c830ab01f31b3c6999953783a0ec1939bdbc25a9 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 18 Jul 2011 11:40:05 -0700
Subject: (#6789) Port SSL::CertificateAuthority::Interface to a Face

The Puppet::SSL::CertificateAuthority::Interface class was an early prototype
heading toward building out a system like Faces.  Now that we have done that,
this changeset ports the early code to a new face.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/application/ca.rb |   5 +
 lib/puppet/face/ca.rb        | 233 ++++++++++++++++++++++++++++
 spec/unit/face/ca_spec.rb    | 355 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 593 insertions(+)
 create mode 100644 lib/puppet/application/ca.rb
 create mode 100644 lib/puppet/face/ca.rb
 create mode 100755 spec/unit/face/ca_spec.rb

diff --git a/lib/puppet/application/ca.rb b/lib/puppet/application/ca.rb
new file mode 100644
index 000000000..d1ec2502e
--- /dev/null
+++ b/lib/puppet/application/ca.rb
@@ -0,0 +1,5 @@
+require 'puppet/application/face_base'
+
+class Puppet::Application::Ca < Puppet::Application::FaceBase
+  run_mode :master
+end
diff --git a/lib/puppet/face/ca.rb b/lib/puppet/face/ca.rb
new file mode 100644
index 000000000..e643530f0
--- /dev/null
+++ b/lib/puppet/face/ca.rb
@@ -0,0 +1,233 @@
+require 'puppet/face'
+
+Puppet::Face.define(:ca, '0.1.0') do
+  copyright "Puppet Labs", 2011
+  license   "Apache 2 license; see COPYING"
+
+  summary "Local Puppet Certificate Authority management."
+
+  description <<TEXT
+This provides local management of the Puppet Certificate Authority.
+
+You can use this subcommand to sign outstanding certificate requests, list
+and manage local certificates, and inspect the state of the CA.
+TEXT
+
+  action :list do
+    summary "List certificates and/or certificate requests."
+
+    description <<-end
+This will list the current certificates and certificate signing requests
+in the Puppet CA.  You will also get the fingerprint, and any certificate
+verification failure reported.
+    end
+
+    option "--[no-]all" do
+      summary "Include all certificates and requests."
+    end
+
+    option "--[no-]pending" do
+      summary "Include pending certificate signing requests."
+    end
+
+    option "--[no-]signed" do
+      summary "Include signed certificates."
+    end
+
+    option "--subject PATTERN" do
+      summary "Only list if the subject matches PATTERN."
+
+      description <<TEXT
+Only include certificates or requests where subject matches PATTERN.
+
+PATTERN is interpreted as a regular expression, allowing complex
+filtering of the content.
+TEXT
+    end
+
+    when_invoked do |options|
+      raise "Not a CA" unless Puppet::SSL::CertificateAuthority.ca?
+      unless ca = Puppet::SSL::CertificateAuthority.instance
+        raise "Unable to fetch the CA"
+      end
+
+      pattern = options[:subject].nil? ? nil :
+        Regexp.new(options[:subject], Regexp::IGNORECASE)
+
+      pending = options[:pending].nil? ? options[:all] : options[:pending]
+      signed  = options[:signed].nil?  ? options[:all] : options[:signed]
+
+      # By default we list pending, so if nothing at all was requested...
+      unless pending or signed then pending = true end
+
+      hosts = []
+
+      pending and hosts += ca.waiting?
+      signed  and hosts += ca.list
+
+      pattern and hosts = hosts.select {|hostname| pattern.match hostname }
+
+      hosts.sort.map {|host| Puppet::SSL::Host.new(host) }
+    end
+
+    when_rendering :console do |hosts|
+      unless ca = Puppet::SSL::CertificateAuthority.instance
+        raise "Unable to fetch the CA"
+      end
+
+      length = hosts.map{|x| x.name.length }.max + 1
+
+      hosts.map do |host|
+        name = host.name.ljust(length)
+        if host.certificate_request then
+          "  #{name} (#{host.certificate_request.fingerprint})"
+        else
+          begin
+            ca.verify(host.certificate)
+            "+ #{name} (#{host.certificate.fingerprint})"
+          rescue Puppet::SSL::CertificateAuthority::CertificateVerificationError => e
+            "- #{name} (#{host.certificate.fingerprint}) (#{e.to_s})"
+          end
+        end
+      end.join("\n")
+    end
+  end
+
+  action :destroy do
+    when_invoked do |host, options|
+      raise "Not a CA" unless Puppet::SSL::CertificateAuthority.ca?
+      unless ca = Puppet::SSL::CertificateAuthority.instance
+        raise "Unable to fetch the CA"
+      end
+
+      ca.destroy host
+    end
+  end
+
+  action :revoke do
+    when_invoked do |host, options|
+      raise "Not a CA" unless Puppet::SSL::CertificateAuthority.ca?
+      unless ca = Puppet::SSL::CertificateAuthority.instance
+        raise "Unable to fetch the CA"
+      end
+
+      begin
+        ca.revoke host
+      rescue ArgumentError => e
+        # This is a bit naff, but it makes the behaviour consistent with the
+        # destroy action.  The underlying tools could be nicer for that sort
+        # of thing; they have fairly inconsistent reporting of failures.
+        raise unless e.to_s =~ /Could not find a serial number for /
+        "Nothing was revoked"
+      end
+    end
+  end
+
+  action :generate do
+    when_invoked do |host, options|
+      raise "Not a CA" unless Puppet::SSL::CertificateAuthority.ca?
+      unless ca = Puppet::SSL::CertificateAuthority.instance
+        raise "Unable to fetch the CA"
+      end
+
+      begin
+        ca.generate host
+      rescue RuntimeError => e
+        if e.to_s =~ /already has a requested certificate/
+          "#{host} already has a certificate request; use sign instead"
+        else
+          raise
+        end
+      rescue ArgumentError => e
+        if e.to_s =~ /A Certificate already exists for /
+          "#{host} already has a certificate"
+        else
+          raise
+        end
+      end
+    end
+  end
+
+  action :sign do
+    when_invoked do |host, options|
+      raise "Not a CA" unless Puppet::SSL::CertificateAuthority.ca?
+      unless ca = Puppet::SSL::CertificateAuthority.instance
+        raise "Unable to fetch the CA"
+      end
+
+      begin
+        ca.sign host
+      rescue ArgumentError => e
+        if e.to_s =~ /Could not find certificate request/
+          e.to_s
+        else
+          raise
+        end
+      end
+    end
+  end
+
+  action :print do
+    when_invoked do |host, options|
+      raise "Not a CA" unless Puppet::SSL::CertificateAuthority.ca?
+      unless ca = Puppet::SSL::CertificateAuthority.instance
+        raise "Unable to fetch the CA"
+      end
+
+      ca.print host
+    end
+  end
+
+  action :fingerprint do
+    option "--digest ALGORITHM" do
+      summary "The hash algorithm to use when displaying the fingerprint"
+    end
+
+    when_invoked do |host, options|
+      raise "Not a CA" unless Puppet::SSL::CertificateAuthority.ca?
+      unless ca = Puppet::SSL::CertificateAuthority.instance
+        raise "Unable to fetch the CA"
+      end
+
+      begin
+        # I want the default from the CA, not to duplicate it, but passing
+        # 'nil' explicitly means that we don't get that.  This works...
+        if options.has_key? :digest
+          ca.fingerprint host, options[:digest]
+        else
+          ca.fingerprint host
+        end
+      rescue ArgumentError => e
+        raise unless e.to_s =~ /Could not find a certificate or csr for/
+        nil
+      end
+    end
+  end
+
+  action :verify do
+    when_invoked do |host, options|
+      raise "Not a CA" unless Puppet::SSL::CertificateAuthority.ca?
+      unless ca = Puppet::SSL::CertificateAuthority.instance
+        raise "Unable to fetch the CA"
+      end
+
+      begin
+        ca.verify host
+        { :host => host, :valid => true }
+      rescue ArgumentError => e
+        raise unless e.to_s =~ /Could not find a certificate for/
+        { :host => host, :valid => false, :error => e.to_s }
+      rescue Puppet::SSL::CertificateAuthority::CertificateVerificationError => e
+        { :host => host, :valid => false, :error => e.to_s }
+      end
+    end
+
+    when_rendering :console do |value|
+      if value[:valid]
+        nil
+      else
+        "Could not verify #{value[:host]}: #{value[:error]}"
+      end
+    end
+  end
+end
diff --git a/spec/unit/face/ca_spec.rb b/spec/unit/face/ca_spec.rb
new file mode 100755
index 000000000..84769605f
--- /dev/null
+++ b/spec/unit/face/ca_spec.rb
@@ -0,0 +1,355 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+require 'puppet/face'
+
+describe Puppet::Face[:ca, '0.1.0'] do
+  include PuppetSpec::Files
+
+  before :each do
+    Puppet.run_mode.stubs(:master?).returns(true)
+    Puppet[:ca]     = true
+    Puppet[:ssldir] = tmpdir("face-ca-ssldir")
+
+    Puppet::SSL::Host.ca_location = :only
+    Puppet[:certificate_revocation] = true
+
+    # This is way more intimate than I want to be with the implementation, but
+    # there doesn't seem any other way to test this. --daniel 2011-07-18
+    Puppet::SSL::CertificateAuthority.stubs(:instance).returns(
+        # ...and this actually does the directory creation, etc.
+        Puppet::SSL::CertificateAuthority.new
+    )
+  end
+
+  def make_certs(csr_names, crt_names)
+    Array(csr_names).map do |name|
+      Puppet::SSL::Host.new(name).generate_certificate_request
+    end
+
+    Array(crt_names).map do |name|
+      Puppet::SSL::Host.new(name).generate
+    end
+  end
+
+  context "#verify" do
+    let :action do Puppet::Face[:ca, '0.1.0'].get_action(:verify) end
+
+    it "should not explode if there is no certificate" do
+      expect {
+        subject.verify('random-host').should == {
+          :host => 'random-host', :valid => false,
+          :error => 'Could not find a certificate for random-host'
+        }
+      }.should_not raise_error
+    end
+
+    it "should not explode if there is only a CSR" do
+      make_certs('random-host', [])
+      expect {
+        subject.verify('random-host').should == {
+          :host => 'random-host', :valid => false,
+          :error => 'Could not find a certificate for random-host'
+        }
+      }.should_not raise_error
+    end
+
+    it "should verify a signed certificate" do
+      make_certs([], 'random-host')
+      subject.verify('random-host').should == {
+        :host => 'random-host', :valid => true
+      }
+    end
+
+    it "should not verify a revoked certificate" do
+      make_certs([], 'random-host')
+      subject.revoke('random-host')
+
+      expect {
+        subject.verify('random-host').should == {
+          :host => 'random-host', :valid => false,
+          :error => 'certificate revoked'
+        }
+      }.should_not raise_error
+    end
+
+    it "should verify a revoked certificate if CRL use was turned off" do
+      make_certs([], 'random-host')
+      subject.revoke('random-host')
+
+      Puppet[:certificate_revocation] = false
+      subject.verify('random-host').should == {
+        :host => 'random-host', :valid => true
+      }
+    end
+  end
+
+  context "#fingerprint" do
+    let :action do Puppet::Face[:ca, '0.1.0'].get_action(:fingerprint) end
+
+    it "should have a 'digest' option" do
+      action.should be_option :digest
+    end
+
+    it "should not explode if there is no certificate" do
+      expect {
+        subject.fingerprint('random-host').should be_nil
+      }.should_not raise_error
+    end
+
+    it "should fingerprint a CSR" do
+      make_certs('random-host', [])
+      expect {
+        subject.fingerprint('random-host').should =~ /^[0-9A-F:]+$/
+      }.should_not raise_error
+    end
+
+    it "should fingerprint a certificate" do
+      make_certs([], 'random-host')
+      subject.fingerprint('random-host').should =~ /^[0-9A-F:]+$/
+    end
+
+    %w{md5 MD5 sha1 ShA1 SHA1 RIPEMD160 sha256 sha512}.each do |digest|
+      it "should fingerprint with #{digest.inspect}" do
+        make_certs([], 'random-host')
+        subject.fingerprint('random-host', :digest => digest).should =~ /^[0-9A-F:]+$/
+      end
+
+      it "should fingerprint with #{digest.to_sym} as a symbol" do
+        make_certs([], 'random-host')
+        subject.fingerprint('random-host', :digest => digest.to_sym).
+          should =~ /^[0-9A-F:]+$/
+      end
+    end
+  end
+
+  context "#print" do
+    let :action do Puppet::Face[:ca, '0.1.0'].get_action(:print) end
+
+    it "should not explode if there is no certificate" do
+      expect {
+        subject.print('random-host').should be_nil
+      }.should_not raise_error
+    end
+
+    it "should return nothing if there is only a CSR" do
+      make_certs('random-host', [])
+      expect {
+        subject.print('random-host').should be_nil
+      }.should_not raise_error
+    end
+
+    it "should return the certificate content if there is a cert" do
+      make_certs([], 'random-host')
+      text = subject.print('random-host')
+      text.should be_an_instance_of String
+      text.should =~ /^Certificate:/
+      text.should =~ /Issuer: CN=Puppet CA: /
+      text.should =~ /Subject: CN=random-host$/
+    end
+  end
+
+  context "#sign" do
+    let :action do Puppet::Face[:ca, '0.1.0'].get_action(:sign) end
+
+    it "should not explode if there is no CSR" do
+      expect {
+        subject.sign('random-host').
+          should == 'Could not find certificate request for random-host'
+      }.should_not raise_error
+    end
+
+    it "should not explode if there is a signed cert" do
+      make_certs([], 'random-host')
+      expect {
+        subject.sign('random-host').
+          should == 'Could not find certificate request for random-host'
+      }.should_not raise_error
+    end
+
+    it "should sign a CSR if one exists" do
+      make_certs('random-host', [])
+      subject.sign('random-host').should be_an_instance_of Puppet::SSL::Certificate
+
+      list = subject.list(:signed => true)
+      list.count.should == 1
+      list.first.name.should == 'random-host'
+    end
+  end
+
+  context "#generate" do
+    let :action do Puppet::Face[:ca, '0.1.0'].get_action(:generate) end
+
+    it "should generate a certificate if requested" do
+      subject.list(:all => true).should == []
+
+      subject.generate('random-host')
+
+      list = subject.list(:signed => true)
+      list.count.should == 1
+      list.first.name.should == 'random-host'
+    end
+
+    it "should not explode if a CSR with that name already exists" do
+      make_certs('random-host', [])
+      expect {
+        subject.generate('random-host').should =~ /already has a certificate request/
+      }.should_not raise_error
+    end
+
+    it "should not explode if the certificate with that name already exists" do
+      make_certs([], 'random-host')
+      expect {
+        subject.generate('random-host').should =~ /already has a certificate/
+      }.should_not raise_error
+    end
+  end
+
+  context "#revoke" do
+    let :action do Puppet::Face[:ca, '0.1.0'].get_action(:revoke) end
+
+    it "should not explode when asked to revoke something that doesn't exist" do
+      expect { subject.revoke('nonesuch') }.should_not raise_error
+    end
+
+    it "should let the user know what went wrong" do
+      subject.revoke('nonesuch').should == 'Nothing was revoked'
+    end
+
+    it "should revoke a certificate" do
+      make_certs([], 'random-host')
+      found = subject.list(:all => true, :subject => 'random-host')
+      subject.get_action(:list).when_rendering(:console).call(found).
+        should =~ /^\+ random-host/
+
+      subject.revoke('random-host')
+
+      found = subject.list(:all => true, :subject => 'random-host')
+      subject.get_action(:list).when_rendering(:console).call(found).
+        should =~ /^- random-host  \([:0-9A-F]+\) \(certificate revoked\)/
+    end
+  end
+
+  context "#destroy" do
+    let :action do Puppet::Face[:ca, '0.1.0'].get_action(:destroy) end
+
+    it "should not explode when asked to delete something that doesn't exist" do
+      expect { subject.destroy('nonesuch') }.should_not raise_error
+    end
+
+    it "should let the user know if nothing was deleted" do
+      subject.destroy('nonesuch').should == "Nothing was deleted"
+    end
+
+    it "should destroy a CSR, if we have one" do
+      make_certs('random-host', [])
+      subject.list(:pending => true, :subject => 'random-host').should_not == []
+
+      subject.destroy('random-host')
+
+      subject.list(:pending => true, :subject => 'random-host').should == []
+    end
+
+    it "should destroy a certificate, if we have one" do
+      make_certs([], 'random-host')
+      subject.list(:signed => true, :subject => 'random-host').should_not == []
+
+      subject.destroy('random-host')
+
+      subject.list(:signed => true, :subject => 'random-host').should == []
+    end
+
+    it "should tell the user something was deleted" do
+      make_certs([], 'random-host')
+      subject.list(:signed => true, :subject => 'random-host').should_not == []
+      subject.destroy('random-host').
+        should == "Deleted for random-host: Puppet::SSL::Certificate, Puppet::SSL::Key"
+    end
+  end
+
+  context "#list" do
+    let :action do Puppet::Face[:ca, '0.1.0'].get_action(:list) end
+
+    context "options" do
+      subject { Puppet::Face[:ca, '0.1.0'].get_action(:list) }
+      it { should be_option :pending }
+      it { should be_option :signed  }
+      it { should be_option :all     }
+      it { should be_option :subject }
+    end
+
+    context "with no hosts in CA" do
+      [:pending, :signed, :all].each do |type|
+        it "should return nothing for #{type}" do
+          subject.list(type => true).should == []
+        end
+
+        it "should not fail when a matcher is passed" do
+          expect {
+            subject.list(type => true, :subject => '.').should == []
+          }.should_not raise_error
+        end
+      end
+    end
+
+    context "with some hosts" do
+      csr_names = (1..3).map {|n| "csr-#{n}" }
+      crt_names = (1..3).map {|n| "crt-#{n}" }
+      all_names = csr_names + crt_names
+
+      {
+        {}                                    => csr_names,
+        { :pending => true                  } => csr_names,
+
+        { :signed  => true                  } => crt_names,
+
+        { :all     => true                  } => all_names,
+        { :pending => true, :signed => true } => all_names,
+      }.each do |input, expect|
+        it "should map #{input.inspect} to #{expect.inspect}" do
+          make_certs(csr_names, crt_names)
+          subject.list(input).map(&:name).should =~ expect
+        end
+
+        ['', '.', '2', 'none'].each do |pattern|
+          filtered = expect.select {|x| Regexp.new(pattern).match(x) }
+
+          it "should filter all hosts matching #{pattern.inspect} to #{filtered.inspect}" do
+            make_certs(csr_names, crt_names)
+            subject.list(input.merge :subject => pattern).map(&:name).should =~ filtered
+          end
+        end
+      end
+
+      context "when_rendering :console" do
+        { [["csr1.local"], []] => '^  csr1.local ',
+          [[], ["crt1.local"]] => '^\+ crt1.local ',
+          [["csr2"], ["crt2"]] => ['^  csr2 ', '^\+ crt2 ']
+        }.each do |input, pattern|
+          it "should render #{input.inspect} to match #{pattern.inspect}" do
+            make_certs(*input)
+            text = action.when_rendering(:console).call(subject.list(:all => true))
+            Array(pattern).each do |item|
+              text.should =~ Regexp.new(item)
+            end
+          end
+        end
+      end
+    end
+  end
+
+  actions = %w{destroy list revoke generate sign print verify fingerprint}
+  actions.each do |action|
+    it { should be_action action }
+    it "should fail #{action} when not a CA" do
+      Puppet[:ca] = false
+      expect {
+        case subject.method(action).arity
+        when -1 then subject.send(action)
+        when -2 then subject.send(action, 'dummy')
+        else
+          raise "#{action} has arity #{subject.method(action).arity}"
+        end
+      }.should raise_error(/Not a CA/)
+    end
+  end
+end
-- 
cgit 


From bdd6a17c995175d240cd2cb098ab41e612ba8d63 Mon Sep 17 00:00:00 2001
From: Nick Lewis <nick@puppetlabs.com>
Date: Wed, 20 Jul 2011 15:57:20 -0700
Subject: Fix order-dependent test failure in certificate_status/file spec

This test was failing if the SSL indirections had previously been configured as
:ca. The was due to the fact that we are explicitly testing the
certificate_status :file terminus, which depends on the other SSL indirections
using corresponding termini. This spec wasn't appropriately ensuring they were
also set to :file, breaking that precondition, and causing failures.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/indirector/certificate_status/file_spec.rb | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/spec/unit/indirector/certificate_status/file_spec.rb b/spec/unit/indirector/certificate_status/file_spec.rb
index ae03aa9cb..5451913a1 100755
--- a/spec/unit/indirector/certificate_status/file_spec.rb
+++ b/spec/unit/indirector/certificate_status/file_spec.rb
@@ -7,6 +7,10 @@ require 'tempfile'
 describe "Puppet::Indirector::CertificateStatus::File" do
   include PuppetSpec::Files
 
+  before :all do
+    Puppet::SSL::Host.configure_indirection(:file)
+  end
+
   before do
     Puppet::SSL::CertificateAuthority.stubs(:ca?).returns true
     @terminus = Puppet::SSL::Host.indirection.terminus(:file)
-- 
cgit 


From 8820a78b5793ba6266b3974ac90a9405d73b8343 Mon Sep 17 00:00:00 2001
From: Nick Lewis <nick@puppetlabs.com>
Date: Wed, 20 Jul 2011 16:30:34 -0700
Subject: Replace calls to Array#count with #length

Array#count is not available in Ruby 1.8.5, so we need to use #length in these
specs for compatibility.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/face/ca_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/unit/face/ca_spec.rb b/spec/unit/face/ca_spec.rb
index 84769605f..b8c82ce99 100755
--- a/spec/unit/face/ca_spec.rb
+++ b/spec/unit/face/ca_spec.rb
@@ -171,7 +171,7 @@ describe Puppet::Face[:ca, '0.1.0'] do
       subject.sign('random-host').should be_an_instance_of Puppet::SSL::Certificate
 
       list = subject.list(:signed => true)
-      list.count.should == 1
+      list.length.should == 1
       list.first.name.should == 'random-host'
     end
   end
@@ -185,7 +185,7 @@ describe Puppet::Face[:ca, '0.1.0'] do
       subject.generate('random-host')
 
       list = subject.list(:signed => true)
-      list.count.should == 1
+      list.length.should == 1
       list.first.name.should == 'random-host'
     end
 
-- 
cgit 


From b75b1c19ecf6c278b065d203ac8486fa598caa8b Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Wed, 20 Jul 2011 11:58:55 -0700
Subject: (#6787) Add `default_to` for options.

This implement support for options with default values, allowing faces to set
those values when not invoked.  This can eliminate substantial duplicate code
from actions, especially when there are face-level options in use.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/interface/action.rb                     |   9 ++
 lib/puppet/interface/option.rb                     |  19 ++++
 lib/puppet/interface/option_builder.rb             |  13 +++
 .../things_that_declare_options.rb                 | 113 +++++++++++++++++++++
 spec/unit/interface/action_spec.rb                 |  45 ++++++++
 spec/unit/interface/option_spec.rb                 |  44 ++++++++
 6 files changed, 243 insertions(+)

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index fe77a9658..fc1121eb6 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -199,6 +199,7 @@ def #{@name}(#{decl.join(", ")})
   options = args.last
 
   action = get_action(#{name.inspect})
+  action.add_default_args(args)
   action.validate_args(args)
   __invoke_decorations(:before, action, args, options)
   rval = self.__send__(#{internal_name.inspect}, *args)
@@ -252,6 +253,14 @@ WRAPPER
     option
   end
 
+  def add_default_args(args)
+    options.map {|x| get_option(x) }.each do |option|
+      if option.has_default? and not option.aliases.any? {|x| args.last.has_key? x}
+        args.last[option.name] = option.default
+      end
+    end
+  end
+
   def validate_args(args)
     # Check for multiple aliases for the same option...
     args.last.keys.each do |name|
diff --git a/lib/puppet/interface/option.rb b/lib/puppet/interface/option.rb
index 3cd930acf..01f6f2307 100644
--- a/lib/puppet/interface/option.rb
+++ b/lib/puppet/interface/option.rb
@@ -6,6 +6,7 @@ class Puppet::Interface::Option
   def initialize(parent, *declaration, &block)
     @parent   = parent
     @optparse = []
+    @default  = nil
 
     # Collect and sort the arguments in the declaration.
     dups = {}
@@ -81,8 +82,26 @@ class Puppet::Interface::Option
     !!@required
   end
 
+  def has_default?
+    !!@default
+  end
+
+  def default=(proc)
+    required and raise ArgumentError, "#{self} can't be optional and have a default value"
+    proc.is_a? Proc or raise ArgumentError, "default value for #{self} is a #{proc.class.name.inspect}, not a proc"
+    @default = proc
+  end
+
+  def default
+    @default and @default.call
+  end
+
   attr_reader   :parent, :name, :aliases, :optparse
   attr_accessor :required
+  def required=(value)
+    has_default? and raise ArgumentError, "#{self} can't be optional and have a default value"
+    @required = value
+  end
 
   attr_accessor :before_action
   def before_action=(proc)
diff --git a/lib/puppet/interface/option_builder.rb b/lib/puppet/interface/option_builder.rb
index 5676ec977..c87adc2c0 100644
--- a/lib/puppet/interface/option_builder.rb
+++ b/lib/puppet/interface/option_builder.rb
@@ -51,4 +51,17 @@ class Puppet::Interface::OptionBuilder
   def required(value = true)
     @option.required = value
   end
+
+  def default_to(&block)
+    block or raise ArgumentError, "#{@option} default_to requires a block"
+    if @option.has_default?
+      raise ArgumentError, "#{@option} already has a default value"
+    end
+    # Ruby 1.8 treats a block without arguments as accepting any number; 1.9
+    # gets this right, so we work around it for now... --daniel 2011-07-20
+    unless block.arity == 0 or (RUBY_VERSION =~ /^1\.8/ and block.arity == -1)
+      raise ArgumentError, "#{@option} default_to block should not take any arguments"
+    end
+    @option.default = block
+  end
 end
diff --git a/spec/shared_behaviours/things_that_declare_options.rb b/spec/shared_behaviours/things_that_declare_options.rb
index 19bba66c1..ecdbfcaea 100755
--- a/spec/shared_behaviours/things_that_declare_options.rb
+++ b/spec/shared_behaviours/things_that_declare_options.rb
@@ -146,4 +146,117 @@ shared_examples_for "things that declare options" do
       end
     end
   end
+
+  describe "#default_to" do
+    it "should not have a default value by default" do
+      option = add_options_to do option "--foo" end.get_option(:foo)
+      option.should_not be_has_default
+    end
+
+    it "should accept a block for the default value" do
+      option = add_options_to do
+        option "--foo" do
+          default_to do
+            12
+          end
+        end
+      end.get_option(:foo)
+
+      option.should be_has_default
+    end
+
+    it "should invoke the block when asked for the default value" do
+      invoked = false
+      option = add_options_to do
+        option "--foo" do
+          default_to do
+            invoked = true
+          end
+        end
+      end.get_option(:foo)
+
+      option.should be_has_default
+      option.default.should be_true
+      invoked.should be_true
+    end
+
+    it "should return the value of the block when asked for the default" do
+      option = add_options_to do
+        option "--foo" do
+          default_to do
+            12
+          end
+        end
+      end.get_option(:foo)
+
+      option.should be_has_default
+      option.default.should == 12
+    end
+
+    it "should invoke the block every time the default is requested" do
+      option = add_options_to do
+        option "--foo" do
+          default_to do
+            {}
+          end
+        end
+      end.get_option(:foo)
+
+      first  = option.default.object_id
+      second = option.default.object_id
+      third  = option.default.object_id
+
+      first.should_not == second
+      first.should_not == third
+      second.should_not == third
+    end
+
+    it "should fail if the option has a default and is required" do
+      expect {
+        add_options_to do
+          option "--foo" do
+            required
+            default_to do 12 end
+          end
+        end
+      }.to raise_error ArgumentError, /can't be optional and have a default value/
+
+      expect {
+        add_options_to do
+          option "--foo" do
+            default_to do 12 end
+            required
+          end
+        end
+      }.to raise_error ArgumentError, /can't be optional and have a default value/
+    end
+
+    it "should fail if default_to has no block" do
+      expect { add_options_to do option "--foo" do default_to end end }.
+        to raise_error ArgumentError, /default_to requires a block/
+    end
+
+    it "should fail if default_to is invoked twice" do
+      expect {
+        add_options_to do
+          option "--foo" do
+            default_to do 12 end
+            default_to do "fun" end
+          end
+        end
+      }.to raise_error ArgumentError, /already has a default value/
+    end
+
+    [ "one", "one, two", "one, *two" ].each do |input|
+      it "should fail if the block has the wrong arity (#{input})" do
+        expect {
+          add_options_to do
+            option "--foo" do
+              eval "default_to do |#{input}| 12 end"
+            end
+          end
+        }.to raise_error ArgumentError, /should not take any arguments/
+      end
+    end
+  end
 end
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index 23216e7b4..dbbf84778 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -552,4 +552,49 @@ describe Puppet::Interface::Action do
         to raise_error ArgumentError, /Multiple aliases for the same option/
     end
   end
+
+  context "default option values" do
+    subject do
+      Puppet::Interface.new(:default_option_values, '1.0.0') do
+        action :foo do
+          option "--foo" do end
+          option "--bar" do end
+          when_invoked do |options| options end
+        end
+      end
+    end
+
+    let :action do subject.get_action :foo end
+    let :option do action.get_option :foo end
+
+    it "should not add options without defaults" do
+      subject.foo.should == {}
+    end
+
+    it "should not add options without defaults, if options are given" do
+      subject.foo(:bar => 1).should == { :bar => 1 }
+    end
+
+    it "should add the option default value when set" do
+      option.default = proc { 12 }
+      subject.foo.should == { :foo => 12 }
+    end
+
+    it "should add the option default value when set, if other options are given" do
+      option.default = proc { 12 }
+      subject.foo(:bar => 1).should == { :foo => 12, :bar => 1 }
+    end
+
+    it "should invoke the same default proc every time called" do
+      option.default = proc { @foo ||= {} }
+      subject.foo[:foo].object_id.should == subject.foo[:foo].object_id
+    end
+
+    [nil, 0, 1, true, false, {}, []].each do |input|
+      it "should not override a passed option (#{input.inspect})" do
+        option.default = proc { :fail }
+        subject.foo(:foo => input).should == { :foo => input }
+      end
+    end
+  end
 end
diff --git a/spec/unit/interface/option_spec.rb b/spec/unit/interface/option_spec.rb
index e77b46e79..e73561fba 100755
--- a/spec/unit/interface/option_spec.rb
+++ b/spec/unit/interface/option_spec.rb
@@ -97,4 +97,48 @@ describe Puppet::Interface::Option do
       end
     end
   end
+
+  context "defaults" do
+    subject { Puppet::Interface::Option.new(face, "--foo") }
+
+    it "should work sanely if member variables are used for state" do
+      subject.default = proc { @foo ||= 0; @foo += 1 }
+      subject.default.should == 1
+      subject.default.should == 2
+      subject.default.should == 3
+    end
+
+    context "with no default" do
+      it { should_not be_has_default }
+      its :default do should be_nil end
+
+      it "should set a proc as default" do
+        expect { subject.default = proc { 12 } }.should_not raise_error
+      end
+
+      [1, {}, [], Object.new, "foo"].each do |input|
+        it "should reject anything but a proc (#{input.class})" do
+          expect { subject.default = input }.to raise_error ArgumentError, /not a proc/
+        end
+      end
+    end
+
+    context "with a default" do
+      before :each do subject.default = proc { [:foo] } end
+
+      it { should be_has_default }
+      its :default do should == [:foo] end
+
+      it "should invoke the block every time" do
+        subject.default.object_id.should_not == subject.default.object_id
+        subject.default.should == subject.default
+      end
+
+      it "should allow replacing the default proc" do
+        subject.default.should == [:foo]
+        subject.default = proc { :bar }
+        subject.default.should == :bar
+      end
+    end
+  end
 end
-- 
cgit 


From fd6a653cb32cb03e339655862c526fd5dccbfcf0 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Wed, 20 Jul 2011 12:35:22 -0700
Subject: (#7123) Support runtime setting of 'default' on actions.

Given the inheritance model for actions, we are sometimes going to need to set
them to 'default' at runtime, rather than during their static declaration.

Add tests to verify that this works correctly, and update the code to ensure
that happens.  This gives up caching of the default action, but this should be
an extremely rare operation - pretty much only CLI invocation, really.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/interface/action_manager.rb | 15 ++++++++++-----
 spec/unit/interface/action_spec.rb     | 18 ++++++++++++++++++
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/lib/puppet/interface/action_manager.rb b/lib/puppet/interface/action_manager.rb
index fbf588d7d..5c9af4f96 100644
--- a/lib/puppet/interface/action_manager.rb
+++ b/lib/puppet/interface/action_manager.rb
@@ -7,13 +7,14 @@ module Puppet::Interface::ActionManager
     require 'puppet/interface/action_builder'
 
     @actions ||= {}
-    @default_action ||= nil
     raise "Action #{name} already defined for #{self}" if action?(name)
+
     action = Puppet::Interface::ActionBuilder.build(self, name, &block)
-    if action.default
-      raise "Actions #{@default_action.name} and #{name} cannot both be default" if @default_action
-      @default_action = action
+
+    if action.default and current = get_default_action
+      raise "Actions #{current.name} and #{name} cannot both be default"
     end
+
     @actions[action.name] = action
   end
 
@@ -61,7 +62,11 @@ module Puppet::Interface::ActionManager
   end
 
   def get_default_action
-    @default_action
+    default = actions.map {|x| get_action(x) }.select {|x| x.default }
+    if default.length > 1
+      raise "The actions #{default.map(&:name).join(", ")} cannot all be default"
+    end
+    default.first
   end
 
   def action?(name)
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index dbbf84778..d71a7d000 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -597,4 +597,22 @@ describe Puppet::Interface::Action do
       end
     end
   end
+
+  context "runtime manipulations" do
+    subject do
+      Puppet::Interface.new(:runtime_manipulations, '1.0.0') do
+        action :foo do
+          when_invoked do |options| options end
+        end
+      end
+    end
+
+    let :action do subject.get_action :foo end
+
+    it "should be the face default action if default is set true" do
+      subject.get_default_action.should be_nil
+      action.default = true
+      subject.get_default_action.should == action
+    end
+  end
 end
-- 
cgit 


From 395c1742981590c694ee6f0154ba7ad63c6bcb36 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Wed, 20 Jul 2011 12:37:53 -0700
Subject: (#7123) Make `find` the default action...

Part of the progress toward getting the `puppet status` invocation working
nicely is that it should default to invoking the `find` operation.  This
implements that, using the new runtime default action facility.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/face/status.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/puppet/face/status.rb b/lib/puppet/face/status.rb
index bdb0c4d26..e8c87e98d 100644
--- a/lib/puppet/face/status.rb
+++ b/lib/puppet/face/status.rb
@@ -12,6 +12,7 @@ Puppet::Indirector::Face.define(:status, '0.0.1') do
   get_action(:search).summary "Invalid for this subcommand."
 
   find = get_action(:find)
+  find.default = true
   find.summary "Check status of puppet master server."
   find.arguments "<dummy_text>"
   find.returns <<-'EOT'
-- 
cgit 


From e6398680e673e69240bacdf2f0262524bf9e964e Mon Sep 17 00:00:00 2001
From: Jacob Helwig <jacob@puppetlabs.com>
Date: Fri, 22 Jul 2011 11:38:14 -0700
Subject: Confine password disclosure acceptance test to hosts with required
 libraries

The useradd provider has the requirement that ruby-shadow[1] be
installed to be able to manage passwords.

On systems where we would use the useradd provider and this library
has not been installed we don't bother running the test, since we will
never be able to see the output we are testing.

[1] http://ttsky.net/ruby/

Signed-off-by: Jacob Helwig <jacob@puppetlabs.com>
Reviewed-by: Dominic Maraglia <dominic@puppetlabs.com>
Reviewed-by: Nick Lewis <nick@puppetlabs.com>
---
 ...password-disclosure-when-changing-a-users-password.rb | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/acceptance/tests/ticket_6857_password-disclosure-when-changing-a-users-password.rb b/acceptance/tests/ticket_6857_password-disclosure-when-changing-a-users-password.rb
index f1e100c2e..d3b415691 100644
--- a/acceptance/tests/ticket_6857_password-disclosure-when-changing-a-users-password.rb
+++ b/acceptance/tests/ticket_6857_password-disclosure-when-changing-a-users-password.rb
@@ -1,5 +1,15 @@
 test_name "#6857: redact password hashes when applying in noop mode"
 
+hosts_to_test = agents.reject do |agent|
+  if agent['platform'].match /(?:ubuntu|centos|debian|rhel)/
+    result = on(agent, %q{ruby -e 'require "shadow" or raise'}, :silent => true)
+    result.exit_code != 0
+  else
+    false
+  end
+end
+skip_test "No suitable hosts found" if hosts_to_test.empty?
+
 adduser_manifest = <<MANIFEST
 user { 'passwordtestuser':
   ensure   => 'present',
@@ -15,9 +25,9 @@ user { 'passwordtestuser':
 }
 MANIFEST
 
-apply_manifest_on(agents, adduser_manifest )
-results = apply_manifest_on(agents, changepass_manifest )
+apply_manifest_on(hosts_to_test, adduser_manifest )
+results = apply_manifest_on(hosts_to_test, changepass_manifest )
 
 results.each do |result|
-  assert_match( /current_value \[old password hash redacted\], should be \[new password hash redacted\]/ , "#{result.stdout}" )
+  assert_match( /current_value \[old password hash redacted\], should be \[new password hash redacted\]/ , "#{result.host}: #{result.stdout}" )
 end
-- 
cgit 


From 039661156116cb6d3c5c8e372ed4d81af221453b Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Thu, 21 Jul 2011 15:35:35 -0700
Subject: maint: better error reporting when test fails

This test ensures, among other things, that we get a log message.  If that
fails, we were trying to call a random method on nil; making that an assertion
means that we get a nice message rather than a failure that needs decoding.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 spec/unit/application/face_base_spec.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spec/unit/application/face_base_spec.rb b/spec/unit/application/face_base_spec.rb
index 0a4a86be6..bebc26210 100755
--- a/spec/unit/application/face_base_spec.rb
+++ b/spec/unit/application/face_base_spec.rb
@@ -55,6 +55,7 @@ describe Puppet::Application::FaceBase do
     it "should stop if the first thing found is not an action" do
       app.command_line.stubs(:args).returns %w{banana count_args}
       expect { app.run }.to exit_with 1
+      @logs.first.should_not be_nil
       @logs.first.message.should =~ /has no 'banana' action/
     end
 
-- 
cgit 


From 1e0655e6bdbc872014abdffa5deacb334616e826 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Thu, 21 Jul 2011 15:34:52 -0700
Subject: (#7184) Centralize "find action for face" into Puppet::Face

As part of moving to load actions first, and their associated face, when
invoked from the command line, it makes sense to push the logic for finding
the action and face down into the Puppet::Face implementation.

This means that we can change the logic there without needing to update the
public part of the CLI implementation, and that any further facades can use
the same, correct, logic to locate the action for the face.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/application/face_base.rb     |  3 ++-
 lib/puppet/interface.rb                 |  4 ++++
 lib/puppet/interface/action.rb          |  1 +
 lib/puppet/interface/face_collection.rb | 11 +++++++++++
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb
index ea5ba4aaf..a111518f1 100644
--- a/lib/puppet/application/face_base.rb
+++ b/lib/puppet/application/face_base.rb
@@ -100,7 +100,8 @@ class Puppet::Application::FaceBase < Puppet::Application
         # action object it represents; if this is an invalid action name that
         # will be nil, and handled later.
         action_name = item.to_sym
-        @action = @face.get_action(action_name)
+        @action = Puppet::Face.find_action(@face.name, action_name)
+        @face   = @action.face if @action
       end
     end
 
diff --git a/lib/puppet/interface.rb b/lib/puppet/interface.rb
index 6c288f3c0..eba99d6be 100644
--- a/lib/puppet/interface.rb
+++ b/lib/puppet/interface.rb
@@ -64,6 +64,10 @@ class Puppet::Interface
       end
       face
     end
+
+    def find_action(name, action, version = :current)
+      Puppet::Interface::FaceCollection.get_action_for_face(name, action, version)
+    end
   end
 
   def set_default_format(format)
diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index fc1121eb6..ce9c60b49 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -38,6 +38,7 @@ class Puppet::Interface::Action
   def to_s() "#{@face}##{@name}" end
 
   attr_reader   :name
+  attr_reader   :face
   attr_accessor :default
   def default?
     !!@default
diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index 4522824fd..ddc66f583 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -20,6 +20,17 @@ module Puppet::Interface::FaceCollection
     get_face(name, version) or load_face(name, version)
   end
 
+  def self.get_action_for_face(face_name, action_name, version)
+    # If the version they request specifically doesn't exist, don't search
+    # elsewhere.  Usually this will start from :current and all...
+    return nil unless face = self[face_name, version]
+    unless action = face.get_action(action_name)
+      # ...we need to search for it bound to an o{lder,ther} version.
+    end
+
+    return action
+  end
+
   # get face from memory, without loading.
   def self.get_face(name, pattern)
     return nil unless @faces.has_key? name
-- 
cgit 


From 2cd3bc47993fbd32a77ca9dfdd51353f2dfbcb58 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Thu, 21 Jul 2011 16:34:20 -0700
Subject: (#7184) Find actions bound to other versions of Faces.

When we first touch a Face, we load all the available Actions from disk.

Given they define themselves against a specific version of a Face, they are
automatically available tied to the correct version; this makes it trivially
possible to locate those on demand and return them.

Now, we have the ability to find and, consequently, invoke Actions on older
versions of Faces.  We don't load enough context, though: the older face will
only have external Actions defined, not anything core.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/interface/face_collection.rb     | 13 ++++++++++---
 spec/lib/puppet/face/huzzah/obsolete.rb     |  6 ++++++
 spec/unit/interface/face_collection_spec.rb | 15 +++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)
 create mode 100644 spec/lib/puppet/face/huzzah/obsolete.rb

diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index ddc66f583..868997b67 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -20,12 +20,19 @@ module Puppet::Interface::FaceCollection
     get_face(name, version) or load_face(name, version)
   end
 
-  def self.get_action_for_face(face_name, action_name, version)
+  def self.get_action_for_face(name, action_name, version)
+    name = underscorize(name)
+
     # If the version they request specifically doesn't exist, don't search
     # elsewhere.  Usually this will start from :current and all...
-    return nil unless face = self[face_name, version]
+    return nil unless face = self[name, version]
     unless action = face.get_action(action_name)
-      # ...we need to search for it bound to an o{lder,ther} version.
+      # ...we need to search for it bound to an o{lder,ther} version.  Since
+      # we load all actions when the face is first references, this will be in
+      # memory in the known set of versions of the face.
+      (@faces[name].keys - [ :current ]).sort.reverse.each do |version|
+        break if action = @faces[name][version].get_action(action_name)
+      end
     end
 
     return action
diff --git a/spec/lib/puppet/face/huzzah/obsolete.rb b/spec/lib/puppet/face/huzzah/obsolete.rb
new file mode 100644
index 000000000..1f717ea2f
--- /dev/null
+++ b/spec/lib/puppet/face/huzzah/obsolete.rb
@@ -0,0 +1,6 @@
+Puppet::Face.define(:huzzah, '1.0.0') do
+  action :obsolete do
+    summary "This is an action on version 1.0.0 of the face"
+    when_invoked do |options| options end
+  end
+end
diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index 98887a778..8f9c349b6 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -97,6 +97,21 @@ describe Puppet::Interface::FaceCollection do
     end
   end
 
+  describe "::get_action_for_face" do
+    it "should return an action on the current face" do
+      Puppet::Face::FaceCollection.get_action_for_face(:huzzah, :bar, :current).
+        should be_an_instance_of Puppet::Interface::Action
+    end
+
+    it "should return an action on an older version of a face" do
+      action = Puppet::Face::FaceCollection.
+        get_action_for_face(:huzzah, :obsolete, :current)
+
+      action.should be_an_instance_of Puppet::Interface::Action
+      action.face.version.should == SemVer.new('1.0.0')
+    end
+  end
+
   describe "::register" do
     it "should store the face by name" do
       face = Puppet::Face.new(:my_face, '0.0.1')
-- 
cgit 


From 532c4f37e4f8289cf4a9871ebc0cb5086c2ba26a Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Thu, 21 Jul 2011 16:43:16 -0700
Subject: (#7184) Load the core of obsolete versions of Faces.

When we define an action on an older version of a Face, we must be sure to
directly load the core of that version, not just define it with the external
Action(s) that it had.

Otherwise we break our contract, which is that any core Actions for a specific
version will be available to your external Action for as long as we support
that core version.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/interface/face_collection.rb     | 32 ++++++++++++++++++++---------
 spec/lib/puppet/face/1.0.0/huzzah.rb        |  9 ++++++++
 spec/lib/puppet/face/huzzah.rb              |  2 ++
 spec/unit/interface/face_collection_spec.rb | 27 ++++++++++++++++++++++--
 spec/unit/interface_spec.rb                 |  9 +++-----
 5 files changed, 61 insertions(+), 18 deletions(-)
 create mode 100755 spec/lib/puppet/face/1.0.0/huzzah.rb

diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index 868997b67..b1f6ba398 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -56,9 +56,7 @@ module Puppet::Interface::FaceCollection
     #
     # We use require to avoid executing the code multiple times, like any
     # other Ruby library that we might want to use.  --daniel 2011-04-06
-    begin
-      require "puppet/face/#{name}"
-
+    if safely_require name then
       # If we wanted :current, we need to index to find that; direct version
       # requests just work™ as they go. --daniel 2011-04-06
       if version == :current then
@@ -90,18 +88,32 @@ module Puppet::Interface::FaceCollection
         latest_ver = @faces[name].keys.sort.last
         @faces[name][:current] = @faces[name][latest_ver]
       end
-    rescue LoadError => e
-      raise unless e.message =~ %r{-- puppet/face/#{name}$}
-      # ...guess we didn't find the file; return a much better problem.
-    rescue SyntaxError => e
-      raise unless e.message =~ %r{puppet/face/#{name}\.rb:\d+: }
-      Puppet.err "Failed to load face #{name}:\n#{e}"
-      # ...but we just carry on after complaining.
+    end
+
+    unless version == :current or get_face(name, version)
+      # Try an obsolete version of the face, if needed, to see if that helps?
+      safely_require name, version
     end
 
     return get_face(name, version)
   end
 
+  def self.safely_require(name, version = nil)
+    path = File.join 'puppet' ,'face', version.to_s, name.to_s
+    require path
+    true
+
+  rescue LoadError => e
+    raise unless e.message =~ %r{-- #{path}$}
+    # ...guess we didn't find the file; return a much better problem.
+    nil
+  rescue SyntaxError => e
+    raise unless e.message =~ %r{#{path}\.rb:\d+: }
+    Puppet.err "Failed to load face #{name}:\n#{e}"
+    # ...but we just carry on after complaining.
+    nil
+  end
+
   def self.register(face)
     @faces[underscorize(face.name)][face.version] = face
   end
diff --git a/spec/lib/puppet/face/1.0.0/huzzah.rb b/spec/lib/puppet/face/1.0.0/huzzah.rb
new file mode 100755
index 000000000..00f20f096
--- /dev/null
+++ b/spec/lib/puppet/face/1.0.0/huzzah.rb
@@ -0,0 +1,9 @@
+require 'puppet/face'
+Puppet::Face.define(:huzzah, '1.0.0') do
+  copyright "Puppet Labs", 2011
+  license   "Apache 2 license; see COPYING"
+  summary "life is a thing for celebration"
+  script :obsolete_in_core do |_| "you are in obsolete core now!" end
+
+  script :call_newer do method_on_newer end
+end
diff --git a/spec/lib/puppet/face/huzzah.rb b/spec/lib/puppet/face/huzzah.rb
index ab465d9e0..47cc3f32c 100755
--- a/spec/lib/puppet/face/huzzah.rb
+++ b/spec/lib/puppet/face/huzzah.rb
@@ -4,4 +4,6 @@ Puppet::Face.define(:huzzah, '2.0.1') do
   license   "Apache 2 license; see COPYING"
   summary "life is a thing for celebration"
   script :bar do |options| "is where beer comes from" end
+
+  script :call_older do method_on_older end
 end
diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index 8f9c349b6..514a624b1 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -35,7 +35,8 @@ describe Puppet::Interface::FaceCollection do
     end
 
     it "should attempt to load the face if it isn't found" do
-      subject.expects(:require).with('puppet/face/bar')
+      subject.expects(:require).once.with('puppet/face/bar')
+      subject.expects(:require).once.with('puppet/face/0.0.1/bar')
       subject["bar", '0.0.1']
     end
 
@@ -64,7 +65,8 @@ describe Puppet::Interface::FaceCollection do
 
     it "should return false if the face file itself is missing" do
       subject.stubs(:require).
-        raises(LoadError, 'no such file to load -- puppet/face/bar')
+        raises(LoadError, 'no such file to load -- puppet/face/bar').then.
+        raises(LoadError, 'no such file to load -- puppet/face/0.0.1/bar')
       subject["bar", '0.0.1'].should be_false
     end
 
@@ -110,6 +112,27 @@ describe Puppet::Interface::FaceCollection do
       action.should be_an_instance_of Puppet::Interface::Action
       action.face.version.should == SemVer.new('1.0.0')
     end
+
+    it "should load the full older version of a face" do
+      action = Puppet::Face::FaceCollection.
+        get_action_for_face(:huzzah, :obsolete, :current)
+
+      action.face.version.should == SemVer.new('1.0.0')
+      action.face.should be_action :obsolete_in_core
+    end
+
+    it "should not add obsolete actions to the current version" do
+      action = Puppet::Face::FaceCollection.
+        get_action_for_face(:huzzah, :obsolete, :current)
+
+      action.face.version.should == SemVer.new('1.0.0')
+      action.face.should be_action :obsolete_in_core
+
+      current = Puppet::Face[:huzzah, :current]
+      current.version.should == SemVer.new('2.0.1')
+      current.should_not be_action :obsolete_in_core
+      current.should_not be_action :obsolete
+    end
   end
 
   describe "::register" do
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index 4cb1f8743..4ff71ac3d 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -126,14 +126,11 @@ describe Puppet::Interface do
   end
 
   it "should try to require faces that are not known" do
-    pending "mocking require causes random stack overflow"
-    subject::FaceCollection.expects(:require).with "puppet/face/foo"
-    subject[:foo, '0.0.1']
+    subject::FaceCollection.expects(:load_face).with(:foo, :current)
+    subject::FaceCollection.expects(:load_face).with(:foo, '0.0.1')
+    expect { subject[:foo, '0.0.1'] }.to raise_error Puppet::Error
   end
 
-  it "should be able to load all actions in all search paths"
-
-
   it_should_behave_like "things that declare options" do
     def add_options_to(&block)
       subject.new(:with_options, '0.0.1', &block)
-- 
cgit 


From 6bec2df0860b71730b99ddd9f1764aa2ce0dc960 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Fri, 22 Jul 2011 14:11:35 -0700
Subject: (#8561) Use canonical names for options to actions.

When we invoke an action, we parse a set of options.  These have a canonical
name, and optionally a set of aliases.  For example, :bar might have :b as an
alias to allow a short name to be given.

Previously we would just pass this on as received; if you passed :bar you got
:bar, and if you passed :b you got :b.  This works, but means that every
action has to write the same code to extract the appropriate version of an
option from whatever set of aliases might be passed.

Now, instead, we centralize that and always pass options as their canonical
name to the action code.  This makes it simpler to work with.  (This happens
before any validation, or other user-supplied, code to simplify everything
that handles options.)

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 spec/unit/interface/action_spec.rb | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index d71a7d000..848a44bba 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -171,14 +171,28 @@ describe Puppet::Interface::Action do
       face.get_action(:foo).options.should =~ [:bar]
     end
 
-    it "should only list options and not aliases" do
-      face = Puppet::Interface.new(:action_level_options, '0.0.1') do
-        action :foo do
-          when_invoked do |options| true end
-          option "--bar", "-b", "--foo-bar"
+    describe "option aliases" do
+      let :option do action.get_option :bar end
+      let :action do face.get_action :foo end
+      let :face do
+        Puppet::Interface.new(:action_level_options, '0.0.1') do
+          action :foo do
+            when_invoked do |options| options end
+            option "--bar", "--foo", "-b"
+          end
+        end
+      end
+
+      it "should only list options and not aliases" do
+        action.options.should =~ [:bar]
+      end
+
+      it "should use the canonical option name when passed aliases" do
+        name = option.name
+        option.aliases.each do |input|
+          face.foo(input => 1).should == { name => 1 }
         end
       end
-      face.get_action(:foo).options.should =~ [:bar]
     end
 
     describe "with both face and action options" do
-- 
cgit 


From 69b4e70ac2fa2b265f6c92d7de073d51ae25d3ed Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Fri, 22 Jul 2011 14:13:50 -0700
Subject: (#7290) Fail on unknown options.

Part of the "social contract" of Faces, Actions and Options is that the
metadata we collect is authoritative: it covers everything that is possible.

In the initial release we didn't enforce that around options.  If you passed
an unknown option in the hash, we just silently ignored it in validation and
made it available down in the action.

Now, instead, we enforce that rule.  If you pass an unknown option we raise an
error and complain; anything that gets to the action will be listed in the set
of inspectable options.

Cases that depended on this behaviour to pass arbitrary content in the hash
should be rewritten to move that content down a level: take a hash value for
one option, and use that for your free content.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 spec/unit/interface/action_spec.rb | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index 848a44bba..66fff2571 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -565,6 +565,16 @@ describe Puppet::Interface::Action do
       expect { subject.test :foo => true, :f => true }.
         to raise_error ArgumentError, /Multiple aliases for the same option/
     end
+
+    it "should fail if an unknown option is passed" do
+      expect { subject.test :unknown => true }.
+        to raise_error ArgumentError, /Unknown options passed: unknown/
+    end
+
+    it "should report all the unknown options passed" do
+      expect { subject.test :unknown => true, :unseen => false }.
+        to raise_error ArgumentError, /Unknown options passed: unknown, unseen/
+    end
   end
 
   context "default option values" do
-- 
cgit 


From 77441be2299bbb96ab9f048855b0fd4c16eb7b5a Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Fri, 22 Jul 2011 14:32:00 -0700
Subject: (#8561) Unify validation and modification of action arguments.

Rather than having multiple, separate operations that modify and validate the
arguments to an action, a single pass makes sense.  This also means less walks
across the set of data, and a few less expensive method calls in Ruby.

Additionally, we work on a duplicate of the arguments hash rather than
directly modifying the original.  Because everything we do is at the top level
key/value mapping, this is sufficient to isolate the original.

While mostly theoretical, we now don't mutilate the hash passed in, so the
user won't get nastily surprised by the fact that we could have done so.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/interface/action.rb     | 21 ++++++++++++++-------
 spec/unit/interface/action_spec.rb |  6 +++---
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index ce9c60b49..2a2f48e03 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -200,8 +200,7 @@ def #{@name}(#{decl.join(", ")})
   options = args.last
 
   action = get_action(#{name.inspect})
-  action.add_default_args(args)
-  action.validate_args(args)
+  args   = action.validate_and_clean(args)
   __invoke_decorations(:before, action, args, options)
   rval = self.__send__(#{internal_name.inspect}, *args)
   __invoke_decorations(:after, action, args, options)
@@ -254,15 +253,19 @@ WRAPPER
     option
   end
 
-  def add_default_args(args)
+  def validate_and_clean(original_args)
+    # Make a shallow copy, so as not to surprise callers.  We only modify the
+    # top level keys and all, so this should be sufficient without being too
+    # costly overall.
+    args = original_args.dup
+
+    # Inject default arguments.
     options.map {|x| get_option(x) }.each do |option|
       if option.has_default? and not option.aliases.any? {|x| args.last.has_key? x}
         args.last[option.name] = option.default
       end
     end
-  end
 
-  def validate_args(args)
     # Check for multiple aliases for the same option...
     args.last.keys.each do |name|
       # #7290: If this isn't actually an option, ignore it for now.  We should
@@ -281,8 +284,12 @@ WRAPPER
       get_option(name)
     end.select(&:required?).collect(&:name) - args.last.keys
 
-    return if required.empty?
-    raise ArgumentError, "The following options are required: #{required.join(', ')}"
+    unless required.empty?
+      raise ArgumentError, "The following options are required: #{required.join(', ')}"
+    end
+
+    # All done.
+    return args
   end
 
   ########################################################################
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index 66fff2571..c1491488e 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -138,8 +138,8 @@ describe Puppet::Interface::Action do
         options.should == { :bar => "beer" }
       end
 
-      it "should call #validate_args on the action when invoked" do
-        face.get_action(:bar).expects(:validate_args).with([1, :two, 'three', {}])
+      it "should call #validate_and_clean on the action when invoked" do
+        face.get_action(:bar).expects(:validate_and_clean).with([1, :two, 'three', {}])
         face.bar 1, :two, 'three'
       end
     end
@@ -548,7 +548,7 @@ describe Puppet::Interface::Action do
     it "should return the block if asked"
   end
 
-  context "#validate_args" do
+  context "#validate_and_clean" do
     subject do
       Puppet::Interface.new(:validate_args, '1.0.0') do
         script :test do |options| true end
-- 
cgit 


From 82e5fa9561e2d4cb1d699a41c14f50953d8f2d97 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Fri, 22 Jul 2011 15:15:38 -0700
Subject: (#8561, #7290) Implement the option contract fully.

Rewrite the process of validating and updating the options to fully reflect
the contract - we fail if there are unknown options passed, report nicely
errors of duplicate names, pass only the canonical names to the action code,
and generally enforce nicely.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/interface/action.rb     | 84 ++++++++++++++++++++++----------------
 spec/unit/interface/action_spec.rb | 11 ++---
 2 files changed, 55 insertions(+), 40 deletions(-)

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index 2a2f48e03..bd47a36ea 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -196,14 +196,12 @@ class Puppet::Interface::Action
     wrapper = <<WRAPPER
 def #{@name}(#{decl.join(", ")})
   #{optn}
-  args = #{args}
-  options = args.last
-
-  action = get_action(#{name.inspect})
-  args   = action.validate_and_clean(args)
-  __invoke_decorations(:before, action, args, options)
+  args    = #{args}
+  action  = get_action(#{name.inspect})
+  args   << action.validate_and_clean(args.pop)
+  __invoke_decorations(:before, action, args, args.last)
   rval = self.__send__(#{internal_name.inspect}, *args)
-  __invoke_decorations(:after, action, args, options)
+  __invoke_decorations(:after, action, args, args.last)
   return rval
 end
 WRAPPER
@@ -253,43 +251,59 @@ WRAPPER
     option
   end
 
-  def validate_and_clean(original_args)
-    # Make a shallow copy, so as not to surprise callers.  We only modify the
-    # top level keys and all, so this should be sufficient without being too
-    # costly overall.
-    args = original_args.dup
-
-    # Inject default arguments.
-    options.map {|x| get_option(x) }.each do |option|
-      if option.has_default? and not option.aliases.any? {|x| args.last.has_key? x}
-        args.last[option.name] = option.default
-      end
-    end
-
-    # Check for multiple aliases for the same option...
-    args.last.keys.each do |name|
-      # #7290: If this isn't actually an option, ignore it for now.  We should
-      # probably fail, but that wasn't our API, and I don't want to perturb
-      # behaviour this late in the RC cycle. --daniel 2011-04-29
+  def validate_and_clean(original)
+    # The final set of arguments; effectively a hand-rolled shallow copy of
+    # the original, which protects the caller from the surprises they might
+    # get if they passed us a hash and we mutated it...
+    result = {}
+
+    # Check for multiple aliases for the same option, and canonicalize the
+    # name of the argument while we are about it.
+    overlap = Hash.new do |h, k| h[k] = [] end
+    unknown = []
+    original.keys.each do |name|
       if option = get_option(name) then
-        overlap = (option.aliases & args.last.keys)
-        unless overlap.length == 1 then
-          raise ArgumentError, "Multiple aliases for the same option passed: #{overlap.join(', ')}"
+        canonical = option.name
+        if result.has_key? canonical
+          overlap[canonical] << name
+        else
+          result[canonical] = original[name]
         end
+      else
+        unknown << name
       end
     end
 
-    # Check for missing mandatory options.
-    required = options.map do |name|
-      get_option(name)
-    end.select(&:required?).collect(&:name) - args.last.keys
+    unless overlap.empty?
+      msg = overlap.map {|k, v| "(#{k}, #{v.sort.join(', ')})" }.join(", ")
+      raise ArgumentError, "Multiple aliases for the same option passed: #{msg}"
+    end
+
+    unless unknown.empty?
+      msg = unknown.sort.join(", ")
+      raise ArgumentError, "Unknown options passed: #{msg}"
+    end
+
+    # Inject default arguments and check for missing mandating options.
+    missing = []
+    options.map {|x| get_option(x) }.each do |option|
+      name = option.name
+      next if result.has_key? name
+
+      if option.has_default?
+        result[name] = option.default
+      elsif option.required?
+        missing << name
+      end
+    end
 
-    unless required.empty?
-      raise ArgumentError, "The following options are required: #{required.join(', ')}"
+    unless missing.empty?
+      msg = missing.sort.join(', ')
+      raise ArgumentError, "The following options are required: #{msg}"
     end
 
     # All done.
-    return args
+    return result
   end
 
   ########################################################################
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index c1491488e..c3f08e817 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -121,6 +121,7 @@ describe Puppet::Interface::Action do
       let :face do
         Puppet::Interface.new(:ruby_api, '1.0.0') do
           action :bar do
+            option "--bar"
             when_invoked do |*args|
               args.last
             end
@@ -139,7 +140,7 @@ describe Puppet::Interface::Action do
       end
 
       it "should call #validate_and_clean on the action when invoked" do
-        face.get_action(:bar).expects(:validate_and_clean).with([1, :two, 'three', {}])
+        face.get_action(:bar).expects(:validate_and_clean).with({}).returns({})
         face.bar 1, :two, 'three'
       end
     end
@@ -450,12 +451,12 @@ describe Puppet::Interface::Action do
         end
 
         it "should be invoked when calling a child action" do
-          subject.on_child(:foo => true, :bar => true).should == :on_child
+          subject.on_child(:foo => true).should == :on_child
           subject.reported.should == [ :child_before ]
         end
 
         it "should be invoked when calling a parent action" do
-          subject.on_parent(:foo => true, :bar => true).should == :on_parent
+          subject.on_parent(:foo => true).should == :on_parent
           subject.reported.should == [ :child_before ]
         end
       end
@@ -467,12 +468,12 @@ describe Puppet::Interface::Action do
         end
 
         it "should be invoked when calling a child action" do
-          subject.on_child(:foo => true, :bar => true).should == :on_child
+          subject.on_child(:foo => true).should == :on_child
           subject.reported.should == [ :parent_before ]
         end
 
         it "should be invoked when calling a parent action" do
-          subject.on_parent(:foo => true, :bar => true).should == :on_parent
+          subject.on_parent(:foo => true).should == :on_parent
           subject.reported.should == [ :parent_before ]
         end
       end
-- 
cgit 


From 88e9cd2138e9f23e056bd3331498a6d4723a8b0c Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Fri, 22 Jul 2011 15:24:16 -0700
Subject: maint: don't print inside action implementations.

Rather than printing directly, we should return the data from the Action and
allow the facade to route that to the user directly.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/indirector/face.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/puppet/indirector/face.rb b/lib/puppet/indirector/face.rb
index ead3f4b46..b4cac5c51 100644
--- a/lib/puppet/indirector/face.rb
+++ b/lib/puppet/indirector/face.rb
@@ -86,11 +86,11 @@ class Puppet::Indirector::Face < Puppet::Face
       run mode with the '--mode' option.
     EOT
 
-    when_invoked do |*args|
+    when_invoked do |options|
       if t = indirection.terminus_class
-        puts "Run mode '#{Puppet.run_mode.name}': #{t}"
+        "Run mode '#{Puppet.run_mode.name}': #{t}"
       else
-        $stderr.puts "No default terminus class for run mode '#{Puppet.run_mode.name}'"
+        "No default terminus class for run mode '#{Puppet.run_mode.name}'"
       end
     end
   end
-- 
cgit 


From ae360034c07f9b16467f5ae245ac6d037789ee13 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Fri, 22 Jul 2011 16:26:10 -0700
Subject: (#7290) Update indirected Faces to avoid unknown options.

Now that we enforce that options must be declared, the model we exposed in the
Ruby API (but not the CLI facade) was that you could pass additional arguments
to the indirection method by passing them as unknown options doesn't work.

Instead, explicitly declare an option, `extra`, that accepts the final
argument to be passed direct to the indirection.  This makes things work
smoothly, as well as making it possible (once you can input a hash on the
command line) to invoke extra arguments from the facade too...

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/indirector/face.rb     | 18 ++++++++++++++----
 spec/unit/indirector/face_spec.rb |  4 +++-
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/lib/puppet/indirector/face.rb b/lib/puppet/indirector/face.rb
index b4cac5c51..adb6b688b 100644
--- a/lib/puppet/indirector/face.rb
+++ b/lib/puppet/indirector/face.rb
@@ -48,16 +48,26 @@ class Puppet::Indirector::Face < Puppet::Face
     return result
   end
 
+  option "--extra HASH" do
+    summary "Extra arguments to pass to the indirection request"
+    description <<-end
+      A terminus can take additional arguments to refine the operation, which
+      are passed as an arbitrary hash to the back-end.  Anything passed as
+      the extra value is just send direct to the back-end.
+    end
+    default_to do Hash.new end
+  end
+
   action :destroy do
     summary "Delete an object."
     arguments "<key>"
-    when_invoked { |key, options| call_indirection_method(:destroy, key, options) }
+    when_invoked {|key, options| call_indirection_method :destroy, key, options[:extra] }
   end
 
   action :find do
     summary "Retrieve an object by name."
     arguments "<key>"
-    when_invoked { |key, options| call_indirection_method(:find, key, options) }
+    when_invoked {|key, options| call_indirection_method :find, key, options[:extra] }
   end
 
   action :save do
@@ -68,13 +78,13 @@ class Puppet::Indirector::Face < Puppet::Face
       currently accept data from STDIN, save actions cannot currently be invoked
       from the command line.
     EOT
-    when_invoked { |key, options| call_indirection_method(:save, key, options) }
+    when_invoked {|key, options| call_indirection_method :save, key, options[:extra] }
   end
 
   action :search do
     summary "Search for an object or retrieve multiple objects."
     arguments "<query>"
-    when_invoked { |key, options| call_indirection_method(:search, key, options) }
+    when_invoked {|key, options| call_indirection_method :search, key, options[:extra] }
   end
 
   # Print the configuration for the current terminus class
diff --git a/spec/unit/indirector/face_spec.rb b/spec/unit/indirector/face_spec.rb
index 943ff7991..8e324f019 100755
--- a/spec/unit/indirector/face_spec.rb
+++ b/spec/unit/indirector/face_spec.rb
@@ -12,6 +12,8 @@ describe Puppet::Indirector::Face do
     instance
   end
 
+  it { should be_option :extra }
+
   it "should be able to return a list of indirections" do
     Puppet::Indirector::Face.indirections.should be_include("catalog")
   end
@@ -48,7 +50,7 @@ describe Puppet::Indirector::Face do
     end
     it "should forward passed options" do
       subject.indirection.expects(method).with(:test, {'one'=>'1'})
-      subject.send(method, :test, {'one'=>'1'})
+      subject.send(method, :test, :extra => {'one'=>'1'})
     end
   end
 
-- 
cgit 


From d522b0b824285d025884ff271cda957df1c9951d Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 25 Jul 2011 11:06:58 -0700
Subject: maint: Fix Face testing bug 1.9.2 revealed.

Ruby 1.8.7 is fairly lax about various bits of introspection, including that
we can't tell much about what arguments a block takes.  Ruby 1.9.2 makes it
possible to do this, though.

Meanwhile, the Faces system uses this to make sure that scripts and actions
take the right set of arguments, to avoid surprises: failing early and
explicitly is better than failing at runtime.

Which, in final turn, exposes that I forgot to accept the right arguments in a
couple of my testing actions for Faces, but didn't notice because 1.8.7
doesn't check that, and I didn't test on 1.9.2.
---
 spec/lib/puppet/face/1.0.0/huzzah.rb | 3 +--
 spec/lib/puppet/face/huzzah.rb       | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/spec/lib/puppet/face/1.0.0/huzzah.rb b/spec/lib/puppet/face/1.0.0/huzzah.rb
index 00f20f096..8a1311704 100755
--- a/spec/lib/puppet/face/1.0.0/huzzah.rb
+++ b/spec/lib/puppet/face/1.0.0/huzzah.rb
@@ -4,6 +4,5 @@ Puppet::Face.define(:huzzah, '1.0.0') do
   license   "Apache 2 license; see COPYING"
   summary "life is a thing for celebration"
   script :obsolete_in_core do |_| "you are in obsolete core now!" end
-
-  script :call_newer do method_on_newer end
+  script :call_newer do |_| method_on_newer end
 end
diff --git a/spec/lib/puppet/face/huzzah.rb b/spec/lib/puppet/face/huzzah.rb
index 47cc3f32c..f3d18a797 100755
--- a/spec/lib/puppet/face/huzzah.rb
+++ b/spec/lib/puppet/face/huzzah.rb
@@ -4,6 +4,5 @@ Puppet::Face.define(:huzzah, '2.0.1') do
   license   "Apache 2 license; see COPYING"
   summary "life is a thing for celebration"
   script :bar do |options| "is where beer comes from" end
-
-  script :call_older do method_on_older end
+  script :call_older do |_| method_on_older end
 end
-- 
cgit 


From 10e05ad302d1b2d93f5da070d612817729473009 Mon Sep 17 00:00:00 2001
From: Pieter van de Bruggen <pieter@puppetlabs.com>
Date: Mon, 25 Jul 2011 12:05:31 -0700
Subject: (#7266) Move Certificate option validation into face.

The validation for the ca_location option on the certificate
application continued to hang around on the application long
after the face realized its potential to take responsibility
for itself.  This change moves (and adds) validation code as
appropriate into the Face.

Reviewed-By: Matt Robinson
---
 lib/puppet/application/certificate.rb |  5 -----
 lib/puppet/face/certificate.rb        | 22 +++++++++++++---------
 spec/unit/face/certificate_spec.rb    | 20 ++++++++++++++++----
 3 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/lib/puppet/application/certificate.rb b/lib/puppet/application/certificate.rb
index eacb830b2..de5b2c499 100644
--- a/lib/puppet/application/certificate.rb
+++ b/lib/puppet/application/certificate.rb
@@ -2,11 +2,6 @@ require 'puppet/application/indirection_base'
 
 class Puppet::Application::Certificate < Puppet::Application::IndirectionBase
   def setup
-    unless options[:ca_location]
-      raise ArgumentError, "You must have a CA location specified;\n" +
-        "use --ca-location to specify the location (remote, local, only)"
-    end
-
     location = Puppet::SSL::Host.ca_location
     if location == :local && !Puppet::SSL::CertificateAuthority.ca?
       self.class.run_mode("master")
diff --git a/lib/puppet/face/certificate.rb b/lib/puppet/face/certificate.rb
index 9a306da37..5e176e27e 100644
--- a/lib/puppet/face/certificate.rb
+++ b/lib/puppet/face/certificate.rb
@@ -6,7 +6,7 @@ Puppet::Indirector::Face.define(:certificate, '0.0.1') do
   license   "Apache 2 license; see COPYING"
 
   summary "Provide access to the CA for certificate management."
-  description <<-'EOT'
+  description <<-EOT
     This subcommand interacts with a local or remote Puppet certificate
     authority. Currently, its behavior is not a full superset of `puppet
     cert`; specifically, it is unable to mimic puppet cert's "clean" option,
@@ -15,8 +15,9 @@ Puppet::Indirector::Face.define(:certificate, '0.0.1') do
   EOT
 
   option "--ca-location LOCATION" do
+    required
     summary "Which certificate authority to use (local or remote)."
-    description <<-'EOT'
+    description <<-EOT
       Whether to act on the local certificate authority or one provided by a
       remote puppet master. Allowed values are 'local' and 'remote.'
 
@@ -24,6 +25,9 @@ Puppet::Indirector::Face.define(:certificate, '0.0.1') do
     EOT
 
     before_action do |action, args, options|
+      unless [:remote, :local, :only].include? options[:ca_location].to_sym
+        raise ArgumentError, "Valid values for ca-location are 'remote', 'local', 'only'."
+      end
       Puppet::SSL::Host.ca_location = options[:ca_location].to_sym
     end
   end
@@ -32,7 +36,7 @@ Puppet::Indirector::Face.define(:certificate, '0.0.1') do
     summary "Generate a new certificate signing request."
     arguments "<host>"
     returns "Nothing."
-    description <<-'EOT'
+    description <<-EOT
       Generates and submits a certificate signing request (CSR) for the
       specified host. This CSR will then have to be signed by a user
       with the proper authorization on the certificate authority.
@@ -41,7 +45,7 @@ Puppet::Indirector::Face.define(:certificate, '0.0.1') do
       primarily useful for requesting certificates for individual users and
       external applications.
     EOT
-    examples <<-'EOT'
+    examples <<-EOT
       Request a certificate for "somenode" from the site's CA:
 
       $ puppet certificate generate somenode.puppetlabs.lan --ca-location remote
@@ -56,7 +60,7 @@ Puppet::Indirector::Face.define(:certificate, '0.0.1') do
 
   action :list do
     summary "List all certificate signing requests."
-    returns <<-'EOT'
+    returns <<-EOT
       An array of #inspect output from CSR objects. This output is
       currently messy, but does contain the names of nodes requesting
       certificates. This action returns #inspect strings even when used
@@ -73,10 +77,10 @@ Puppet::Indirector::Face.define(:certificate, '0.0.1') do
   action :sign do
     summary "Sign a certificate signing request for HOST."
     arguments "<host>"
-    returns <<-'EOT'
+    returns <<-EOT
       A string that appears to be (but isn't) an x509 certificate.
     EOT
-    examples <<-'EOT'
+    examples <<-EOT
       Sign somenode.puppetlabs.lan's certificate:
 
       $ puppet certificate sign somenode.puppetlabs.lan --ca-location remote
@@ -93,7 +97,7 @@ Puppet::Indirector::Face.define(:certificate, '0.0.1') do
   find = get_action(:find)
   find.summary "Retrieve a certificate."
   find.arguments "<host>"
-  find.returns <<-'EOT'
+  find.returns <<-EOT
     An x509 SSL certificate. You will usually want to render this as a
     string (--render-as s).
 
@@ -105,7 +109,7 @@ Puppet::Indirector::Face.define(:certificate, '0.0.1') do
   destroy.summary "Delete a certificate."
   destroy.arguments "<host>"
   destroy.returns "Nothing."
-  destroy.description <<-'EOT'
+  destroy.description <<-EOT
     Deletes a certificate. This action currently only works on the local CA.
   EOT
 
diff --git a/spec/unit/face/certificate_spec.rb b/spec/unit/face/certificate_spec.rb
index 0cb905b75..9291d7523 100755
--- a/spec/unit/face/certificate_spec.rb
+++ b/spec/unit/face/certificate_spec.rb
@@ -10,14 +10,26 @@ describe Puppet::Face[:certificate, '0.0.1'] do
   end
 
   it "should set the ca location when invoked" do
-    Puppet::SSL::Host.expects(:ca_location=).with(:foo)
+    Puppet::SSL::Host.expects(:ca_location=).with(:local)
     Puppet::SSL::Host.indirection.expects(:save)
-    subject.sign "hello, friend", :ca_location => :foo
+    subject.sign "hello, friend", :ca_location => :local
   end
 
   it "(#7059) should set the ca location when an inherited action is invoked" do
-    Puppet::SSL::Host.expects(:ca_location=).with(:foo)
+    Puppet::SSL::Host.expects(:ca_location=).with(:local)
     subject.indirection.expects(:find)
-    subject.find "hello, friend", :ca_location => :foo
+    subject.find "hello, friend", :ca_location => :local
+  end
+
+  it "should validate the option as required" do
+    expect do
+      subject.find 'hello, friend'
+    end.to raise_exception ArgumentError, /required/i
+  end
+
+  it "should validate the option as a supported value" do
+    expect do
+      subject.find 'hello, friend', :ca_location => :foo
+    end.to raise_exception ArgumentError, /valid values/i
   end
 end
-- 
cgit 


From f484851b7528c0fcf1254d25a022e9cb7ef9b3bd Mon Sep 17 00:00:00 2001
From: Matt Robinson <matt@puppetlabs.com>
Date: Tue, 19 Jul 2011 14:11:37 -0700
Subject: maint: Add debug logging when the master receives a report

It's always bothered me that when running puppet inspect (or any
application that produces a report really) the master gives no
indication that anything happened when it processes the report.

Reviewed-by: Max Martin <max@puppetlabs.com>
---
 lib/puppet/indirector/report/processor.rb     |  2 ++
 spec/unit/indirector/report/processor_spec.rb | 12 +++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/lib/puppet/indirector/report/processor.rb b/lib/puppet/indirector/report/processor.rb
index 88fe4b487..81b379eb8 100644
--- a/lib/puppet/indirector/report/processor.rb
+++ b/lib/puppet/indirector/report/processor.rb
@@ -20,9 +20,11 @@ class Puppet::Transaction::Report::Processor < Puppet::Indirector::Code
   # LAK:NOTE This isn't necessarily the best design, but it's backward
   # compatible and that's good enough for now.
   def process(report)
+    Puppet.debug "Recieved report to process from #{report.host}"
     return if Puppet[:reports] == "none"
 
     reports.each do |name|
+      Puppet.debug "Processing report from #{report.host} with processor #{name}"
       if mod = Puppet::Reports.report(name)
         # We have to use a dup because we're including a module in the
         # report.
diff --git a/spec/unit/indirector/report/processor_spec.rb b/spec/unit/indirector/report/processor_spec.rb
index 5602a271f..ac3162886 100755
--- a/spec/unit/indirector/report/processor_spec.rb
+++ b/spec/unit/indirector/report/processor_spec.rb
@@ -25,9 +25,11 @@ describe Puppet::Transaction::Report::Processor, " when saving a report" do
 
   it "should not process the report if reports are set to 'none'" do
     Puppet::Reports.expects(:report).never
-    Puppet.settings.expects(:value).with(:reports).returns("none")
+    Puppet[:reports] = 'none'
 
-    request = stub 'request', :instance => mock("report")
+    request = Puppet::Indirector::Request.new(:indirection_name, :head, "key")
+    report = Puppet::Transaction::Report.new('apply')
+    request.instance = report
 
     @reporter.save(request)
   end
@@ -40,14 +42,14 @@ end
 
 describe Puppet::Transaction::Report::Processor, " when processing a report" do
   before do
-    Puppet.settings.stubs(:value).with(:reports).returns("one")
+    Puppet[:reports] = "one"
     Puppet.settings.stubs(:use)
     @reporter = Puppet::Transaction::Report::Processor.new
 
     @report_type = mock 'one'
     @dup_report = mock 'dupe report'
     @dup_report.stubs(:process)
-    @report = mock 'report'
+    @report = Puppet::Transaction::Report.new('apply')
     @report.expects(:dup).returns(@dup_report)
 
     @request = stub 'request', :instance => @report
@@ -74,7 +76,7 @@ describe Puppet::Transaction::Report::Processor, " when processing a report" do
   end
 
   it "should not raise exceptions" do
-    Puppet.settings.stubs(:value).with(:trace).returns(false)
+    Puppet[:trace] = false
     @dup_report.expects(:process).raises(ArgumentError)
     proc { @reporter.save(@request) }.should_not raise_error
   end
-- 
cgit 


From 3165364e20fc008b3997effafb290fe47c13bf42 Mon Sep 17 00:00:00 2001
From: Matt Robinson <matt@puppetlabs.com>
Date: Tue, 19 Jul 2011 14:18:43 -0700
Subject: maint: Adding logging to include environment when source fails

Reviewed-by: Max Martin <max@puppetlabs.com>
---
 lib/puppet/type/file/source.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index 6ebec51fe..2fb65bbad 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -154,7 +154,7 @@ module Puppet
           fail detail, "Could not retrieve file metadata for #{source}: #{detail}"
         end
       end
-      fail "Could not retrieve information from source(s) #{value.join(", ")}" unless result
+      fail "Could not retrieve information from environment #{Puppet[:environment]} source(s) #{value.join(", ")}" unless result
       result
     end
 
-- 
cgit 


From 89c021cac52a4bdecbe490772cb73cef9ef049c5 Mon Sep 17 00:00:00 2001
From: Matt Robinson <matt@puppetlabs.com>
Date: Tue, 19 Jul 2011 14:19:26 -0700
Subject: (#8418) Fix inspect app to have the correct run_mode

Requiring puppet before the run_mode has been set by the application
causes the default run_mode of 'user' to be set instead of what the
application wants.  This leads to the incorrect default settings to be
used, which lead to inspect not being able to properly retrieve file
metadata from a fileserver.

Reviewed-by: Max Martin <max@puppetlabs.com>
---
 lib/puppet/application/inspect.rb     | 7 +++++--
 spec/unit/application/inspect_spec.rb | 5 +++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/application/inspect.rb b/lib/puppet/application/inspect.rb
index ce32c661c..2260feed7 100644
--- a/lib/puppet/application/inspect.rb
+++ b/lib/puppet/application/inspect.rb
@@ -1,6 +1,4 @@
-require 'puppet'
 require 'puppet/application'
-require 'puppet/file_bucket/dipper'
 
 class Puppet::Application::Inspect < Puppet::Application
 
@@ -97,6 +95,11 @@ Licensed under the GNU General Public License version 2
     Puppet::Resource::Catalog.terminus_class = :yaml
   end
 
+  def preinit
+    require 'puppet'
+    require 'puppet/file_bucket/dipper'
+  end
+
   def run_command
     benchmark(:notice, "Finished inspection") do
       retrieval_starttime = Time.now
diff --git a/spec/unit/application/inspect_spec.rb b/spec/unit/application/inspect_spec.rb
index d334a87ee..637a95d42 100755
--- a/spec/unit/application/inspect_spec.rb
+++ b/spec/unit/application/inspect_spec.rb
@@ -13,6 +13,11 @@ describe Puppet::Application::Inspect do
 
   before :each do
     @inspect = Puppet::Application[:inspect]
+    @inspect.preinit
+  end
+
+  it "should operate in agent run_mode" do
+    @inspect.class.run_mode.name.should == :agent
   end
 
   describe "during setup" do
-- 
cgit 


From cc2c3edbb6907e8be03792a83f2b81fb839f5189 Mon Sep 17 00:00:00 2001
From: Pieter van de Bruggen <pieter@puppetlabs.com>
Date: Mon, 25 Jul 2011 15:13:28 -0700
Subject: (Maint.) Unquoting HEREDOCs.

The additional quotation marks frustrate certain syntax highlighters,
and are completely unnecessary for their use.

Reviewed-By: Daniel Pittman
---
 lib/puppet/face/certificate_request.rb         | 10 +++++-----
 lib/puppet/face/certificate_revocation_list.rb |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/lib/puppet/face/certificate_request.rb b/lib/puppet/face/certificate_request.rb
index 774821f12..1fb4e81cc 100644
--- a/lib/puppet/face/certificate_request.rb
+++ b/lib/puppet/face/certificate_request.rb
@@ -5,7 +5,7 @@ Puppet::Indirector::Face.define(:certificate_request, '0.0.1') do
   license   "Apache 2 license; see COPYING"
 
   summary "Manage certificate requests."
-  description <<-'EOT'
+  description <<-EOT
     This subcommand retrieves and submits certificate signing requests (CSRs).
   EOT
 
@@ -15,14 +15,14 @@ Puppet::Indirector::Face.define(:certificate_request, '0.0.1') do
   find = get_action(:find)
   find.summary "Retrieve a single CSR."
   find.arguments "<host>"
-  find.returns <<-'EOT'
+  find.returns <<-EOT
     A single certificate request. When used from the Ruby API, returns a
     Puppet::SSL::CertificateRequest object.
 
     RENDERING ISSUES: In most cases, you will want to render this as a string
     ('--render-as s').
   EOT
-  find.examples <<-'EOT'
+  find.examples <<-EOT
     Retrieve a single CSR from the puppet master's CA:
 
     $ puppet certificate_request find somenode.puppetlabs.lan --terminus rest
@@ -31,10 +31,10 @@ Puppet::Indirector::Face.define(:certificate_request, '0.0.1') do
   search = get_action(:search)
   search.summary "Retrieve all outstanding CSRs."
   search.arguments "<dummy_text>"
-  search.returns <<-'EOT'
     A list of certificate requests; be sure to to render this as a string
     ('--render-as s'). When used from the Ruby API, returns an array of
     Puppet::SSL::CertificateRequest objects.
+  search.returns <<-EOT
   EOT
   search.short_description <<-EOT
     Retrieves all outstanding certificate signing requests. Due to a known bug,
@@ -44,7 +44,7 @@ Puppet::Indirector::Face.define(:certificate_request, '0.0.1') do
     Although this action always returns all CSRs, it requires a dummy search
     key; this is a known bug.
   EOT
-  search.examples <<-'EOT'
+  search.examples <<-EOT
     Retrieve all CSRs from the local CA (similar to 'puppet cert list'):
 
     $ puppet certificate_request search x --terminus ca
diff --git a/lib/puppet/face/certificate_revocation_list.rb b/lib/puppet/face/certificate_revocation_list.rb
index f58368f75..1623d4342 100644
--- a/lib/puppet/face/certificate_revocation_list.rb
+++ b/lib/puppet/face/certificate_revocation_list.rb
@@ -5,7 +5,7 @@ Puppet::Indirector::Face.define(:certificate_revocation_list, '0.0.1') do
   license   "Apache 2 license; see COPYING"
 
   summary "Manage the list of revoked certificates."
-  description <<-'EOT'
+  description <<-EOT
     This subcommand is primarily for retrieving the certificate revocation
     list from the CA.
   EOT
@@ -13,7 +13,7 @@ Puppet::Indirector::Face.define(:certificate_revocation_list, '0.0.1') do
   find = get_action(:find)
   find.summary "Retrieve the certificate revocation list."
   find.arguments "<dummy_text>"
-  find.returns <<-'EOT'
+  find.returns <<-EOT
     The certificate revocation list. When used from the Ruby API: returns an
     OpenSSL::X509::CRL object.
 
@@ -28,7 +28,7 @@ Puppet::Indirector::Face.define(:certificate_revocation_list, '0.0.1') do
     Although this action always returns the CRL from the specified terminus, it
     requires a dummy argument; this is a known bug.
   EOT
-  find.examples <<-'EXAMPLES'
+  find.examples <<-EXAMPLES
     Retrieve a copy of the puppet master's CRL:
 
     $ puppet certificate_revocation_list find crl --terminus rest
@@ -38,7 +38,7 @@ Puppet::Indirector::Face.define(:certificate_revocation_list, '0.0.1') do
   destroy.summary "Delete the certificate revocation list."
   destroy.arguments "<dummy_text>"
   destroy.returns "Nothing."
-  destroy.description <<-'EOT'
+  destroy.description <<-EOT
     Deletes the certificate revocation list. This cannot be done over REST, but
     it is possible to delete the locally cached copy or the local CA's copy of
     the CRL.
-- 
cgit 


From 0366b1856489f01f1c46519dfebbda3a8676f933 Mon Sep 17 00:00:00 2001
From: Pieter van de Bruggen <pieter@puppetlabs.com>
Date: Mon, 25 Jul 2011 15:15:51 -0700
Subject: (#7293) Set default format for SSL-related faces.

By default, the SSL-related faces should all render a strings,
not with `Object#inspect`.

Reviewed-By: Daniel Pittman
---
 lib/puppet/face/certificate.rb                 |  4 ++--
 lib/puppet/face/certificate_request.rb         | 10 ++++------
 lib/puppet/face/certificate_revocation_list.rb |  4 +---
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/lib/puppet/face/certificate.rb b/lib/puppet/face/certificate.rb
index 5e176e27e..8019b6bea 100644
--- a/lib/puppet/face/certificate.rb
+++ b/lib/puppet/face/certificate.rb
@@ -97,9 +97,9 @@ Puppet::Indirector::Face.define(:certificate, '0.0.1') do
   find = get_action(:find)
   find.summary "Retrieve a certificate."
   find.arguments "<host>"
+  find.render_as = :s
   find.returns <<-EOT
-    An x509 SSL certificate. You will usually want to render this as a
-    string (--render-as s).
+    An x509 SSL certificate.
 
     Note that this action has a side effect of caching a copy of the
     certificate in Puppet's `ssldir`.
diff --git a/lib/puppet/face/certificate_request.rb b/lib/puppet/face/certificate_request.rb
index 1fb4e81cc..cf342d51a 100644
--- a/lib/puppet/face/certificate_request.rb
+++ b/lib/puppet/face/certificate_request.rb
@@ -15,12 +15,10 @@ Puppet::Indirector::Face.define(:certificate_request, '0.0.1') do
   find = get_action(:find)
   find.summary "Retrieve a single CSR."
   find.arguments "<host>"
+  find.render_as = :s
   find.returns <<-EOT
     A single certificate request. When used from the Ruby API, returns a
     Puppet::SSL::CertificateRequest object.
-
-    RENDERING ISSUES: In most cases, you will want to render this as a string
-    ('--render-as s').
   EOT
   find.examples <<-EOT
     Retrieve a single CSR from the puppet master's CA:
@@ -31,10 +29,10 @@ Puppet::Indirector::Face.define(:certificate_request, '0.0.1') do
   search = get_action(:search)
   search.summary "Retrieve all outstanding CSRs."
   search.arguments "<dummy_text>"
-    A list of certificate requests; be sure to to render this as a string
-    ('--render-as s'). When used from the Ruby API, returns an array of
-    Puppet::SSL::CertificateRequest objects.
+  search.render_as = :s
   search.returns <<-EOT
+    A list of certificate requests. When used from the Ruby API, returns an
+    array of Puppet::SSL::CertificateRequest objects.
   EOT
   search.short_description <<-EOT
     Retrieves all outstanding certificate signing requests. Due to a known bug,
diff --git a/lib/puppet/face/certificate_revocation_list.rb b/lib/puppet/face/certificate_revocation_list.rb
index 1623d4342..022323b29 100644
--- a/lib/puppet/face/certificate_revocation_list.rb
+++ b/lib/puppet/face/certificate_revocation_list.rb
@@ -13,12 +13,10 @@ Puppet::Indirector::Face.define(:certificate_revocation_list, '0.0.1') do
   find = get_action(:find)
   find.summary "Retrieve the certificate revocation list."
   find.arguments "<dummy_text>"
+  find.render_as = :s
   find.returns <<-EOT
     The certificate revocation list. When used from the Ruby API: returns an
     OpenSSL::X509::CRL object.
-
-    RENDERING ISSUES: this should usually be rendered as a string
-    ('--render-as s').
   EOT
   find.short_description <<-EOT
     Retrieves the certificate revocation list. Due to a known bug, this action
-- 
cgit 


From 778127d0c3cf01701d227ec88a8e5e6550638c35 Mon Sep 17 00:00:00 2001
From: Matt Robinson <matt@puppetlabs.com>
Date: Mon, 25 Jul 2011 16:18:54 -0700
Subject: maint: Fix cert app to print help and exit if no subcommand

In 2.6.x this was the behavior, but the changes to the way options
parsing worked in 2.7.x to support faces changed the behavior of how
help was called.

This resulted in the follow unhelpful error message when you just called
`puppet cert`:

    /Users/matthewrobinson/work/puppet/lib/puppet/ssl/certificate_authority/interface.rb:85:in `method='
    Invalid method  to apply

Reviewed-by: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/application/cert.rb     |  3 ++-
 spec/unit/application/cert_spec.rb | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/application/cert.rb b/lib/puppet/application/cert.rb
index 162672b6a..330fba8bd 100644
--- a/lib/puppet/application/cert.rb
+++ b/lib/puppet/application/cert.rb
@@ -218,7 +218,8 @@ Copyright (c) 2011 Puppet Labs, LLC Licensed under the Apache 2.0 License
       if sub = self.command_line.args.shift then
         self.subcommand = sub
       else
-        help
+        puts help
+        exit
       end
     end
     result
diff --git a/spec/unit/application/cert_spec.rb b/spec/unit/application/cert_spec.rb
index 7510f0783..300234c2b 100755
--- a/spec/unit/application/cert_spec.rb
+++ b/spec/unit/application/cert_spec.rb
@@ -208,5 +208,15 @@ describe Puppet::Application::Cert, :'fails_on_ruby_1.9.2' => true do
         args.should == ["fun.example.com"]
       end
     end
+
+    it "should print help and exit if there is no subcommand" do
+      args = []
+      @cert_app.command_line.stubs(:args).returns(args)
+      @cert_app.stubs(:help).returns("I called for help!")
+      @cert_app.expects(:puts).with("I called for help!")
+
+      expect { @cert_app.parse_options }.to exit_with 0
+      @cert_app.subcommand.should be_nil
+    end
   end
 end
-- 
cgit 


From fb2ffd6879f4c885fe29f70e3cf9bcde89cdc8e9 Mon Sep 17 00:00:00 2001
From: Nick Lewis <nick@puppetlabs.com>
Date: Mon, 25 Jul 2011 15:34:56 -0700
Subject: (#8596) Detect resource alias conflicts when titles do not match

The introduction of composite namevars caused the resource title used in
resource aliases to be set as an array, even when the resource only had one
namevar. This would fail to conflict with non-alias entries in the resource
table, which used a string for the title, even though the single element array
contained the same string.

Now, we flatten the key used in the resource table, so that single element
arrays are represented as strings, and will properly conflict with resource
titles.

Paired-With: Jacob Helwig <jacob@puppetlabs.com>
---
 lib/puppet/resource/catalog.rb     |  9 ++++--
 spec/unit/resource/catalog_spec.rb | 61 +++++++++++++++++++++++++++++++++-----
 2 files changed, 60 insertions(+), 10 deletions(-)

diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index 0a63ff172..2fdd19b0c 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -98,7 +98,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     resource.ref =~ /^(.+)\[/
     class_name = $1 || resource.class.name
 
-    newref = [class_name, key]
+    newref = [class_name, key].flatten
 
     if key.is_a? String
       ref_string = "#{class_name}[#{key}]"
@@ -111,7 +111,10 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     # isn't sufficient.
     if existing = @resource_table[newref]
       return if existing == resource
-      raise(ArgumentError, "Cannot alias #{resource.ref} to #{key.inspect}; resource #{newref.inspect} already exists")
+      resource_definition = " at #{resource.file}:#{resource.line}" if resource.file and resource.line
+      existing_definition = " at #{existing.file}:#{existing.line}" if existing.file and existing.line
+      msg = "Cannot alias #{resource.ref} to #{key.inspect}#{resource_definition}; resource #{newref.inspect} already defined#{existing_definition}"
+      raise ArgumentError, msg
     end
     @resource_table[newref] = resource
     @aliases[resource.ref] ||= []
@@ -377,7 +380,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
       res = Puppet::Resource.new(nil, type)
     end
     title_key      = [res.type, res.title.to_s]
-    uniqueness_key = [res.type, res.uniqueness_key]
+    uniqueness_key = [res.type, res.uniqueness_key].flatten
     @resource_table[title_key] || @resource_table[uniqueness_key]
   end
 
diff --git a/spec/unit/resource/catalog_spec.rb b/spec/unit/resource/catalog_spec.rb
index a15a912ef..4600022b3 100755
--- a/spec/unit/resource/catalog_spec.rb
+++ b/spec/unit/resource/catalog_spec.rb
@@ -507,7 +507,7 @@ describe Puppet::Resource::Catalog, "when compiling" do
       lambda { @catalog.alias(@one, "other") }.should_not raise_error
     end
 
-    it "should create aliases for resources isomorphic resources whose names do not match their titles" do
+    it "should create aliases for isomorphic resources whose names do not match their titles" do
       resource = Puppet::Type::File.new(:title => "testing", :path => @basepath+"/something")
 
       @catalog.add_resource(resource)
@@ -515,7 +515,7 @@ describe Puppet::Resource::Catalog, "when compiling" do
       @catalog.resource(:file, @basepath+"/something").should equal(resource)
     end
 
-    it "should not create aliases for resources non-isomorphic resources whose names do not match their titles" do
+    it "should not create aliases for non-isomorphic resources whose names do not match their titles" do
       resource = Puppet::Type.type(:exec).new(:title => "testing", :command => "echo", :path => %w{/bin /usr/bin /usr/local/bin})
 
       @catalog.add_resource(resource)
@@ -531,11 +531,6 @@ describe Puppet::Resource::Catalog, "when compiling" do
       @catalog.resource("notify", "other").should equal(@one)
     end
 
-    it "should ignore conflicting aliases that point to the aliased resource" do
-      @catalog.alias(@one, "other")
-      lambda { @catalog.alias(@one, "other") }.should_not raise_error
-    end
-
     it "should fail to add an alias if the aliased name already exists" do
       @catalog.add_resource @one
       proc { @catalog.alias @two, "one" }.should raise_error(ArgumentError)
@@ -589,6 +584,58 @@ describe Puppet::Resource::Catalog, "when compiling" do
       @catalog.create_resource :file, args
       @catalog.resource("File[/yay]").should equal(resource)
     end
+
+    describe "when adding resources with multiple namevars" do
+      before :each do
+        Puppet::Type.newtype(:multiple) do
+          newparam(:color, :namevar => true)
+          newparam(:designation, :namevar => true)
+
+          def self.title_patterns
+            [ [
+                /^(\w+) (\w+)$/,
+                [
+                  [:color,  lambda{|x| x}],
+                  [:designation, lambda{|x| x}]
+                ]
+            ] ]
+          end
+        end
+      end
+
+      it "should add an alias using the uniqueness key" do
+        @resource = Puppet::Type.type(:multiple).new(:title => "some resource", :color => "red", :designation => "5")
+
+        @catalog.add_resource(@resource)
+        @catalog.resource(:multiple, "some resource").must == @resource
+        @catalog.resource("Multiple[some resource]").must == @resource
+        @catalog.resource("Multiple[red 5]").must == @resource
+      end
+
+      it "should conflict with a resource with the same uniqueness key" do
+        @resource = Puppet::Type.type(:multiple).new(:title => "some resource", :color => "red", :designation => "5")
+        @other    = Puppet::Type.type(:multiple).new(:title => "another resource", :color => "red", :designation => "5")
+
+        @catalog.add_resource(@resource)
+        expect { @catalog.add_resource(@other) }.to raise_error(ArgumentError, /Cannot alias Multiple\[another resource\] to \["red", "5"\].*resource \["Multiple", "red", "5"\] already defined/)
+      end
+
+      it "should conflict when its uniqueness key matches another resource's title" do
+        @resource = Puppet::Type.type(:file).new(:title => "/tmp/foo")
+        @other    = Puppet::Type.type(:file).new(:title => "another file", :path => "/tmp/foo")
+
+        @catalog.add_resource(@resource)
+        expect { @catalog.add_resource(@other) }.to raise_error(ArgumentError, /Cannot alias File\[another file\] to \["\/tmp\/foo"\].*resource \["File", "\/tmp\/foo"\] already defined/)
+      end
+
+      it "should conflict when its uniqueness key matches the uniqueness key derived from another resource's title" do
+        @resource = Puppet::Type.type(:multiple).new(:title => "red leader")
+        @other    = Puppet::Type.type(:multiple).new(:title => "another resource", :color => "red", :designation => "leader")
+
+        @catalog.add_resource(@resource)
+        expect { @catalog.add_resource(@other) }.to raise_error(ArgumentError, /Cannot alias Multiple\[another resource\] to \["red", "leader"\].*resource \["Multiple", "red", "leader"\] already defined/)
+      end
+    end
   end
 
   describe "when applying" do
-- 
cgit 


From 1d4acb5afda61b1f2c05223afff19c68248a3996 Mon Sep 17 00:00:00 2001
From: Matt Robinson <matt@puppetlabs.com>
Date: Tue, 19 Jul 2011 11:27:03 -0700
Subject: maint: Suggest where to start troubleshooting SSL error message

Much like the infamous "hostname was not match" error message, there's
another SSL error that people run into that isn't clear how to
troubleshoot.

   err: Could not send report: SSL_connect returned=1 errno=0
   state=SSLv3 read server certificate B: certificate verify failed.

As far as I can tell this only ever happens when the clock is off on the
master or client.  People seem to think it will happen other times, but
I haven't been able to reproduce it other ways - missing private key,
revoked cert, offline CA all have their own errors.  I googled around
and the only thing I've seen for this error in relation to puppet is the
time sync problem.

So the error message text just has some additional info to suggest you
check your clocks.

Reviewed-by: Nick Lewis <nick@puppetlabs.com>
---
 lib/puppet/indirector/rest.rb     |  4 +-
 spec/unit/indirector/rest_spec.rb | 83 ++++++++++++++++++++++-----------------
 2 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index 8018fe8e3..19daff51d 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -93,7 +93,9 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
 
     http_connection.send(method, *args)
   rescue OpenSSL::SSL::SSLError => error
-    if error.message.include? "hostname was not match"
+    if error.message.include? "certificate verify failed"
+      raise Puppet::Error, "#{error.message}.  This is often because the time is out of sync on the server or client"
+    elsif error.message.include? "hostname was not match"
       raise unless cert = peer_certs.find { |c| c.name !~ /^puppet ca/i }
 
       valid_certnames = [cert.name, *cert.alternate_names].uniq
diff --git a/spec/unit/indirector/rest_spec.rb b/spec/unit/indirector/rest_spec.rb
index ee0111a77..042b7ca16 100755
--- a/spec/unit/indirector/rest_spec.rb
+++ b/spec/unit/indirector/rest_spec.rb
@@ -90,42 +90,53 @@ describe Puppet::Indirector::REST do
     @rest_class.port.should == 543
   end
 
-	describe "when making http requests" do
-		it "should provide a helpful error message when hostname was not match with server certificate" do
-			Puppet[:certdnsnames] = 'foo:bar:baz'
-			csr            = OpenSSL::X509::Request.new
-			csr.subject    = OpenSSL::X509::Name.new([['CN', 'not_my_server']])
-			csr.public_key = OpenSSL::PKey::RSA.generate(Puppet[:keylength]).public_key
-			cert = Puppet::SSL::CertificateFactory.new('server', csr, csr, 14).result
-
-			connection = Net::HTTP.new('my_server', 8140)
-			@searcher.stubs(:network).returns(connection)
-			ssl_context = OpenSSL::SSL::SSLContext.new
-			ssl_context.stubs(:current_cert).returns(cert)
-			connection.stubs(:get).with do
-				connection.verify_callback.call(true, ssl_context)
-			end.raises(OpenSSL::SSL::SSLError.new('hostname was not match with server certificate'))
-
-			msg = /Server hostname 'my_server' did not match server certificate; expected one of (.+)/
-			expect { @searcher.http_request(:get, stub('request')) }.to(
-				raise_error(Puppet::Error, msg) do |error|
-					error.message =~ msg
-					$1.split(', ').should =~ ['foo', 'bar', 'baz', 'not_my_server']
-				end
-			)
-		end
-
-		it "should pass along the error message otherwise" do
-			connection = Net::HTTP.new('my_server', 8140)
-			@searcher.stubs(:network).returns(connection)
-
-			connection.stubs(:get).raises(OpenSSL::SSL::SSLError.new('certificate verify failed'))
-
-			expect do
-				@searcher.http_request(:get, stub('request'))
-			end.to raise_error(/certificate verify failed/)
-		end
-	end
+  describe "when making http requests" do
+    it "should provide a suggestive error message when certificate verify failed" do
+      connection = Net::HTTP.new('my_server', 8140)
+      @searcher.stubs(:network).returns(connection)
+
+      connection.stubs(:get).raises(OpenSSL::SSL::SSLError.new('certificate verify failed'))
+
+      expect do
+        @searcher.http_request(:get, stub('request'))
+      end.to raise_error(/This is often because the time is out of sync on the server or client/)
+    end
+
+    it "should provide a helpful error message when hostname was not match with server certificate" do
+      Puppet[:certdnsnames] = 'foo:bar:baz'
+      csr            = OpenSSL::X509::Request.new
+      csr.subject    = OpenSSL::X509::Name.new([['CN', 'not_my_server']])
+      csr.public_key = OpenSSL::PKey::RSA.generate(Puppet[:keylength]).public_key
+      cert = Puppet::SSL::CertificateFactory.new('server', csr, csr, 14).result
+
+      connection = Net::HTTP.new('my_server', 8140)
+      @searcher.stubs(:network).returns(connection)
+      ssl_context = OpenSSL::SSL::SSLContext.new
+      ssl_context.stubs(:current_cert).returns(cert)
+      connection.stubs(:get).with do
+        connection.verify_callback.call(true, ssl_context)
+      end.raises(OpenSSL::SSL::SSLError.new('hostname was not match with server certificate'))
+
+      msg = /Server hostname 'my_server' did not match server certificate; expected one of (.+)/
+      expect { @searcher.http_request(:get, stub('request')) }.to(
+        raise_error(Puppet::Error, msg) do |error|
+          error.message =~ msg
+          $1.split(', ').should =~ ['foo', 'bar', 'baz', 'not_my_server']
+        end
+      )
+    end
+
+    it "should pass along the error message otherwise" do
+      connection = Net::HTTP.new('my_server', 8140)
+      @searcher.stubs(:network).returns(connection)
+
+      connection.stubs(:get).raises(OpenSSL::SSL::SSLError.new('some other message'))
+
+      expect do
+        @searcher.http_request(:get, stub('request'))
+      end.to raise_error(/some other message/)
+    end
+  end
 
   describe "when deserializing responses" do
     it "should return nil if the response code is 404" do
-- 
cgit 


From 8da0486c36e095e9a08b6f5a79abf3cabeba6603 Mon Sep 17 00:00:00 2001
From: Brice Figureau <brice-puppet@daysofwonder.com>
Date: Mon, 30 May 2011 20:08:19 +0200
Subject: Fix #5777 - rule interpolation broke auth.conf CIDR rules

Due to the interpolation performed to support regex back-references,
various type of rules (mainly IP CIDR notations and wildcard hosts)
were not working anymore.

This patch adds an auth.conf integration test that covers most
of the various rules types.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>
---
 spec/integration/network/rest_authconfig_spec.rb | 147 +++++++++++++++++++++++
 1 file changed, 147 insertions(+)
 create mode 100644 spec/integration/network/rest_authconfig_spec.rb

diff --git a/spec/integration/network/rest_authconfig_spec.rb b/spec/integration/network/rest_authconfig_spec.rb
new file mode 100644
index 000000000..dc4d3282b
--- /dev/null
+++ b/spec/integration/network/rest_authconfig_spec.rb
@@ -0,0 +1,147 @@
+require 'spec_helper'
+
+require 'puppet/network/rest_authconfig'
+
+RSpec::Matchers.define :allow do |params|
+
+  match do |auth|
+    begin
+      auth.allowed?(params[0], params[1], params[2], params[3])
+      true
+    rescue Puppet::Network::AuthorizationError
+      false
+    end
+  end
+
+  failure_message_for_should do |instance|
+    "expected #{params[3][:node]}/#{params[3][:ip]} to be allowed"
+  end
+
+  failure_message_for_should_not do |instance|
+    "expected #{params[3][:node]}/#{params[3][:ip]} to be forbidden"
+  end
+end
+
+describe Puppet::Network::RestAuthConfig do
+  include PuppetSpec::Files
+
+  before(:each) do
+    Puppet[:rest_authconfig] = tmpfile('auth.conf')
+  end
+
+  def add_rule(rule)
+    File.open(Puppet[:rest_authconfig],"w+") do |f|
+      f.print "path /test\n#{rule}\n"
+    end
+    @auth = Puppet::Network::RestAuthConfig.new(Puppet[:rest_authconfig], true)
+  end
+
+  def add_regex_rule(regex, rule)
+    File.open(Puppet[:rest_authconfig],"w+") do |f|
+      f.print "path ~ #{regex}\n#{rule}\n"
+    end
+    @auth = Puppet::Network::RestAuthConfig.new(Puppet[:rest_authconfig], true)
+  end
+
+  def request(args = {})
+    { :ip => '10.1.1.1', :node => 'host.domain.com', :key => 'key', :authenticated => true }.each do |k,v|
+      args[k] ||= v
+    end
+    ['test', :find, args[:key], args]
+  end
+
+  it "should support IPv4 address" do
+    add_rule("allow 10.1.1.1")
+
+    @auth.should allow(request)
+  end
+
+  it "should support CIDR IPv4 address" do
+    add_rule("allow 10.0.0.0/8")
+
+    @auth.should allow(request)
+  end
+
+  it "should support wildcard IPv4 address" do
+    add_rule("allow 10.1.1.*")
+
+    @auth.should allow(request)
+  end
+
+  it "should support IPv6 address" do
+    add_rule("allow 2001:DB8::8:800:200C:417A")
+
+    @auth.should allow(request(:ip => '2001:DB8::8:800:200C:417A'))
+  end
+
+  it "should support hostname" do
+    add_rule("allow host.domain.com")
+
+    @auth.should allow(request)
+  end
+
+  it "should support wildcard host" do
+    add_rule("allow *.domain.com")
+
+    @auth.should allow(request)
+  end
+
+  it "should support hostname backreferences" do
+    add_regex_rule('^/test/([^/]+)$', "allow $1.domain.com")
+
+    @auth.should allow(request(:key => 'host'))
+  end
+
+  it "should support opaque strings" do
+    add_rule("allow this-is-opaque@or-not")
+
+    @auth.should allow(request(:node => 'this-is-opaque@or-not'))
+  end
+
+  it "should support opaque strings and backreferences" do
+    add_regex_rule('^/test/([^/]+)$', "allow $1")
+
+    @auth.should allow(request(:key => 'this-is-opaque@or-not', :node => 'this-is-opaque@or-not'))
+  end
+
+  it "should support hostname ending with '.'" do
+    pending('bug #7589')
+    add_rule("allow host.domain.com.")
+
+    @auth.should allow(request(:node => 'host.domain.com.'))
+  end
+
+  it "should support hostname ending with '.' and backreferences" do
+    pending('bug #7589')
+    add_regex_rule('^/test/([^/]+)$',"allow $1")
+
+    @auth.should allow(request(:node => 'host.domain.com.'))
+  end
+
+  it "should support trailing whitespace" do
+    pending('bug #5010')
+    add_rule('allow host.domain.com    ')
+
+    @auth.should allow(request)
+  end
+
+  it "should support inlined comments" do
+    pending('bug #6026')
+    add_rule('allow host.domain.com # will it work?')
+
+    @auth.should allow(request)
+  end
+
+  it "should deny non-matching host" do
+    add_rule("allow inexistant")
+
+    @auth.should_not allow(request)
+  end
+
+  it "should deny denied hosts" do
+    add_rule("deny host.domain.com")
+
+    @auth.should_not allow(request)
+  end
+
+end
\ No newline at end of file
-- 
cgit 


From 0c385f1fb436ab6f667693d347f711470305a019 Mon Sep 17 00:00:00 2001
From: Brice Figureau <brice-puppet@daysofwonder.com>
Date: Mon, 30 May 2011 20:17:11 +0200
Subject: Fix #5010 - Allow leading whitespace in auth.conf

The regex used to detect ACE is too lax and would allow trailing
spaces to sneak in, which in turn would confuse the ACE parser.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>
---
 lib/puppet/network/authconfig.rb                 |  2 ++
 spec/integration/network/rest_authconfig_spec.rb |  1 -
 spec/unit/network/authconfig_spec.rb             | 12 ++++++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/network/authconfig.rb b/lib/puppet/network/authconfig.rb
index 4ba89fa71..61fb24ded 100644
--- a/lib/puppet/network/authconfig.rb
+++ b/lib/puppet/network/authconfig.rb
@@ -130,6 +130,7 @@ module Puppet
     end
 
     def parse_right_directive(right, var, value, count)
+      value.strip!
       case var
       when "allow"
         modify_right(right, :allow, value, "allowing %s access", count)
@@ -159,6 +160,7 @@ module Puppet
     def modify_right(right, method, value, msg, count)
       value.split(/\s*,\s*/).each do |val|
         begin
+          val.strip!
           right.info msg % val
           right.send(method, val)
         rescue AuthStoreError => detail
diff --git a/spec/integration/network/rest_authconfig_spec.rb b/spec/integration/network/rest_authconfig_spec.rb
index dc4d3282b..7c50d8970 100644
--- a/spec/integration/network/rest_authconfig_spec.rb
+++ b/spec/integration/network/rest_authconfig_spec.rb
@@ -119,7 +119,6 @@ describe Puppet::Network::RestAuthConfig do
   end
 
   it "should support trailing whitespace" do
-    pending('bug #5010')
     add_rule('allow host.domain.com    ')
 
     @auth.should allow(request)
diff --git a/spec/unit/network/authconfig_spec.rb b/spec/unit/network/authconfig_spec.rb
index c47b2e0c5..f33de7872 100755
--- a/spec/unit/network/authconfig_spec.rb
+++ b/spec/unit/network/authconfig_spec.rb
@@ -184,6 +184,18 @@ describe Puppet::Network::AuthConfig do
       @authconfig.read
     end
 
+    it "should strip whitespace around ACE" do
+      acl = stub 'acl', :info
+
+      @fd.stubs(:each).multiple_yields('[puppetca]', ' allow 127.0.0.1 , 172.16.10.0  ')
+      @rights.stubs(:newright).with("[puppetca]", 1, 'dummy').returns(acl)
+
+      acl.expects(:allow).with('127.0.0.1')
+      acl.expects(:allow).with('172.16.10.0')
+
+      @authconfig.read
+    end
+
     it "should create an allow ACE on each subsequent allow" do
       acl = stub 'acl', :info
 
-- 
cgit 


From 6401dfe5602fd39cc59ec1f1b3822110e4ad864a Mon Sep 17 00:00:00 2001
From: Brice Figureau <brice-puppet@daysofwonder.com>
Date: Mon, 30 May 2011 20:31:14 +0200
Subject: Fix #6026 - security file should support inline comments

Auth.conf, namespaceauth.conf and fileserver.conf were not supporting
trailing inlined comments.
Also this commit fixes some indentation and error management.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>
---
 lib/puppet/file_serving/configuration/parser.rb     | 19 ++++++-------------
 lib/puppet/network/authconfig.rb                    |  2 +-
 spec/integration/network/rest_authconfig_spec.rb    |  1 -
 spec/unit/file_serving/configuration/parser_spec.rb |  8 ++++++++
 spec/unit/network/authconfig_spec.rb                | 11 +++++++++++
 5 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/lib/puppet/file_serving/configuration/parser.rb b/lib/puppet/file_serving/configuration/parser.rb
index 334201d37..83b75e28f 100644
--- a/lib/puppet/file_serving/configuration/parser.rb
+++ b/lib/puppet/file_serving/configuration/parser.rb
@@ -24,9 +24,10 @@ class Puppet::FileServing::Configuration::Parser < Puppet::Util::LoadedFile
         when /^\s*$/; next # skip blank lines
         when /\[([-\w]+)\]/
           mount = newmount($1)
-        when /^\s*(\w+)\s+(.+)$/
+        when /^\s*(\w+)\s+(.+?)(\s*#.*)?$/
           var = $1
           value = $2
+          value.strip!
           raise(ArgumentError, "Fileserver configuration file does not use '=' as a separator") if value =~ /^=/
           case var
           when "path"
@@ -58,12 +59,8 @@ class Puppet::FileServing::Configuration::Parser < Puppet::Util::LoadedFile
       begin
         mount.info "allowing #{val} access"
         mount.allow(val)
-      rescue AuthStoreError => detail
-
-              raise ArgumentError.new(
-        detail.to_s,
-        
-          @count, file)
+      rescue Puppet::AuthStoreError => detail
+        raise ArgumentError.new(detail.to_s, @count, file)
       end
     }
   end
@@ -75,12 +72,8 @@ class Puppet::FileServing::Configuration::Parser < Puppet::Util::LoadedFile
       begin
         mount.info "denying #{val} access"
         mount.deny(val)
-      rescue AuthStoreError => detail
-
-              raise ArgumentError.new(
-        detail.to_s,
-        
-          @count, file)
+      rescue Puppet::AuthStoreError => detail
+        raise ArgumentError.new(detail.to_s, @count, file)
       end
     }
   end
diff --git a/lib/puppet/network/authconfig.rb b/lib/puppet/network/authconfig.rb
index 61fb24ded..1e486a2f9 100644
--- a/lib/puppet/network/authconfig.rb
+++ b/lib/puppet/network/authconfig.rb
@@ -102,7 +102,7 @@ module Puppet
               name = $3 if $2 == "path"
               name.chomp!
               right = newrights.newright(name, count, @file)
-            when /^\s*(allow|deny|method|environment|auth(?:enticated)?)\s+(.+)$/
+            when /^\s*(allow|deny|method|environment|auth(?:enticated)?)\s+(.+?)(\s*#.*)?$/
               parse_right_directive(right, $1, $2, count)
             else
               raise ConfigurationError, "Invalid line #{count}: #{line}"
diff --git a/spec/integration/network/rest_authconfig_spec.rb b/spec/integration/network/rest_authconfig_spec.rb
index 7c50d8970..0e5278860 100644
--- a/spec/integration/network/rest_authconfig_spec.rb
+++ b/spec/integration/network/rest_authconfig_spec.rb
@@ -125,7 +125,6 @@ describe Puppet::Network::RestAuthConfig do
   end
 
   it "should support inlined comments" do
-    pending('bug #6026')
     add_rule('allow host.domain.com # will it work?')
 
     @auth.should allow(request)
diff --git a/spec/unit/file_serving/configuration/parser_spec.rb b/spec/unit/file_serving/configuration/parser_spec.rb
index 3d6b3e234..5ccfc5075 100755
--- a/spec/unit/file_serving/configuration/parser_spec.rb
+++ b/spec/unit/file_serving/configuration/parser_spec.rb
@@ -118,6 +118,14 @@ describe Puppet::FileServing::Configuration::Parser do
       @parser.parse
     end
 
+    it "should support inline comments" do
+      mock_file_content "[one]\nallow something \# will it work?\n"
+
+      @mount.expects(:info)
+      @mount.expects(:allow).with("something")
+      @parser.parse
+    end
+
     it "should tell the mount to deny any deny values from the section" do
       mock_file_content "[one]\ndeny something\n"
 
diff --git a/spec/unit/network/authconfig_spec.rb b/spec/unit/network/authconfig_spec.rb
index f33de7872..ca94cc1ab 100755
--- a/spec/unit/network/authconfig_spec.rb
+++ b/spec/unit/network/authconfig_spec.rb
@@ -196,6 +196,17 @@ describe Puppet::Network::AuthConfig do
       @authconfig.read
     end
 
+    it "should allow ACE inline comments" do
+      acl = stub 'acl', :info
+
+      @fd.stubs(:each).multiple_yields('[puppetca]', ' allow 127.0.0.1 # will it work?')
+      @rights.stubs(:newright).with("[puppetca]", 1, 'dummy').returns(acl)
+
+      acl.expects(:allow).with('127.0.0.1')
+
+      @authconfig.read
+    end
+
     it "should create an allow ACE on each subsequent allow" do
       acl = stub 'acl', :info
 
-- 
cgit 


From 7e6fc0d80ccd29f206c3b56960ee1eef3afc33a3 Mon Sep 17 00:00:00 2001
From: Brice Figureau <brice-puppet@daysofwonder.com>
Date: Tue, 31 May 2011 20:01:36 +0200
Subject: Deprecate RestAuthConfig#allowed? in favor of #check_authorization

 #allowed? was a poorly named method since it isn't actually a predicate
method. Instead of returning a boolean, this methods throws an
exception when the access is denied (in order to keep the full context
of what ACE triggered the deny).

Given that #allowed? was overriding the behavior from AuthConfig, we
leave a version of #allowed? in place that will issue a deprecation
warning before delegating to #check_authorization.  Once support for
XML-RPC agents is removed from the master, we will be able to remove
this delegation, since there should no longer be a reason for a
distinction between AuthConfig and RestAuthConfig.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>
Signed-off-by: Jacob Helwig <jacob@puppetlabs.com>
---
 lib/puppet/network/rest_authconfig.rb            | 7 ++++++-
 lib/puppet/network/rest_authorization.rb         | 2 +-
 spec/integration/network/rest_authconfig_spec.rb | 2 +-
 spec/unit/network/rest_authconfig_spec.rb        | 2 +-
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/puppet/network/rest_authconfig.rb b/lib/puppet/network/rest_authconfig.rb
index dfe8f85c4..7dcc81ef4 100644
--- a/lib/puppet/network/rest_authconfig.rb
+++ b/lib/puppet/network/rest_authconfig.rb
@@ -29,10 +29,15 @@ module Puppet
       @main
     end
 
+    def allowed?(request)
+      Puppet.deprecation_warning "allowed? should not be called for REST authorization - use check_authorization instead"
+      check_authorization(request)
+    end
+
     # check wether this request is allowed in our ACL
     # raise an Puppet::Network::AuthorizedError if the request
     # is denied.
-    def allowed?(indirection, method, key, params)
+    def check_authorization(indirection, method, key, params)
       read
 
       # we're splitting the request in part because
diff --git a/lib/puppet/network/rest_authorization.rb b/lib/puppet/network/rest_authorization.rb
index 50f094e3e..d636d486a 100644
--- a/lib/puppet/network/rest_authorization.rb
+++ b/lib/puppet/network/rest_authorization.rb
@@ -16,7 +16,7 @@ module Puppet::Network
 
     # Verify that our client has access.
     def check_authorization(indirection, method, key, params)
-      authconfig.allowed?(indirection, method, key, params)
+      authconfig.check_authorization(indirection, method, key, params)
     end
   end
 end
diff --git a/spec/integration/network/rest_authconfig_spec.rb b/spec/integration/network/rest_authconfig_spec.rb
index 0e5278860..d2f539cd4 100644
--- a/spec/integration/network/rest_authconfig_spec.rb
+++ b/spec/integration/network/rest_authconfig_spec.rb
@@ -6,7 +6,7 @@ RSpec::Matchers.define :allow do |params|
 
   match do |auth|
     begin
-      auth.allowed?(params[0], params[1], params[2], params[3])
+      auth.check_authorization(params[0], params[1], params[2], params[3])
       true
     rescue Puppet::Network::AuthorizationError
       false
diff --git a/spec/unit/network/rest_authconfig_spec.rb b/spec/unit/network/rest_authconfig_spec.rb
index e1403997f..bebbb874f 100755
--- a/spec/unit/network/rest_authconfig_spec.rb
+++ b/spec/unit/network/rest_authconfig_spec.rb
@@ -29,7 +29,7 @@ describe Puppet::Network::RestAuthConfig do
     params = {:ip => "127.0.0.1", :node => "me", :environment => :env, :authenticated => true}
     @acl.expects(:is_request_forbidden_and_why?).with("path", :save, "to/resource", params).returns(nil)
 
-    @authconfig.allowed?("path", :save, "to/resource", params)
+    @authconfig.check_authorization("path", :save, "to/resource", params)
   end
 
   describe "when defining an acl with mk_acl" do
-- 
cgit 


From c315da0efeace1878a877dc4b2f4aebc1ec13f0d Mon Sep 17 00:00:00 2001
From: Peter Meier <peter.meier@immerda.ch>
Date: Fri, 29 Apr 2011 15:56:15 +0200
Subject: Fix #1886 - Add node cleanup capability

Here is a changeset that adds a new action to the puppet node face.

This application removes all traces of a node on the puppetmaster
(including certs, cached facts and nodes, reports, and storedconfig
entries).

Furthermore it is capable of unexporting exported resources of a
host so that consumers of these resources can remove the exported
resources and we will safely remove the node from our
infrastructure.

Usage:
puppet node clean [--unexport] <host> [<host2> ...]

To achieve this we add different destroy methods to the different
parts of the indirector. So for example for yaml indirections we
already offer read access for the yaml, this changeset adds
the destroy handler which only removes the yaml file for
a request. This can be used to remove cached entries.

This work is based on the initial work of Brice Figureau
<brice-puppet@daysofwonder.com>
---
 lib/puppet/face/node/clean.rb                 | 154 ++++++++++++++++
 lib/puppet/indirector/report/processor.rb     |  45 +++--
 lib/puppet/indirector/yaml.rb                 |   5 +
 lib/puppet/reports/store.rb                   |  15 ++
 spec/unit/face/node_spec.rb                   | 256 ++++++++++++++++++++++++++
 spec/unit/indirector/report/processor_spec.rb |  27 ++-
 spec/unit/indirector/yaml_spec.rb             |  18 ++
 7 files changed, 500 insertions(+), 20 deletions(-)
 create mode 100644 lib/puppet/face/node/clean.rb

diff --git a/lib/puppet/face/node/clean.rb b/lib/puppet/face/node/clean.rb
new file mode 100644
index 000000000..10d6239ba
--- /dev/null
+++ b/lib/puppet/face/node/clean.rb
@@ -0,0 +1,154 @@
+Puppet::Indirector::Face.define(:node, '0.0.1') do
+  action(:clean) do
+    option "--[no-]unexport" do
+      summary "Unexport exported resources"
+    end
+  
+    summary "Clean up everything a puppetmaster knows about a node"
+    
+    arguments "<host1> [<host2> ...]"
+
+    description <<-EOT
+This includes
+
+ * Signed certificates ($vardir/ssl/ca/signed/node.domain.pem)
+ * Cached facts ($vardir/yaml/facts/node.domain.yaml)
+ * Cached node stuff ($vardir/yaml/node/node.domain.yaml)
+ * Reports ($vardir/reports/node.domain)
+ * Stored configs: it can either remove all data from an host in your storedconfig
+ database, or with --unexport turn every exported resource supporting ensure to absent
+ so that any other host checking out their config can remove those exported configurations.
+
+This will unexport exported resources of a
+host, so that consumers of these resources can remove the exported
+resources and we will safely remove the node from our
+infrastructure.
+EOT
+    when_invoked do |*args|
+      nodes = args[0..-2]
+      options = args.last
+      raise "At least one node should be passed" if nodes.empty? || nodes == options
+
+      # TODO: this is a hack and should be removed if faces provide the proper
+      # infrastructure to set the run mode.
+      require 'puppet/util/run_mode'
+      $puppet_application_mode = Puppet::Util::RunMode[:master]
+
+      if Puppet::SSL::CertificateAuthority.ca?
+        Puppet::SSL::Host.ca_location = :local
+      else
+        Puppet::SSL::Host.ca_location = :none
+      end
+  
+      Puppet::Node::Facts.indirection.terminus_class = :yaml
+      Puppet::Node::Facts.indirection.cache_class = :yaml
+      Puppet::Node.indirection.terminus_class = :yaml
+      Puppet::Node.indirection.cache_class = :yaml
+
+      begin
+        nodes.each do |node|
+          node = node.downcase
+          clean_cert(node)
+          clean_cached_facts(node)
+          clean_cached_node(node)
+          clean_reports(node)
+          clean_storeconfigs(node,options[:unexport])
+        end
+      rescue => detail
+        puts detail.backtrace if Puppet[:trace]
+        puts detail.to_s
+      end
+    end
+  end
+  
+  # clean signed cert for +host+
+  def clean_cert(node)
+    if Puppet::SSL::Host.ca_location == :local
+      ca.apply(:revoke, :to => [node])
+      ca.apply(:destroy, :to => [node])
+      Puppet.info "%s certificates removed from ca" % node
+    else
+      Puppet.info "Not managing %s certs as this host is not a CA" % node
+    end
+  end
+
+  # clean facts for +host+
+  def clean_cached_facts(node)
+    Puppet::Node::Facts.indirection.destroy(node)
+    Puppet.info "%s's facts removed" % node
+  end
+
+  # clean cached node +host+
+  def clean_cached_node(node)
+    Puppet::Node.indirection.destroy(node)
+    Puppet.info "%s's cached node removed" % node
+  end
+
+  # clean node reports for +host+
+  def clean_reports(node)
+    Puppet::Transaction::Report.indirection.destroy(node)
+    Puppet.info "%s's reports removed" % node
+  end
+
+  # clean storeconfig for +node+
+  def clean_storeconfigs(node,do_unexport=false)
+    return unless Puppet[:storeconfigs] && Puppet.features.rails?
+    require 'puppet/rails'
+    Puppet::Rails.connect
+    unless rails_node = Puppet::Rails::Host.find_by_name(node)
+      Puppet.notice "No entries found for %s in storedconfigs." % node
+      return
+    end
+
+    if do_unexport
+      unexport(rails_node)
+      Puppet.notice "Force %s's exported resources to absent" % node
+      Puppet.warning "Please wait until all other hosts have checked out their configuration before finishing the cleanup with:"
+      Puppet.warning "$ puppet node clean #{node}"
+    else
+      rails_node.destroy
+      Puppet.notice "%s storeconfigs removed" % node
+    end
+  end
+
+  def unexport(node)
+    # fetch all exported resource
+    query = {:include => {:param_values => :param_name}}
+    query[:conditions] = ["exported=? AND host_id=?", true, node.id]
+    Puppet::Rails::Resource.find(:all, query).each do |resource|
+      if type_is_ensurable(resource)
+        line = 0
+        param_name = Puppet::Rails::ParamName.find_or_create_by_name("ensure")
+
+        if ensure_param = resource.param_values.find(
+          :first,
+          :conditions => [ 'param_name_id = ?', param_name.id]
+        )
+          line = ensure_param.line.to_i
+          Puppet::Rails::ParamValue.delete(ensure_param.id);
+        end
+
+        # force ensure parameter to "absent"
+        resource.param_values.create(
+          :value => "absent",
+          :line => line,
+          :param_name => param_name
+        )
+        Puppet.info("%s has been marked as \"absent\"" % resource.name)
+      end
+    end
+  end
+
+  def ca
+    @ca ||= Puppet::SSL::CertificateAuthority.instance
+  end
+
+  def environment
+    @environemnt ||= Puppet::Node::Environment.new
+  end
+
+  def type_is_ensurable(resource)
+      (type=Puppet::Type.type(resource.restype)) && type.validattr?(:ensure) || \
+        (type = environment.known_resource_types.find_definition('',resource.restype)) && type.arguments.keys.include?('ensure')
+  end
+end
\ No newline at end of file
diff --git a/lib/puppet/indirector/report/processor.rb b/lib/puppet/indirector/report/processor.rb
index 81b379eb8..7bdadcb36 100644
--- a/lib/puppet/indirector/report/processor.rb
+++ b/lib/puppet/indirector/report/processor.rb
@@ -14,6 +14,12 @@ class Puppet::Transaction::Report::Processor < Puppet::Indirector::Code
     process(request.instance)
   end
 
+  def destroy(request)
+    processors do |mod|
+      mod.destroy(request.key) if mod.respond_to?(:destroy)
+    end
+  end
+
   private
 
   # Process the report with each of the configured report types.
@@ -21,23 +27,17 @@ class Puppet::Transaction::Report::Processor < Puppet::Indirector::Code
   # compatible and that's good enough for now.
   def process(report)
     Puppet.debug "Recieved report to process from #{report.host}"
-    return if Puppet[:reports] == "none"
-
-    reports.each do |name|
-      Puppet.debug "Processing report from #{report.host} with processor #{name}"
-      if mod = Puppet::Reports.report(name)
-        # We have to use a dup because we're including a module in the
-        # report.
-        newrep = report.dup
-        begin
-          newrep.extend(mod)
-          newrep.process
-        rescue => detail
-          puts detail.backtrace if Puppet[:trace]
-          Puppet.err "Report #{name} failed: #{detail}"
-        end
-      else
-        Puppet.warning "No report named '#{name}'"
+    processors do |mod|
+      Puppet.debug "Processing report from #{report.host} with processor #{mod}"
+      # We have to use a dup because we're including a module in the
+      # report.
+      newrep = report.dup
+      begin
+        newrep.extend(mod)
+        newrep.process
+      rescue => detail
+        puts detail.backtrace if Puppet[:trace]
+        Puppet.err "Report #{name} failed: #{detail}"
       end
     end
   end
@@ -47,4 +47,15 @@ class Puppet::Transaction::Report::Processor < Puppet::Indirector::Code
     # LAK:NOTE See http://snurl.com/21zf8  [groups_google_com]
     x = Puppet[:reports].gsub(/(^\s+)|(\s+$)/, '').split(/\s*,\s*/)
   end
+
+  def processors(&blk)
+    return if Puppet[:reports] == "none"
+    reports.each do |name|
+      if mod = Puppet::Reports.report(name)
+        yield(mod)
+      else
+        Puppet.warning "No report named '#{name}'"
+      end
+    end
+  end
 end
diff --git a/lib/puppet/indirector/yaml.rb b/lib/puppet/indirector/yaml.rb
index 23997e97a..7b12d25e2 100644
--- a/lib/puppet/indirector/yaml.rb
+++ b/lib/puppet/indirector/yaml.rb
@@ -47,6 +47,11 @@ class Puppet::Indirector::Yaml < Puppet::Indirector::Terminus
     File.join(base, self.class.indirection_name.to_s, name.to_s + ext)
   end
 
+  def destroy(request)
+    file_path = path(request.key)
+    File.unlink(file_path) if File.exists?(file_path)
+  end
+
   def search(request)
     Dir.glob(path(request.key,'')).collect do |file|
       YAML.load_file(file)
diff --git a/lib/puppet/reports/store.rb b/lib/puppet/reports/store.rb
index 625a263b3..997206ec4 100644
--- a/lib/puppet/reports/store.rb
+++ b/lib/puppet/reports/store.rb
@@ -41,5 +41,20 @@ Puppet::Reports.register_report(:store) do
     # Only testing cares about the return value
     file
   end
+
+  # removes all reports for a given host
+  def self.destroy(host)
+    client = host.gsub("..",".")
+    dir = File.join(Puppet[:reportdir], client)
+
+    if File.exists?(dir)
+      Dir.entries(dir).each do |file|
+        next if ['.','..'].include?(file)
+        file = File.join(dir, file)
+        File.unlink(file) if File.file?(file)
+      end
+      Dir.rmdir(dir)
+    end
+  end
 end
 
diff --git a/spec/unit/face/node_spec.rb b/spec/unit/face/node_spec.rb
index 027a4cce0..4ba1d134c 100755
--- a/spec/unit/face/node_spec.rb
+++ b/spec/unit/face/node_spec.rb
@@ -4,4 +4,260 @@ require 'puppet/face'
 
 describe Puppet::Face[:node, '0.0.1'] do
   it "REVISIT: really should have some tests"
+
+  describe "clean action" do
+    it "should have a clean action" do
+      subject.should be_action :clean
+    end
+
+    it "should not accept a call with no arguments" do
+      expect { subject.clean() }.should raise_error
+    end
+
+    it "should accept a node name" do
+      expect { subject.clean('hostname') }.should_not raise_error
+    end
+
+    it "should accept more than one node name" do
+      expect { subject.clean(['hostname', 'hostname2'],{}) }.should_not raise_error
+      expect { subject.clean(['hostname', 'hostname2', 'hostname3'],{:unexport => true}) }.should_not raise_error
+    end
+
+    it "should accept the option --unexport" do
+      expect { subject.help('hostname', :unexport => true) }.
+        should_not raise_error ArgumentError
+    end
+
+    context "clean action" do
+      subject { Puppet::Face[:node, 'current'] }
+      before :each do
+        Puppet::Util::Log.stubs(:newdestination)
+        Puppet::Util::Log.stubs(:level=)
+      end
+
+      describe "during setup" do
+        before :each do
+          Puppet::Log.stubs(:newdestination)
+          Puppet::Log.stubs(:level=)
+          Puppet::Node::Facts.indirection.stubs(:terminus_class=)
+          Puppet::Node.stubs(:cache_class=)
+        end
+
+        it "should set facts terminus and cache class to yaml" do
+          Puppet::Node::Facts.indirection.expects(:terminus_class=).with(:yaml)
+          Puppet::Node::Facts.indirection.expects(:cache_class=).with(:yaml)
+
+          subject.clean('hostname')
+        end
+
+        it "should run in master mode" do
+          subject.clean('hostname')
+          $puppet_application_mode.name.should == :master
+        end
+
+        it "should set node cache as yaml" do
+          Puppet::Node.indirection.expects(:terminus_class=).with(:yaml)
+          Puppet::Node.indirection.expects(:cache_class=).with(:yaml)
+
+          subject.clean('hostname')
+        end
+
+        it "should manage the certs if the host is a CA" do
+           Puppet::SSL::CertificateAuthority.stubs(:ca?).returns(true)
+           Puppet::SSL::Host.expects(:ca_location=).with(:local)
+           subject.clean('hostname')
+        end
+
+        it "should not manage the certs if the host is not a CA" do
+           Puppet::SSL::CertificateAuthority.stubs(:ca?).returns(false)
+           Puppet::SSL::Host.expects(:ca_location=).with(:none)
+           subject.clean('hostname')
+        end
+      end
+
+      describe "when running" do
+
+        before :each do
+          @host = 'hostname'
+          Puppet.stubs(:info)
+          [ "cert", "cached_facts", "cached_node", "reports" ].each do |m|
+            subject.stubs("clean_#{m}".to_sym).with(@host)
+          end
+          subject.stubs(:clean_storeconfigs).with(@host,nil)
+        end
+
+        [ "cert", "cached_facts", "cached_node", "reports" ].each do |m|
+          it "should clean #{m.sub('_',' ')}" do
+            subject.expects("clean_#{m}".to_sym).with(@host)
+
+            subject.clean(@host)
+          end
+        end
+
+        it "should clean storeconfigs" do
+          subject.expects(:clean_storeconfigs).with(@host,nil)
+
+          subject.clean(@host)
+        end
+
+      end
+
+      describe "when cleaning certificate" do
+        before :each do
+          Puppet::SSL::Host.stubs(:destroy)
+          @ca = mock()
+          Puppet::SSL::CertificateAuthority.stubs(:instance).returns(@ca)
+        end
+
+        it "should send the :destroy order to the ca if we are a CA" do
+          Puppet::SSL::Host.stubs(:ca_location).returns(:local)
+          @ca.expects(:apply).with { |cert_mode,to| cert_mode == :revoke }
+          @ca.expects(:apply).with { |cert_mode,to| cert_mode == :destroy }
+
+          subject.clean_cert(@host)
+        end
+
+        it "should not destroy the certs if we are not a CA" do
+          Puppet::SSL::Host.stubs(:ca_location).returns(:none)
+          @ca.expects(:apply).never
+          subject.clean_cert(@host)
+        end
+      end
+
+      describe "when cleaning cached facts" do
+        it "should destroy facts" do
+          @host = 'node'
+          Puppet::Node::Facts.indirection.expects(:destroy).with(@host)
+
+          subject.clean_cached_facts(@host)
+        end
+      end
+
+      describe "when cleaning cached node" do
+        it "should destroy the cached node" do
+          Puppet::Node::Yaml.any_instance.expects(:destroy)
+          subject.clean_cached_node(@host)
+        end
+      end
+
+      describe "when cleaning archived reports" do
+        it "should tell the reports to remove themselves" do
+          Puppet::Transaction::Report.indirection.stubs(:destroy).with(@host)
+
+          subject.clean_reports(@host)
+        end
+      end
+
+      describe "when cleaning storeconfigs entries for host", :if => Puppet.features.rails? do
+        before :each do
+          # Stub this so we don't need access to the DB
+          require 'puppet/rails/host'
+
+          Puppet.stubs(:[]).with(:storeconfigs).returns(true)
+
+          Puppet::Rails.stubs(:connect)
+          @rails_node = stub_everything 'rails_node'
+          Puppet::Rails::Host.stubs(:find_by_name).returns(@rails_node)
+        end
+
+        it "should connect to the database" do
+          Puppet::Rails.expects(:connect)
+          subject.clean_storeconfigs(@host,false)
+        end
+
+        it "should find the right host entry" do
+          Puppet::Rails::Host.expects(:find_by_name).with(@host).returns(@rails_node)
+
+          subject.clean_storeconfigs(@host,false)
+        end
+
+        describe "without unexport" do
+          it "should remove the host and it's content" do
+            @rails_node.expects(:destroy)
+
+           subject.clean_storeconfigs(@host,false)
+          end
+        end
+
+        describe "with unexport" do
+          before :each do
+            @rails_node.stubs(:id).returns(1234)
+
+            @type = stub_everything 'type'
+            @type.stubs(:validattr?).with(:ensure).returns(true)
+
+            @ensure_name = stub_everything 'ensure_name', :id => 23453
+            Puppet::Rails::ParamName.stubs(:find_or_create_by_name).returns(@ensure_name)
+
+            @param_values = stub_everything 'param_values'
+            @resource = stub_everything 'resource', :param_values => @param_values, :restype => "File"
+            Puppet::Rails::Resource.stubs(:find).returns([@resource])
+          end
+
+          it "should find all resources" do
+            Puppet::Rails::Resource.expects(:find).with(:all, {:include => {:param_values => :param_name}, :conditions => ["exported=? AND host_id=?", true, 1234]}).returns([])
+
+            subject.clean_storeconfigs(@host,true)
+          end
+
+          describe "with an exported native type" do
+            before :each do
+              Puppet::Type.stubs(:type).returns(@type)
+              @type.expects(:validattr?).with(:ensure).returns(true)
+            end
+
+            it "should test a native type for ensure as an attribute" do
+              subject.clean_storeconfigs(@host,true)
+            end
+
+            it "should delete the old ensure parameter" do
+              ensure_param = stub 'ensure_param', :id => 12345, :line => 12
+              @param_values.stubs(:find).returns(ensure_param)
+              Puppet::Rails::ParamValue.expects(:delete).with(12345);
+              subject.clean_storeconfigs(@host,true)
+            end
+
+            it "should add an ensure => absent parameter" do
+              @param_values.expects(:create).with(:value => "absent",
+                                                       :line => 0,
+                                                       :param_name => @ensure_name)
+              subject.clean_storeconfigs(@host,true)
+            end
+          end
+
+          describe "with an exported definition" do
+            it "should try to lookup a definition and test it for the ensure argument" do
+              Puppet::Type.stubs(:type).returns(nil)
+              definition = stub_everything 'definition', :arguments => { 'ensure' => 'present' }
+              Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('',"File").returns(definition)
+              subject.clean_storeconfigs(@host,true)
+            end
+          end
+
+          it "should not unexport the resource of an unkown type" do
+            Puppet::Type.stubs(:type).returns(nil)
+            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('',"File").returns(nil)
+            Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
+            subject.clean_storeconfigs(@host)
+          end
+
+          it "should not unexport the resource of a not ensurable native type" do
+            Puppet::Type.stubs(:type).returns(@type)
+            @type.expects(:validattr?).with(:ensure).returns(false)
+            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('',"File").returns(nil)
+            Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
+            subject.clean_storeconfigs(@host,true)
+          end
+
+          it "should not unexport the resource of a not ensurable definition" do
+            Puppet::Type.stubs(:type).returns(nil)
+            definition = stub_everything 'definition', :arguments => { 'foobar' => 'someValue' }
+            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('',"File").returns(definition)
+            Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
+            subject.clean_storeconfigs(@host,true)
+          end
+        end
+      end
+    end
+  end
 end
diff --git a/spec/unit/indirector/report/processor_spec.rb b/spec/unit/indirector/report/processor_spec.rb
index fbc70a104..fda0d90fd 100755
--- a/spec/unit/indirector/report/processor_spec.rb
+++ b/spec/unit/indirector/report/processor_spec.rb
@@ -15,15 +15,21 @@ describe Puppet::Transaction::Report::Processor do
   it "should provide a method for saving reports" do
     Puppet::Transaction::Report::Processor.new.should respond_to(:save)
   end
+
+  it "should provide a method for cleaning reports" do
+    Puppet::Transaction::Report::Processor.new.should respond_to(:destroy)
+  end
+
 end
 
-describe Puppet::Transaction::Report::Processor, " when saving a report" do
+describe Puppet::Transaction::Report::Processor, " when processing a report" do
   before do
     Puppet.settings.stubs(:use)
     @reporter = Puppet::Transaction::Report::Processor.new
+    @request = stub 'request', :instance => mock("report"), :key => 'node'
   end
 
-  it "should not process the report if reports are set to 'none'" do
+  it "should not save the report if reports are set to 'none'" do
     Puppet::Reports.expects(:report).never
     Puppet[:reports] = 'none'
 
@@ -34,9 +40,24 @@ describe Puppet::Transaction::Report::Processor, " when saving a report" do
     @reporter.save(request)
   end
 
-  it "should process the report with each configured report type" do
+  it "should save the report with each configured report type" do
     Puppet.settings.stubs(:value).with(:reports).returns("one,two")
     @reporter.send(:reports).should == %w{one two}
+
+    Puppet::Reports.expects(:report).with('one')
+    Puppet::Reports.expects(:report).with('two')
+
+    @reporter.save(@request)
+  end
+
+  it "should destroy reports for each processor that responds to destroy" do
+    Puppet.settings.stubs(:value).with(:reports).returns("http,store")
+    http_report = mock()
+    store_report = mock()
+    store_report.expects(:destroy).with(@request.key)
+    Puppet::Reports.expects(:report).with('http').returns(http_report)
+    Puppet::Reports.expects(:report).with('store').returns(store_report)
+    @reporter.destroy(@request)
   end
 end
 
diff --git a/spec/unit/indirector/yaml_spec.rb b/spec/unit/indirector/yaml_spec.rb
index c43dbcaf6..29f6d466f 100755
--- a/spec/unit/indirector/yaml_spec.rb
+++ b/spec/unit/indirector/yaml_spec.rb
@@ -154,5 +154,23 @@ describe Puppet::Indirector::Yaml, " when choosing file location" do
       Dir.expects(:glob).with(:glob).returns []
       @store.search(@request).should == []
     end
+
+    describe Puppet::Indirector::Yaml, " when destroying" do
+      it "should unlink the right yaml file if it exists" do
+        path = File.join("/what/ever", @store.class.indirection_name.to_s, @request.key.to_s + ".yaml")
+        File.expects(:exists?).with(path).returns true
+        File.expects(:unlink).with(path)
+
+        @store.destroy(@request)
+      end
+
+      it "should not unlink the yaml file if it does not exists" do
+        path = File.join("/what/ever", @store.class.indirection_name.to_s, @request.key.to_s + ".yaml")
+        File.expects(:exists?).with(path).returns false
+        File.expects(:unlink).with(path).never
+
+        @store.destroy(@request)
+      end
+    end
   end
 end
-- 
cgit 


From ccd622a96dfe5871689f5b2f059c11aef3caf3b4 Mon Sep 17 00:00:00 2001
From: Pieter van de Bruggen <pieter@puppetlabs.com>
Date: Wed, 27 Jul 2011 14:10:02 -0700
Subject: (#1886) Clean up `node clean` for merge.

This includes various style changes, and assorted fixes to testing.

Paired-With: Matt Robinson
---
 lib/puppet/face/ca.rb                         |  30 +++---
 lib/puppet/face/node/clean.rb                 | 111 +++++++++++-----------
 spec/integration/node/facts_spec.rb           |   2 +-
 spec/unit/face/node_spec.rb                   | 132 +++++++++++++-------------
 spec/unit/indirector/report/processor_spec.rb |   2 +-
 5 files changed, 139 insertions(+), 138 deletions(-)

diff --git a/lib/puppet/face/ca.rb b/lib/puppet/face/ca.rb
index e643530f0..00591d637 100644
--- a/lib/puppet/face/ca.rb
+++ b/lib/puppet/face/ca.rb
@@ -6,21 +6,21 @@ Puppet::Face.define(:ca, '0.1.0') do
 
   summary "Local Puppet Certificate Authority management."
 
-  description <<TEXT
-This provides local management of the Puppet Certificate Authority.
+  description <<-TEXT
+    This provides local management of the Puppet Certificate Authority.
 
-You can use this subcommand to sign outstanding certificate requests, list
-and manage local certificates, and inspect the state of the CA.
-TEXT
+    You can use this subcommand to sign outstanding certificate requests, list
+    and manage local certificates, and inspect the state of the CA.
+  TEXT
 
   action :list do
     summary "List certificates and/or certificate requests."
 
-    description <<-end
-This will list the current certificates and certificate signing requests
-in the Puppet CA.  You will also get the fingerprint, and any certificate
-verification failure reported.
-    end
+    description <<-TEXT
+      This will list the current certificates and certificate signing requests
+      in the Puppet CA.  You will also get the fingerprint, and any certificate
+      verification failure reported.
+    TEXT
 
     option "--[no-]all" do
       summary "Include all certificates and requests."
@@ -37,12 +37,12 @@ verification failure reported.
     option "--subject PATTERN" do
       summary "Only list if the subject matches PATTERN."
 
-      description <<TEXT
-Only include certificates or requests where subject matches PATTERN.
+      description <<-TEXT
+        Only include certificates or requests where subject matches PATTERN.
 
-PATTERN is interpreted as a regular expression, allowing complex
-filtering of the content.
-TEXT
+        PATTERN is interpreted as a regular expression, allowing complex
+        filtering of the content.
+      TEXT
     end
 
     when_invoked do |options|
diff --git a/lib/puppet/face/node/clean.rb b/lib/puppet/face/node/clean.rb
index 10d6239ba..a4df1bfaf 100644
--- a/lib/puppet/face/node/clean.rb
+++ b/lib/puppet/face/node/clean.rb
@@ -1,29 +1,29 @@
-Puppet::Indirector::Face.define(:node, '0.0.1') do
+Puppet::Face.define(:node, '0.0.1') do
   action(:clean) do
     option "--[no-]unexport" do
       summary "Unexport exported resources"
     end
-  
+
     summary "Clean up everything a puppetmaster knows about a node"
-    
     arguments "<host1> [<host2> ...]"
-
     description <<-EOT
-This includes
-
- * Signed certificates ($vardir/ssl/ca/signed/node.domain.pem)
- * Cached facts ($vardir/yaml/facts/node.domain.yaml)
- * Cached node stuff ($vardir/yaml/node/node.domain.yaml)
- * Reports ($vardir/reports/node.domain)
- * Stored configs: it can either remove all data from an host in your storedconfig
- database, or with --unexport turn every exported resource supporting ensure to absent
- so that any other host checking out their config can remove those exported configurations.
-
-This will unexport exported resources of a
-host, so that consumers of these resources can remove the exported
-resources and we will safely remove the node from our
-infrastructure.
-EOT
+      This includes
+
+       * Signed certificates ($vardir/ssl/ca/signed/node.domain.pem)
+       * Cached facts ($vardir/yaml/facts/node.domain.yaml)
+       * Cached node stuff ($vardir/yaml/node/node.domain.yaml)
+       * Reports ($vardir/reports/node.domain)
+       * Stored configs: it can either remove all data from an host in your
+       storedconfig database, or with --unexport turn every exported resource
+       supporting ensure to absent so that any other host checking out their
+       config can remove those exported configurations.
+
+      This will unexport exported resources of a
+      host, so that consumers of these resources can remove the exported
+      resources and we will safely remove the node from our
+      infrastructure.
+    EOT
+
     when_invoked do |*args|
       nodes = args[0..-2]
       options = args.last
@@ -39,82 +39,78 @@ EOT
       else
         Puppet::SSL::Host.ca_location = :none
       end
-  
+
       Puppet::Node::Facts.indirection.terminus_class = :yaml
       Puppet::Node::Facts.indirection.cache_class = :yaml
       Puppet::Node.indirection.terminus_class = :yaml
       Puppet::Node.indirection.cache_class = :yaml
 
-      begin
-        nodes.each do |node|
-          node = node.downcase
-          clean_cert(node)
-          clean_cached_facts(node)
-          clean_cached_node(node)
-          clean_reports(node)
-          clean_storeconfigs(node,options[:unexport])
-        end
-      rescue => detail
-        puts detail.backtrace if Puppet[:trace]
-        puts detail.to_s
-      end
+      nodes.each { |node| cleanup(node.downcase, options[:unexport]) }
     end
   end
-  
+
+  def cleanup(node, unexport)
+    clean_cert(node)
+    clean_cached_facts(node)
+    clean_cached_node(node)
+    clean_reports(node)
+    clean_storeconfigs(node, unexport)
+  end
+
   # clean signed cert for +host+
   def clean_cert(node)
-    if Puppet::SSL::Host.ca_location == :local
-      ca.apply(:revoke, :to => [node])
-      ca.apply(:destroy, :to => [node])
-      Puppet.info "%s certificates removed from ca" % node
+    if Puppet::SSL::CertificateAuthority.ca?
+      Puppet::Face[:ca, :current].revoke(node)
+      Puppet::Face[:ca, :current].destroy(node)
+      Puppet.info "#{node} certificates removed from ca"
     else
-      Puppet.info "Not managing %s certs as this host is not a CA" % node
+      Puppet.info "Not managing #{node} certs as this host is not a CA"
     end
   end
 
   # clean facts for +host+
   def clean_cached_facts(node)
     Puppet::Node::Facts.indirection.destroy(node)
-    Puppet.info "%s's facts removed" % node
+    Puppet.info "#{node}'s facts removed"
   end
 
   # clean cached node +host+
   def clean_cached_node(node)
     Puppet::Node.indirection.destroy(node)
-    Puppet.info "%s's cached node removed" % node
+    Puppet.info "#{node}'s cached node removed"
   end
 
   # clean node reports for +host+
   def clean_reports(node)
     Puppet::Transaction::Report.indirection.destroy(node)
-    Puppet.info "%s's reports removed" % node
+    Puppet.info "#{node}'s reports removed"
   end
 
   # clean storeconfig for +node+
-  def clean_storeconfigs(node,do_unexport=false)
+  def clean_storeconfigs(node, do_unexport=false)
     return unless Puppet[:storeconfigs] && Puppet.features.rails?
     require 'puppet/rails'
     Puppet::Rails.connect
     unless rails_node = Puppet::Rails::Host.find_by_name(node)
-      Puppet.notice "No entries found for %s in storedconfigs." % node
+      Puppet.notice "No entries found for #{node} in storedconfigs."
       return
     end
 
     if do_unexport
       unexport(rails_node)
-      Puppet.notice "Force %s's exported resources to absent" % node
+      Puppet.notice "Force #{node}'s exported resources to absent"
       Puppet.warning "Please wait until all other hosts have checked out their configuration before finishing the cleanup with:"
       Puppet.warning "$ puppet node clean #{node}"
     else
       rails_node.destroy
-      Puppet.notice "%s storeconfigs removed" % node
+      Puppet.notice "#{node} storeconfigs removed"
     end
   end
 
   def unexport(node)
     # fetch all exported resource
     query = {:include => {:param_values => :param_name}}
-    query[:conditions] = ["exported=? AND host_id=?", true, node.id]
+    query[:conditions] = [ "exported=? AND host_id=?", true, node.id ]
     Puppet::Rails::Resource.find(:all, query).each do |resource|
       if type_is_ensurable(resource)
         line = 0
@@ -122,7 +118,7 @@ EOT
 
         if ensure_param = resource.param_values.find(
           :first,
-          :conditions => [ 'param_name_id = ?', param_name.id]
+          :conditions => [ 'param_name_id = ?', param_name.id ]
         )
           line = ensure_param.line.to_i
           Puppet::Rails::ParamValue.delete(ensure_param.id);
@@ -134,21 +130,22 @@ EOT
           :line => line,
           :param_name => param_name
         )
-        Puppet.info("%s has been marked as \"absent\"" % resource.name)
+        Puppet.info("#{resource.name} has been marked as \"absent\"")
       end
     end
   end
 
-  def ca
-    @ca ||= Puppet::SSL::CertificateAuthority.instance
-  end
-
   def environment
-    @environemnt ||= Puppet::Node::Environment.new
+    @environment ||= Puppet::Node::Environment.new
   end
 
   def type_is_ensurable(resource)
-      (type=Puppet::Type.type(resource.restype)) && type.validattr?(:ensure) || \
-        (type = environment.known_resource_types.find_definition('',resource.restype)) && type.arguments.keys.include?('ensure')
+    if (type = Puppet::Type.type(resource.restype)) && type.validattr?(:ensure)
+      return true
+    else
+      type = environment.known_resource_types.find_definition('', resource.restype)
+      return true if type && type.arguments.keys.include?('ensure')
+    end
+    return false
   end
-end
\ No newline at end of file
+end
diff --git a/spec/integration/node/facts_spec.rb b/spec/integration/node/facts_spec.rb
index f54d7f9aa..e87a0bdeb 100755
--- a/spec/integration/node/facts_spec.rb
+++ b/spec/integration/node/facts_spec.rb
@@ -7,7 +7,7 @@ require 'spec_helper'
 
 describe Puppet::Node::Facts do
   describe "when using the indirector" do
-    after { Puppet::Util::Cacher.expire }
+    after(:each) { Puppet::Util::Cacher.expire }
 
     it "should expire any cached node instances when it is saved" do
       Puppet::Node::Facts.indirection.stubs(:terminus_class).returns :yaml
diff --git a/spec/unit/face/node_spec.rb b/spec/unit/face/node_spec.rb
index 4ba1d134c..7151353a0 100755
--- a/spec/unit/face/node_spec.rb
+++ b/spec/unit/face/node_spec.rb
@@ -3,9 +3,42 @@ require 'spec_helper'
 require 'puppet/face'
 
 describe Puppet::Face[:node, '0.0.1'] do
-  it "REVISIT: really should have some tests"
+  describe '#cleanup' do
+    it "should clean everything" do
+      {
+        "cert"         => ['hostname'],
+        "cached_facts" => ['hostname'],
+        "cached_node"  => ['hostname'],
+        "reports"      => ['hostname'],
+        "storeconfigs" => ['hostname', :unexport]
+      }.each { |k, v| subject.expects("clean_#{k}".to_sym).with(*v) }
+      subject.cleanup('hostname', :unexport)
+    end
+  end
+
+  describe 'when running #clean' do
+    before :each do
+      Puppet::Node::Facts.indirection.stubs(:terminus_class=)
+      Puppet::Node::Facts.indirection.stubs(:cache_class=)
+      Puppet::Node.stubs(:terminus_class=)
+      Puppet::Node.stubs(:cache_class=)
+    end
+
+    it 'should invoke #cleanup' do
+      subject.expects(:cleanup).with('hostname', nil)
+      subject.clean('hostname')
+    end
+  end
 
   describe "clean action" do
+    before :each do
+      Puppet::Node::Facts.indirection.stubs(:terminus_class=)
+      Puppet::Node::Facts.indirection.stubs(:cache_class=)
+      Puppet::Node.stubs(:terminus_class=)
+      Puppet::Node.stubs(:cache_class=)
+      subject.stubs(:cleanup)
+    end
+
     it "should have a clean action" do
       subject.should be_action :clean
     end
@@ -19,8 +52,13 @@ describe Puppet::Face[:node, '0.0.1'] do
     end
 
     it "should accept more than one node name" do
-      expect { subject.clean(['hostname', 'hostname2'],{}) }.should_not raise_error
-      expect { subject.clean(['hostname', 'hostname2', 'hostname3'],{:unexport => true}) }.should_not raise_error
+      expect do
+        subject.clean('hostname', 'hostname2', {})
+      end.should_not raise_error
+
+      expect do
+        subject.clean('hostname', 'hostname2', 'hostname3', { :unexport => true })
+      end.should_not raise_error
     end
 
     it "should accept the option --unexport" do
@@ -29,20 +67,13 @@ describe Puppet::Face[:node, '0.0.1'] do
     end
 
     context "clean action" do
-      subject { Puppet::Face[:node, 'current'] }
+      subject { Puppet::Face[:node, :current] }
       before :each do
         Puppet::Util::Log.stubs(:newdestination)
         Puppet::Util::Log.stubs(:level=)
       end
 
       describe "during setup" do
-        before :each do
-          Puppet::Log.stubs(:newdestination)
-          Puppet::Log.stubs(:level=)
-          Puppet::Node::Facts.indirection.stubs(:terminus_class=)
-          Puppet::Node.stubs(:cache_class=)
-        end
-
         it "should set facts terminus and cache class to yaml" do
           Puppet::Node::Facts.indirection.expects(:terminus_class=).with(:yaml)
           Puppet::Node::Facts.indirection.expects(:cache_class=).with(:yaml)
@@ -63,43 +94,16 @@ describe Puppet::Face[:node, '0.0.1'] do
         end
 
         it "should manage the certs if the host is a CA" do
-           Puppet::SSL::CertificateAuthority.stubs(:ca?).returns(true)
-           Puppet::SSL::Host.expects(:ca_location=).with(:local)
-           subject.clean('hostname')
+          Puppet::SSL::CertificateAuthority.stubs(:ca?).returns(true)
+          Puppet::SSL::Host.expects(:ca_location=).with(:local)
+          subject.clean('hostname')
         end
 
         it "should not manage the certs if the host is not a CA" do
-           Puppet::SSL::CertificateAuthority.stubs(:ca?).returns(false)
-           Puppet::SSL::Host.expects(:ca_location=).with(:none)
-           subject.clean('hostname')
-        end
-      end
-
-      describe "when running" do
-
-        before :each do
-          @host = 'hostname'
-          Puppet.stubs(:info)
-          [ "cert", "cached_facts", "cached_node", "reports" ].each do |m|
-            subject.stubs("clean_#{m}".to_sym).with(@host)
-          end
-          subject.stubs(:clean_storeconfigs).with(@host,nil)
-        end
-
-        [ "cert", "cached_facts", "cached_node", "reports" ].each do |m|
-          it "should clean #{m.sub('_',' ')}" do
-            subject.expects("clean_#{m}".to_sym).with(@host)
-
-            subject.clean(@host)
-          end
-        end
-
-        it "should clean storeconfigs" do
-          subject.expects(:clean_storeconfigs).with(@host,nil)
-
-          subject.clean(@host)
+          Puppet::SSL::CertificateAuthority.stubs(:ca?).returns(false)
+          Puppet::SSL::Host.expects(:ca_location=).with(:none)
+          subject.clean('hostname')
         end
-
       end
 
       describe "when cleaning certificate" do
@@ -110,16 +114,16 @@ describe Puppet::Face[:node, '0.0.1'] do
         end
 
         it "should send the :destroy order to the ca if we are a CA" do
-          Puppet::SSL::Host.stubs(:ca_location).returns(:local)
-          @ca.expects(:apply).with { |cert_mode,to| cert_mode == :revoke }
-          @ca.expects(:apply).with { |cert_mode,to| cert_mode == :destroy }
-
+          Puppet::SSL::CertificateAuthority.stubs(:ca?).returns(true)
+          @ca.expects(:revoke).with(@host)
+          @ca.expects(:destroy).with(@host)
           subject.clean_cert(@host)
         end
 
         it "should not destroy the certs if we are not a CA" do
-          Puppet::SSL::Host.stubs(:ca_location).returns(:none)
-          @ca.expects(:apply).never
+          Puppet::SSL::CertificateAuthority.stubs(:ca?).returns(false)
+          @ca.expects(:revoke).never
+          @ca.expects(:destroy).never
           subject.clean_cert(@host)
         end
       end
@@ -162,20 +166,20 @@ describe Puppet::Face[:node, '0.0.1'] do
 
         it "should connect to the database" do
           Puppet::Rails.expects(:connect)
-          subject.clean_storeconfigs(@host,false)
+          subject.clean_storeconfigs(@host, false)
         end
 
         it "should find the right host entry" do
           Puppet::Rails::Host.expects(:find_by_name).with(@host).returns(@rails_node)
 
-          subject.clean_storeconfigs(@host,false)
+          subject.clean_storeconfigs(@host, false)
         end
 
         describe "without unexport" do
           it "should remove the host and it's content" do
             @rails_node.expects(:destroy)
 
-           subject.clean_storeconfigs(@host,false)
+           subject.clean_storeconfigs(@host, false)
           end
         end
 
@@ -197,7 +201,7 @@ describe Puppet::Face[:node, '0.0.1'] do
           it "should find all resources" do
             Puppet::Rails::Resource.expects(:find).with(:all, {:include => {:param_values => :param_name}, :conditions => ["exported=? AND host_id=?", true, 1234]}).returns([])
 
-            subject.clean_storeconfigs(@host,true)
+            subject.clean_storeconfigs(@host, true)
           end
 
           describe "with an exported native type" do
@@ -207,21 +211,21 @@ describe Puppet::Face[:node, '0.0.1'] do
             end
 
             it "should test a native type for ensure as an attribute" do
-              subject.clean_storeconfigs(@host,true)
+              subject.clean_storeconfigs(@host, true)
             end
 
             it "should delete the old ensure parameter" do
               ensure_param = stub 'ensure_param', :id => 12345, :line => 12
               @param_values.stubs(:find).returns(ensure_param)
               Puppet::Rails::ParamValue.expects(:delete).with(12345);
-              subject.clean_storeconfigs(@host,true)
+              subject.clean_storeconfigs(@host, true)
             end
 
             it "should add an ensure => absent parameter" do
               @param_values.expects(:create).with(:value => "absent",
                                                        :line => 0,
                                                        :param_name => @ensure_name)
-              subject.clean_storeconfigs(@host,true)
+              subject.clean_storeconfigs(@host, true)
             end
           end
 
@@ -229,14 +233,14 @@ describe Puppet::Face[:node, '0.0.1'] do
             it "should try to lookup a definition and test it for the ensure argument" do
               Puppet::Type.stubs(:type).returns(nil)
               definition = stub_everything 'definition', :arguments => { 'ensure' => 'present' }
-              Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('',"File").returns(definition)
-              subject.clean_storeconfigs(@host,true)
+              Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(definition)
+              subject.clean_storeconfigs(@host, true)
             end
           end
 
           it "should not unexport the resource of an unkown type" do
             Puppet::Type.stubs(:type).returns(nil)
-            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('',"File").returns(nil)
+            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(nil)
             Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
             subject.clean_storeconfigs(@host)
           end
@@ -244,17 +248,17 @@ describe Puppet::Face[:node, '0.0.1'] do
           it "should not unexport the resource of a not ensurable native type" do
             Puppet::Type.stubs(:type).returns(@type)
             @type.expects(:validattr?).with(:ensure).returns(false)
-            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('',"File").returns(nil)
+            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(nil)
             Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
-            subject.clean_storeconfigs(@host,true)
+            subject.clean_storeconfigs(@host, true)
           end
 
           it "should not unexport the resource of a not ensurable definition" do
             Puppet::Type.stubs(:type).returns(nil)
             definition = stub_everything 'definition', :arguments => { 'foobar' => 'someValue' }
-            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('',"File").returns(definition)
+            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(definition)
             Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
-            subject.clean_storeconfigs(@host,true)
+            subject.clean_storeconfigs(@host, true)
           end
         end
       end
diff --git a/spec/unit/indirector/report/processor_spec.rb b/spec/unit/indirector/report/processor_spec.rb
index fda0d90fd..c64cc7eff 100755
--- a/spec/unit/indirector/report/processor_spec.rb
+++ b/spec/unit/indirector/report/processor_spec.rb
@@ -26,7 +26,7 @@ describe Puppet::Transaction::Report::Processor, " when processing a report" do
   before do
     Puppet.settings.stubs(:use)
     @reporter = Puppet::Transaction::Report::Processor.new
-    @request = stub 'request', :instance => mock("report"), :key => 'node'
+    @request = stub 'request', :instance => stub("report", :host => 'hostname'), :key => 'node'
   end
 
   it "should not save the report if reports are set to 'none'" do
-- 
cgit 


From bff817c5bea1a1b298a81d5aa4f28c2789125286 Mon Sep 17 00:00:00 2001
From: Pieter van de Bruggen <pieter@puppetlabs.com>
Date: Thu, 28 Jul 2011 13:12:58 -0700
Subject: (Maint.) Fix spec failures related to leaking state.

The `node clean` code has introduced a systematic change in
state which is not uniformly protected against by the tests.

As these order dependent failures arise, we should refactor
the tests to be more robust.

Reviewed-By: Matt Robinson
---
 spec/unit/application/apply_spec.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/spec/unit/application/apply_spec.rb b/spec/unit/application/apply_spec.rb
index c9555157c..489f4db36 100755
--- a/spec/unit/application/apply_spec.rb
+++ b/spec/unit/application/apply_spec.rb
@@ -134,7 +134,9 @@ describe Puppet::Application::Apply do
         Puppet[:postrun_command] = ''
 
         Puppet::Node::Facts.indirection.terminus_class = :memory
+        Puppet::Node::Facts.indirection.cache_class = :memory
         Puppet::Node.indirection.terminus_class = :memory
+        Puppet::Node.indirection.cache_class = :memory
 
         @facts = Puppet::Node::Facts.new(Puppet[:node_name_value])
         Puppet::Node::Facts.indirection.save(@facts)
-- 
cgit 


From 023d9597b9895f57fda05dc79adad41684179eb2 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Thu, 28 Jul 2011 15:08:00 -0700
Subject: (#8690) Accept 'global' options in Puppet Faces

When we introduced verification of options, we forgot to handle the case that
global options from the Puppet settings system could be passed to the face.
This, in turn, means that the system would fail if you used any of those.

This remediates that, and now these work as expected.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/interface/action.rb     | 2 ++
 spec/unit/interface/action_spec.rb | 8 +++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index bd47a36ea..60ddb2ca3 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -269,6 +269,8 @@ WRAPPER
         else
           result[canonical] = original[name]
         end
+      elsif Puppet.settings.include? name
+        result[name] = original[name]
       else
         unknown << name
       end
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index c3f08e817..6b68eb149 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -552,7 +552,7 @@ describe Puppet::Interface::Action do
   context "#validate_and_clean" do
     subject do
       Puppet::Interface.new(:validate_args, '1.0.0') do
-        script :test do |options| true end
+        script :test do |options| options end
       end
     end
 
@@ -576,6 +576,12 @@ describe Puppet::Interface::Action do
       expect { subject.test :unknown => true, :unseen => false }.
         to raise_error ArgumentError, /Unknown options passed: unknown, unseen/
     end
+
+    it "should accept 'global' options from settings" do
+      expect {
+        subject.test(:certname => "true").should == { :certname => "true" }
+      }.not_to raise_error
+    end
   end
 
   context "default option values" do
-- 
cgit 


From 38801ddbd3036b45d35e48e444d1e7d566b5db47 Mon Sep 17 00:00:00 2001
From: Pieter van de Bruggen <pieter@puppetlabs.com>
Date: Thu, 28 Jul 2011 15:34:47 -0700
Subject: (Maint.) Disable cleaning of storeconfigs.

This feature (and the corresponding tests) were causing intermittent
failures which we were unable to trace.  We will reintroduce this
behavior when we can do so without test fragility.

Reviewed-By: Matt Robinson
---
 lib/puppet/face/node/clean.rb |   5 +-
 spec/unit/face/node_spec.rb   | 222 +++++++++++++++++++++---------------------
 2 files changed, 115 insertions(+), 112 deletions(-)

diff --git a/lib/puppet/face/node/clean.rb b/lib/puppet/face/node/clean.rb
index a4df1bfaf..d2852de04 100644
--- a/lib/puppet/face/node/clean.rb
+++ b/lib/puppet/face/node/clean.rb
@@ -54,7 +54,10 @@ Puppet::Face.define(:node, '0.0.1') do
     clean_cached_facts(node)
     clean_cached_node(node)
     clean_reports(node)
-    clean_storeconfigs(node, unexport)
+
+    # This is roughly functional, but seems to introduce order-dependent test
+    # failures; this can be re-added when those issues are resolved.
+    # clean_storeconfigs(node, unexport)
   end
 
   # clean signed cert for +host+
diff --git a/spec/unit/face/node_spec.rb b/spec/unit/face/node_spec.rb
index 7151353a0..6f6edc6cc 100755
--- a/spec/unit/face/node_spec.rb
+++ b/spec/unit/face/node_spec.rb
@@ -10,7 +10,9 @@ describe Puppet::Face[:node, '0.0.1'] do
         "cached_facts" => ['hostname'],
         "cached_node"  => ['hostname'],
         "reports"      => ['hostname'],
-        "storeconfigs" => ['hostname', :unexport]
+
+        # Support for cleaning storeconfigs has been temporarily suspended.
+        # "storeconfigs" => ['hostname', :unexport]
       }.each { |k, v| subject.expects("clean_#{k}".to_sym).with(*v) }
       subject.cleanup('hostname', :unexport)
     end
@@ -152,116 +154,114 @@ describe Puppet::Face[:node, '0.0.1'] do
         end
       end
 
-      describe "when cleaning storeconfigs entries for host", :if => Puppet.features.rails? do
-        before :each do
-          # Stub this so we don't need access to the DB
-          require 'puppet/rails/host'
-
-          Puppet.stubs(:[]).with(:storeconfigs).returns(true)
-
-          Puppet::Rails.stubs(:connect)
-          @rails_node = stub_everything 'rails_node'
-          Puppet::Rails::Host.stubs(:find_by_name).returns(@rails_node)
-        end
-
-        it "should connect to the database" do
-          Puppet::Rails.expects(:connect)
-          subject.clean_storeconfigs(@host, false)
-        end
-
-        it "should find the right host entry" do
-          Puppet::Rails::Host.expects(:find_by_name).with(@host).returns(@rails_node)
-
-          subject.clean_storeconfigs(@host, false)
-        end
-
-        describe "without unexport" do
-          it "should remove the host and it's content" do
-            @rails_node.expects(:destroy)
-
-           subject.clean_storeconfigs(@host, false)
-          end
-        end
-
-        describe "with unexport" do
-          before :each do
-            @rails_node.stubs(:id).returns(1234)
-
-            @type = stub_everything 'type'
-            @type.stubs(:validattr?).with(:ensure).returns(true)
-
-            @ensure_name = stub_everything 'ensure_name', :id => 23453
-            Puppet::Rails::ParamName.stubs(:find_or_create_by_name).returns(@ensure_name)
-
-            @param_values = stub_everything 'param_values'
-            @resource = stub_everything 'resource', :param_values => @param_values, :restype => "File"
-            Puppet::Rails::Resource.stubs(:find).returns([@resource])
-          end
-
-          it "should find all resources" do
-            Puppet::Rails::Resource.expects(:find).with(:all, {:include => {:param_values => :param_name}, :conditions => ["exported=? AND host_id=?", true, 1234]}).returns([])
-
-            subject.clean_storeconfigs(@host, true)
-          end
-
-          describe "with an exported native type" do
-            before :each do
-              Puppet::Type.stubs(:type).returns(@type)
-              @type.expects(:validattr?).with(:ensure).returns(true)
-            end
-
-            it "should test a native type for ensure as an attribute" do
-              subject.clean_storeconfigs(@host, true)
-            end
-
-            it "should delete the old ensure parameter" do
-              ensure_param = stub 'ensure_param', :id => 12345, :line => 12
-              @param_values.stubs(:find).returns(ensure_param)
-              Puppet::Rails::ParamValue.expects(:delete).with(12345);
-              subject.clean_storeconfigs(@host, true)
-            end
-
-            it "should add an ensure => absent parameter" do
-              @param_values.expects(:create).with(:value => "absent",
-                                                       :line => 0,
-                                                       :param_name => @ensure_name)
-              subject.clean_storeconfigs(@host, true)
-            end
-          end
-
-          describe "with an exported definition" do
-            it "should try to lookup a definition and test it for the ensure argument" do
-              Puppet::Type.stubs(:type).returns(nil)
-              definition = stub_everything 'definition', :arguments => { 'ensure' => 'present' }
-              Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(definition)
-              subject.clean_storeconfigs(@host, true)
-            end
-          end
-
-          it "should not unexport the resource of an unkown type" do
-            Puppet::Type.stubs(:type).returns(nil)
-            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(nil)
-            Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
-            subject.clean_storeconfigs(@host)
-          end
-
-          it "should not unexport the resource of a not ensurable native type" do
-            Puppet::Type.stubs(:type).returns(@type)
-            @type.expects(:validattr?).with(:ensure).returns(false)
-            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(nil)
-            Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
-            subject.clean_storeconfigs(@host, true)
-          end
-
-          it "should not unexport the resource of a not ensurable definition" do
-            Puppet::Type.stubs(:type).returns(nil)
-            definition = stub_everything 'definition', :arguments => { 'foobar' => 'someValue' }
-            Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(definition)
-            Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
-            subject.clean_storeconfigs(@host, true)
-          end
-        end
-      end
+      # describe "when cleaning storeconfigs entries for host", :if => Puppet.features.rails? do
+      #   before :each do
+      #     # Stub this so we don't need access to the DB
+      #     require 'puppet/rails/host'
+      #
+      #     Puppet.stubs(:[]).with(:storeconfigs).returns(true)
+      #
+      #     Puppet::Rails.stubs(:connect)
+      #     @rails_node = stub_everything 'rails_node'
+      #     Puppet::Rails::Host.stubs(:find_by_name).returns(@rails_node)
+      #   end
+      #
+      #   it "should connect to the database" do
+      #     Puppet::Rails.expects(:connect)
+      #     subject.clean_storeconfigs(@host, false)
+      #   end
+      #
+      #   it "should find the right host entry" do
+      #     Puppet::Rails::Host.expects(:find_by_name).with(@host).returns(@rails_node)
+      #     subject.clean_storeconfigs(@host, false)
+      #   end
+      #
+      #   describe "without unexport" do
+      #     it "should remove the host and it's content" do
+      #       @rails_node.expects(:destroy)
+      #       subject.clean_storeconfigs(@host, false)
+      #     end
+      #   end
+      #
+      #   describe "with unexport" do
+      #     before :each do
+      #       @rails_node.stubs(:id).returns(1234)
+      #
+      #       @type = stub_everything 'type'
+      #       @type.stubs(:validattr?).with(:ensure).returns(true)
+      #
+      #       @ensure_name = stub_everything 'ensure_name', :id => 23453
+      #       Puppet::Rails::ParamName.stubs(:find_or_create_by_name).returns(@ensure_name)
+      #
+      #       @param_values = stub_everything 'param_values'
+      #       @resource = stub_everything 'resource', :param_values => @param_values, :restype => "File"
+      #       Puppet::Rails::Resource.stubs(:find).returns([@resource])
+      #     end
+      #
+      #     it "should find all resources" do
+      #       Puppet::Rails::Resource.expects(:find).with(:all, {:include => {:param_values => :param_name}, :conditions => ["exported=? AND host_id=?", true, 1234]}).returns([])
+      #
+      #       subject.clean_storeconfigs(@host, true)
+      #     end
+      #
+      #     describe "with an exported native type" do
+      #       before :each do
+      #         Puppet::Type.stubs(:type).returns(@type)
+      #         @type.expects(:validattr?).with(:ensure).returns(true)
+      #       end
+      #
+      #       it "should test a native type for ensure as an attribute" do
+      #         subject.clean_storeconfigs(@host, true)
+      #       end
+      #
+      #       it "should delete the old ensure parameter" do
+      #         ensure_param = stub 'ensure_param', :id => 12345, :line => 12
+      #         @param_values.stubs(:find).returns(ensure_param)
+      #         Puppet::Rails::ParamValue.expects(:delete).with(12345);
+      #         subject.clean_storeconfigs(@host, true)
+      #       end
+      #
+      #       it "should add an ensure => absent parameter" do
+      #         @param_values.expects(:create).with(:value => "absent",
+      #                                             :line => 0,
+      #                                             :param_name => @ensure_name)
+      #         subject.clean_storeconfigs(@host, true)
+      #       end
+      #     end
+      #
+      #     describe "with an exported definition" do
+      #       it "should try to lookup a definition and test it for the ensure argument" do
+      #         Puppet::Type.stubs(:type).returns(nil)
+      #         definition = stub_everything 'definition', :arguments => { 'ensure' => 'present' }
+      #         Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(definition)
+      #         subject.clean_storeconfigs(@host, true)
+      #       end
+      #     end
+      #
+      #     it "should not unexport the resource of an unknown type" do
+      #       Puppet::Type.stubs(:type).returns(nil)
+      #       Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(nil)
+      #       Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
+      #       subject.clean_storeconfigs(@host)
+      #     end
+      #
+      #     it "should not unexport the resource of a not ensurable native type" do
+      #       Puppet::Type.stubs(:type).returns(@type)
+      #       @type.expects(:validattr?).with(:ensure).returns(false)
+      #       Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(nil)
+      #       Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
+      #       subject.clean_storeconfigs(@host, true)
+      #     end
+      #
+      #     it "should not unexport the resource of a not ensurable definition" do
+      #       Puppet::Type.stubs(:type).returns(nil)
+      #       definition = stub_everything 'definition', :arguments => { 'foobar' => 'someValue' }
+      #       Puppet::Resource::TypeCollection.any_instance.expects(:find_definition).with('', "File").returns(definition)
+      #       Puppet::Rails::ParamName.expects(:find_or_create_by_name).never
+      #       subject.clean_storeconfigs(@host, true)
+      #     end
+      #   end
+      # end
     end
   end
 end
-- 
cgit 


From 94f0b93b6065d1818f0f3b99d12d651655247c30 Mon Sep 17 00:00:00 2001
From: Matt Robinson <matt@puppetlabs.com>
Date: Fri, 29 Jul 2011 12:29:51 -0700
Subject: (#8704) Give better errors for invalid fileserver.conf

If you tried to just put an allow or deny line in the fileserver.conf
without a mount point, you got a really confusing error message:

    lib/puppet/network/handler/fileserver.rb:285:in `readconfig': undefined method `info' for nil:NilClass (NoMethodError)

Now instead we give an error saying no mount point was specified.

Reviewed-by: Josh Cooper <josh@puppetlabs.com>
---
 lib/puppet/network/handler/fileserver.rb     |  3 +++
 spec/unit/network/handler/fileserver_spec.rb | 32 ++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/lib/puppet/network/handler/fileserver.rb b/lib/puppet/network/handler/fileserver.rb
index 5b4b17a32..5da4cedef 100755
--- a/lib/puppet/network/handler/fileserver.rb
+++ b/lib/puppet/network/handler/fileserver.rb
@@ -269,6 +269,7 @@ class Puppet::Network::Handler
               value = $2
               case var
               when "path"
+                raise FileServerError.new("No mount specified for argument #{var} #{value}") unless mount
                 if mount.name == MODULES
                   Puppet.warning "The '#{mount.name}' module can not have a path. Ignoring attempt to set it"
                 else
@@ -280,6 +281,7 @@ class Puppet::Network::Handler
                   end
                 end
               when "allow"
+                raise FileServerError.new("No mount specified for argument #{var} #{value}") unless mount
                 value.split(/\s*,\s*/).each { |val|
                   begin
                     mount.info "allowing #{val} access"
@@ -294,6 +296,7 @@ class Puppet::Network::Handler
                   end
                 }
               when "deny"
+                raise FileServerError.new("No mount specified for argument #{var} #{value}") unless mount
                 value.split(/\s*,\s*/).each { |val|
                   begin
                     mount.info "denying #{val} access"
diff --git a/spec/unit/network/handler/fileserver_spec.rb b/spec/unit/network/handler/fileserver_spec.rb
index 08852634d..851736e76 100755
--- a/spec/unit/network/handler/fileserver_spec.rb
+++ b/spec/unit/network/handler/fileserver_spec.rb
@@ -25,6 +25,38 @@ describe Puppet::Network::Handler::FileServer do
     @mount = Puppet::Network::Handler::FileServer::Mount.new("some_path", @basedir)
   end
 
+  describe "when parsing the fileserver.conf" do
+    it "should create a valid mount when a valid conf is read" do
+      config_file = tmpfile('fileserver.conf')
+      mountdir = tmpdir('mountdir')
+
+      conf_text = <<-HEREDOC
+        [mymount]
+          path #{mountdir}
+          allow anyone.com
+          deny nobody.com
+      HEREDOC
+      File.open(config_file, 'w') { |f| f.write conf_text }
+
+      fs = Puppet::Network::Handler::FileServer.new(:Config => config_file) 
+      mounts = fs.instance_variable_get(:@mounts)
+      mount = mounts["mymount"]
+      mount.path == mountdir
+      mount.instance_variable_get(:@declarations).map {|d| d.pattern}.should =~ [["com", "nobody"], ["com", "anyone"]]
+    end
+
+    ['path', 'allow', 'deny'].each do |arg|
+      it "should error if config file doesn't specify a mount for #{arg} argument" do
+        config_file = tmpfile('fileserver.conf')
+        File.open(config_file, 'w') { |f| f.puts "#{arg} 127.0.0.1/24" }
+
+        expect { 
+          Puppet::Network::Handler::FileServer.new(:Config => config_file) 
+        }.should raise_error(Puppet::Network::Handler::FileServerError, "No mount specified for argument #{arg} 127.0.0.1/24")
+      end
+    end
+  end
+
   it "should list a single directory" do
     @mount.list("/", false, false).should == [["/", "directory"]]
   end
-- 
cgit 


From ea0f2bf3fe596c6356db1150b28ddf6842f22ae9 Mon Sep 17 00:00:00 2001
From: Jacob Helwig <jacob@puppetlabs.com>
Date: Tue, 2 Aug 2011 14:12:16 -0700
Subject: Revert "Merge branch 'vcsrepo'"

vcsrepo is available in a module of its own, is missing tests, and
goes against the long-term goal of moving "extra" types out of core
puppet into modules (an example of this is the nagios types).

This reverts commit 25b967559dfa39eb094008c7a3952c4ee885530b, reversing
changes made to b87a1dea704ed981f2f0af728afac2c63e87b5a8.

Reviewed-by: Michael Stahnke <mike@puppetlabs.com>
---
 lib/puppet/provider/vcsrepo.rb     |  34 -----
 lib/puppet/provider/vcsrepo/bzr.rb |  64 ---------
 lib/puppet/provider/vcsrepo/cvs.rb |  80 -----------
 lib/puppet/provider/vcsrepo/git.rb | 264 -------------------------------------
 lib/puppet/provider/vcsrepo/hg.rb  | 101 --------------
 lib/puppet/provider/vcsrepo/svn.rb |  82 ------------
 lib/puppet/type/vcsrepo.rb         | 138 -------------------
 7 files changed, 763 deletions(-)
 delete mode 100644 lib/puppet/provider/vcsrepo.rb
 delete mode 100644 lib/puppet/provider/vcsrepo/bzr.rb
 delete mode 100644 lib/puppet/provider/vcsrepo/cvs.rb
 delete mode 100644 lib/puppet/provider/vcsrepo/git.rb
 delete mode 100644 lib/puppet/provider/vcsrepo/hg.rb
 delete mode 100644 lib/puppet/provider/vcsrepo/svn.rb
 delete mode 100644 lib/puppet/type/vcsrepo.rb

diff --git a/lib/puppet/provider/vcsrepo.rb b/lib/puppet/provider/vcsrepo.rb
deleted file mode 100644
index 2c026ba86..000000000
--- a/lib/puppet/provider/vcsrepo.rb
+++ /dev/null
@@ -1,34 +0,0 @@
-require 'tmpdir'
-require 'digest/md5'
-require 'fileutils'
-
-# Abstract
-class Puppet::Provider::Vcsrepo < Puppet::Provider
-
-  private
-
-  def set_ownership
-    owner = @resource.value(:owner) || nil
-    group = @resource.value(:group) || nil
-    FileUtils.chown_R(owner, group, @resource.value(:path))
-  end
-
-  def path_exists?
-    File.directory?(@resource.value(:path))
-  end
-
-  # Note: We don't rely on Dir.chdir's behavior of automatically returning the
-  # value of the last statement -- for easier stubbing.
-  def at_path(&block) #:nodoc:
-    value = nil
-    Dir.chdir(@resource.value(:path)) do
-      value = yield
-    end
-    value
-  end
-
-  def tempdir
-    @tempdir ||= File.join(Dir.tmpdir, 'vcsrepo-' + Digest::MD5.hexdigest(@resource.value(:path)))
-  end
-
-end
diff --git a/lib/puppet/provider/vcsrepo/bzr.rb b/lib/puppet/provider/vcsrepo/bzr.rb
deleted file mode 100644
index a0605624b..000000000
--- a/lib/puppet/provider/vcsrepo/bzr.rb
+++ /dev/null
@@ -1,64 +0,0 @@
-require File.join(File.dirname(__FILE__), '..', 'vcsrepo')
-
-Puppet::Type.type(:vcsrepo).provide(:bzr, :parent => Puppet::Provider::Vcsrepo) do
-  desc "Supports Bazaar repositories"
-
-  commands   :bzr => 'bzr'
-  defaultfor :bzr => :exists
-  has_features :reference_tracking
-
-  def create
-    if !@resource.value(:source)
-      create_repository(@resource.value(:path))
-    else
-      clone_repository(@resource.value(:revision))
-    end
-  end
-
-  def exists?
-    File.directory?(File.join(@resource.value(:path), '.bzr'))
-  end
-
-  def destroy
-    FileUtils.rm_rf(@resource.value(:path))
-  end
-  
-  def revision
-    at_path do
-      current_revid = bzr('version-info')[/^revision-id:\s+(\S+)/, 1]
-      desired = @resource.value(:revision)
-      begin
-        desired_revid = bzr('revision-info', desired).strip.split(/\s+/).last
-      rescue Puppet::ExecutionFailure
-        # Possible revid available during update (but definitely not current)
-        desired_revid = nil
-      end
-      if current_revid == desired_revid
-        desired
-      else
-        current_revid
-      end
-    end
-  end
-
-  def revision=(desired)
-    bzr('update', '-r', desired, @resource.value(:path))
-  end
-
-  private
-
-  def create_repository(path)
-    bzr('init', path)
-  end
-
-  def clone_repository(revision)
-    args = ['branch']
-    if revision
-      args.push('-r', revision)
-    end
-    args.push(@resource.value(:source),
-              @resource.value(:path))
-    bzr(*args)
-  end
-
-end
diff --git a/lib/puppet/provider/vcsrepo/cvs.rb b/lib/puppet/provider/vcsrepo/cvs.rb
deleted file mode 100644
index e82c23afe..000000000
--- a/lib/puppet/provider/vcsrepo/cvs.rb
+++ /dev/null
@@ -1,80 +0,0 @@
-require File.join(File.dirname(__FILE__), '..', 'vcsrepo')
-
-Puppet::Type.type(:vcsrepo).provide(:cvs, :parent => Puppet::Provider::Vcsrepo) do
-  desc "Supports CVS repositories/workspaces"
-
-  commands   :cvs => 'cvs'
-  defaultfor :cvs => :exists
-  has_features :gzip_compression, :reference_tracking
-  
-  def create
-    if !@resource.value(:source)
-      create_repository(@resource.value(:path))
-    else
-      checkout_repository
-    end
-  end
-
-  def exists?
-    if @resource.value(:source)
-      directory = File.join(@resource.value(:path), 'CVS')
-    else
-      directory = File.join(@resource.value(:path), 'CVSROOT')
-    end
-    File.directory?(directory)
-  end
-
-  def destroy
-    FileUtils.rm_rf(@resource.value(:path))
-  end
-
-  def revision
-    if File.exist?(tag_file)
-      contents = File.read(tag_file)
-      # Note: Doesn't differentiate between N and T entries
-      contents[1..-1]
-    else
-      'MAIN'
-    end
-  end
-
-  def revision=(desired)
-    at_path do
-      cvs('update', '-r', desired, '.')
-    end
-  end
-
-  private
-
-  def tag_file
-    File.join(@resource.value(:path), 'CVS', 'Tag')
-  end
-
-  def checkout_repository
-    dirname, basename = File.split(@resource.value(:path))
-    Dir.chdir(dirname) do
-      args = ['-d', @resource.value(:source)]
-      if @resource.value(:compression)
-        args.push('-z', @resource.value(:compression))
-      end
-      args.push('checkout', '-d', basename, module_name)
-      cvs(*args)
-    end
-    if @resource.value(:revision)
-      self.revision = @resource.value(:revision)
-    end
-  end
-
-  # When the source:
-  # * Starts with ':' (eg, :pserver:...)
-  def module_name
-    if (source = @resource.value(:source))
-      source[0, 1] == ':' ? File.basename(source) : '.'
-    end
-  end
-
-  def create_repository(path)
-    cvs('-d', path, 'init')
-  end
-
-end
diff --git a/lib/puppet/provider/vcsrepo/git.rb b/lib/puppet/provider/vcsrepo/git.rb
deleted file mode 100644
index fa7e492cf..000000000
--- a/lib/puppet/provider/vcsrepo/git.rb
+++ /dev/null
@@ -1,264 +0,0 @@
-require File.join(File.dirname(__FILE__), '..', 'vcsrepo')
-
-Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo) do
-  desc "Supports Git repositories"
-
-  ##TODO modify the commands below so that the su - is included
-  commands :git => 'git'
-  defaultfor :git => :exists
-  has_features :bare_repositories, :reference_tracking
-
-  def create
-    if !@resource.value(:source)
-      init_repository(@resource.value(:path))
-    else
-      clone_repository(@resource.value(:source), @resource.value(:path))
-      if @resource.value(:revision)
-        if @resource.value(:ensure) == :bare
-          notice "Ignoring revision for bare repository"
-        else
-          checkout_or_reset
-        end
-      end
-      if @resource.value(:ensure) != :bare
-        update_submodules
-      end
-    end
-    update_owner_and_excludes
-  end
-
-  def destroy
-    FileUtils.rm_rf(@resource.value(:path))
-  end
-
-  def latest?
-    at_path do
-      return self.revision == self.latest
-    end
-  end
-
-  def latest
-    branch = on_branch?
-    if branch == 'master'
-      return get_revision('origin/HEAD')
-    else
-        return get_revision('origin/%s' % branch)
-    end
-  end
-
-  def revision
-    update_references
-    current   = at_path { git('rev-parse', 'HEAD') }
-    canonical = at_path { git('rev-parse', @resource.value(:revision)) }
-    if current == canonical
-      @resource.value(:revision)
-    else
-      current
-    end
-  end
-
-  def revision=(desired)
-    checkout_or_reset(desired)
-    if local_branch_revision?(desired)
-      # reset instead of pull to avoid merge conflicts. assuming remote is
-      # authoritative.
-      # might be worthwhile to have an allow_local_changes param to decide
-      # whether to reset or pull when we're ensuring latest.
-      at_path { git('reset', '--hard', "origin/#{desired}") }
-    end
-    if @resource.value(:ensure) != :bare
-      update_submodules
-    end
-    update_owner_and_excludes
-  end
-
-  def bare_exists?
-    bare_git_config_exists? && !working_copy_exists?
-  end
-
-  def working_copy_exists?
-    File.directory?(File.join(@resource.value(:path), '.git'))
-  end
-
-  def exists?
-    working_copy_exists? || bare_exists?
-  end
-
-  def update_references
-    at_path do
-      git('fetch', '--tags', 'origin')
-    end
-  end
-
-  private
-
-  def bare_git_config_exists?
-    File.exist?(File.join(@resource.value(:path), 'config'))
-  end
-
-  def clone_repository(source, path)
-    check_force
-    args = ['clone']
-    if @resource.value(:ensure) == :bare
-      args << '--bare'
-    end
-    if !File.exist?(File.join(@resource.value(:path), '.git'))
-      args.push(source, path)
-      git(*args)
-    else
-      notice "Repo has already been cloned"
-    end
-  end
-
-  def check_force
-    if path_exists?
-      if @resource.value(:force)
-        notice "Removing %s to replace with vcsrepo." % @resource.value(:path)
-        destroy
-      else
-        raise Puppet::Error, "Could not create repository (non-repository at path)"
-      end
-    end
-  end
-
-  def init_repository(path)
-    check_force
-    if @resource.value(:ensure) == :bare && working_copy_exists?
-      convert_working_copy_to_bare
-    elsif @resource.value(:ensure) == :present && bare_exists?
-      convert_bare_to_working_copy
-    else
-      # normal init
-      FileUtils.mkdir(@resource.value(:path))
-      args = ['init']
-      if @resource.value(:ensure) == :bare
-        args << '--bare'
-      end
-      at_path do
-        git(*args)
-      end
-    end
-  end
-
-  # Convert working copy to bare
-  #
-  # Moves:
-  #   <path>/.git
-  # to:
-  #   <path>/
-  def convert_working_copy_to_bare
-    notice "Converting working copy repository to bare repository"
-    FileUtils.mv(File.join(@resource.value(:path), '.git'), tempdir)
-    FileUtils.rm_rf(@resource.value(:path))
-    FileUtils.mv(tempdir, @resource.value(:path))
-  end
-
-  # Convert bare to working copy
-  #
-  # Moves:
-  #   <path>/
-  # to:
-  #   <path>/.git
-  def convert_bare_to_working_copy
-    notice "Converting bare repository to working copy repository"
-    FileUtils.mv(@resource.value(:path), tempdir)
-    FileUtils.mkdir(@resource.value(:path))
-    FileUtils.mv(tempdir, File.join(@resource.value(:path), '.git'))
-    if commits_in?(File.join(@resource.value(:path), '.git'))
-      reset('HEAD')
-      git('checkout', '-f')
-      update_owner_and_excludes
-    end
-  end
-
-  def commits_in?(dot_git)
-    Dir.glob(File.join(dot_git, 'objects/info/*'), File::FNM_DOTMATCH) do |e|
-      return true unless %w(. ..).include?(File::basename(e))
-    end
-    false
-  end
-
-  def checkout_or_reset(revision = @resource.value(:revision))
-    if local_branch_revision? 
-      reset(revision)
-    elsif tag_revision?
-      at_path { git('checkout', '-b', revision) }
-    elsif remote_branch_revision?
-      at_path { git('checkout', '-b', revision, '--track', "origin/#{revision}") }
-    end
-  end
-
-  def reset(desired)
-    at_path do
-      git('reset', '--hard', desired)
-    end
-  end
-
-  def update_submodules
-    at_path do
-      git('submodule', 'init')
-      git('submodule', 'update')
-      git('submodule', 'foreach', 'git', 'submodule', 'init')
-      git('submodule', 'foreach', 'git', 'submodule', 'update')
-    end
-  end
-
-  def remote_branch_revision?(revision = @resource.value(:revision))
-    # git < 1.6 returns 'origin/#{revision}'
-    # git 1.6+ returns 'remotes/origin/#{revision}'
-    at_path { branches.grep /(remotes\/)?origin\/#{revision}/ }
-  end
-
-  def local_branch_revision?(revision = @resource.value(:revision))
-    at_path { branches.include?(revision) }
-  end
-
-  def tag_revision?(revision = @resource.value(:revision))
-    at_path { tags.include?(revision) }
-  end
-
-  def branches
-    at_path { git('branch', '-a') }.gsub('*', ' ').split(/\n/).map { |line| line.strip }
-  end
-
-  def on_branch?
-    at_path { git('branch', '-a') }.split(/\n/).grep(/\*/).to_s.gsub('*', '').strip
-  end
-
-  def tags
-    at_path { git('tag', '-l') }.split(/\n/).map { |line| line.strip }
-  end
-
-  def set_excludes
-    at_path { open('.git/info/exclude', 'w') { |f| @resource.value(:excludes).each { |ex| f.write(ex + "\n") }}}
-  end
-
-  def get_revision(rev)
-    if !working_copy_exists?
-      create
-    end
-    at_path do
-      git('fetch', 'origin')
-      git('fetch', '--tags', 'origin')
-    end
-    current = at_path { git('rev-parse', rev).strip }
-    if @resource.value(:revision)
-      if local_branch_revision?
-        canonical = at_path { git('rev-parse', @resource.value(:revision)).strip }
-      elsif remote_branch_revision?
-        canonical = at_path { git('rev-parse', 'origin/' + @resource.value(:revision)).strip }
-      end
-        current = @resource.value(:revision) if current == canonical
-    end
-    return current
-  end
-
-  def update_owner_and_excludes
-    if @resource.value(:owner) or @resource.value(:group)
-      set_ownership
-    end
-    if @resource.value(:excludes)
-      set_excludes
-    end
-  end
-end
diff --git a/lib/puppet/provider/vcsrepo/hg.rb b/lib/puppet/provider/vcsrepo/hg.rb
deleted file mode 100644
index f96758612..000000000
--- a/lib/puppet/provider/vcsrepo/hg.rb
+++ /dev/null
@@ -1,101 +0,0 @@
-require File.join(File.dirname(__FILE__), '..', 'vcsrepo')
-
-Puppet::Type.type(:vcsrepo).provide(:hg, :parent => Puppet::Provider::Vcsrepo) do
-  desc "Supports Mercurial repositories"
-
-  commands   :hg => 'hg'
-  defaultfor :hg => :exists
-  has_features :reference_tracking
-
-  def create
-    if !@resource.value(:source)
-      create_repository(@resource.value(:path))
-    else
-      clone_repository(@resource.value(:revision))
-    end
-    update_owner
-  end
-
-  def working_copy_exists?
-    File.directory?(File.join(@resource.value(:path), '.hg'))
-  end
-
-  def exists?
-    working_copy_exists?
-  end
-
-  def destroy
-    FileUtils.rm_rf(@resource.value(:path))
-  end
-
-  def latest?
-    at_path do
-      return self.revision == self.latest
-    end
-  end
-
-  def latest
-    at_path do
-      begin
-        hg('incoming', '--branch', '.', '--newest-first', '--limit', '1')[/^changeset:\s+(?:-?\d+):(\S+)/m, 1]
-      rescue Puppet::ExecutionFailure
-        # If there are no new changesets, return the current nodeid
-        self.revision
-      end
-    end
-  end
-
-  def revision
-    at_path do
-      current = hg('parents')[/^changeset:\s+(?:-?\d+):(\S+)/m, 1]
-      desired = @resource.value(:revision)
-      if desired
-        # Return the tag name if it maps to the current nodeid
-        mapped = hg('tags')[/^#{Regexp.quote(desired)}\s+\d+:(\S+)/m, 1]
-        if current == mapped
-          desired
-        else
-          current
-        end
-      else
-        current
-      end
-    end
-  end
-
-  def revision=(desired)
-    at_path do
-      hg('pull')
-      begin
-        hg('merge')
-      rescue Puppet::ExecutionFailure
-        # If there's nothing to merge, just skip
-      end
-      hg('update', '--clean', '-r', desired)
-    end
-    update_owner
-  end
-
-  private
-
-  def create_repository(path)
-    hg('init', path)
-  end
-
-  def clone_repository(revision)
-    args = ['clone']
-    if revision
-      args.push('-u', revision)
-    end
-    args.push(@resource.value(:source),
-              @resource.value(:path))
-    hg(*args)
-  end
-
-  def update_owner
-    if @resource.value(:owner) or @resource.value(:group)
-      set_ownership
-    end
-  end
-
-end
diff --git a/lib/puppet/provider/vcsrepo/svn.rb b/lib/puppet/provider/vcsrepo/svn.rb
deleted file mode 100644
index 680188c1e..000000000
--- a/lib/puppet/provider/vcsrepo/svn.rb
+++ /dev/null
@@ -1,82 +0,0 @@
-require File.join(File.dirname(__FILE__), '..', 'vcsrepo')
-
-Puppet::Type.type(:vcsrepo).provide(:svn, :parent => Puppet::Provider::Vcsrepo) do
-  desc "Supports Subversion repositories"
-
-  commands :svn      => 'svn',
-           :svnadmin => 'svnadmin'
-
-  defaultfor :svn => :exists
-  has_features :filesystem_types, :reference_tracking
-
-  def create
-    if !@resource.value(:source)
-      create_repository(@resource.value(:path))
-    else
-      checkout_repository(@resource.value(:source),
-                          @resource.value(:path),
-                          @resource.value(:revision))
-    end
-  end
-
-  def working_copy_exists?
-    File.directory?(File.join(@resource.value(:path), '.svn'))
-  end
-
-  def exists?
-    working_copy_exists?
-  end
-
-  def destroy
-    FileUtils.rm_rf(@resource.value(:path))
-  end
-
-  def latest?
-    at_path do
-      if self.revision < self.latest then
-        return false
-      else
-        return true
-      end
-    end
-  end
-
-  def latest
-    at_path do
-      svn('info', '-r', 'HEAD')[/^Revision:\s+(\d+)/m, 1]
-    end
-  end
-  
-  def revision
-    at_path do
-      svn('info')[/^Revision:\s+(\d+)/m, 1]
-    end
-  end
-
-  def revision=(desired)
-    at_path do
-      svn('update', '-r', desired)
-    end
-  end
-
-  private
-
-  def checkout_repository(source, path, revision = nil)
-    args = ['checkout']
-    if revision
-      args.push('-r', revision)
-    end
-    args.push(source, path)
-    svn(*args)
-  end
-
-  def create_repository(path)
-    args = ['create']
-    if @resource.value(:fstype)
-      args.push('--fs-type', @resource.value(:fstype))
-    end
-    args << path
-    svnadmin(*args)
-  end
-
-end
diff --git a/lib/puppet/type/vcsrepo.rb b/lib/puppet/type/vcsrepo.rb
deleted file mode 100644
index f0d2613ca..000000000
--- a/lib/puppet/type/vcsrepo.rb
+++ /dev/null
@@ -1,138 +0,0 @@
-require 'pathname'
-
-Puppet::Type.newtype(:vcsrepo) do
-  desc "A local version control repository"
-
-  feature :gzip_compression,
-          "The provider supports explicit GZip compression levels"
-
-  feature :bare_repositories,
-          "The provider differentiates between bare repositories
-          and those with working copies",
-          :methods => [:bare_exists?, :working_copy_exists?]
-
-  feature :filesystem_types,
-          "The provider supports different filesystem types"
-
-  feature :reference_tracking,
-          "The provider supports tracking revision references that can change
-           over time (eg, some VCS tags and branch names)"
-
-  ensurable do
-    attr_accessor :latest
-
-    def insync?(is)
-      @should ||= []
-
-      case should
-        when :present
-          return true unless [:absent, :purged, :held].include?(is)
-        when :latest
-          if is == :latest
-            return true
-          else
-            self.debug "%s repo revision is %s, latest is %s" %
-                [@resource.name, provider.revision, provider.latest]
-            return false
-          end
-      end
-    end
-
-    newvalue :present do
-      provider.create
-    end
-
-    newvalue :bare, :required_features => [:bare_repositories] do
-      provider.create
-    end
-
-    newvalue :absent do
-      provider.destroy
-    end
-
-    newvalue :latest, :required_features => [:reference_tracking] do
-      if provider.exists?
-        if provider.respond_to?(:update_references)
-          provider.update_references
-        end
-        if provider.respond_to?(:latest?)
-            reference = provider.latest || provider.revision
-        else
-          reference = resource.value(:revision) || provider.revision
-        end
-        notice "Updating to latest '#{reference}' revision"
-        provider.revision = reference
-      else
-        provider.create
-      end
-    end
-
-    def retrieve
-      prov = @resource.provider
-      if prov
-        if prov.working_copy_exists?
-          prov.latest? ? :latest : :present
-        elsif prov.class.feature?(:bare_repositories) and prov.bare_exists?
-          :bare
-        else
-          :absent
-        end
-      else
-        raise Puppet::Error, "Could not find provider"
-      end
-    end
-
-  end
-
-  newparam(:path) do
-    desc "Absolute path to repository"
-    isnamevar
-    validate do |value|
-      path = Pathname.new(value)
-      unless path.absolute?
-        raise ArgumentError, "Path must be absolute: #{path}"
-      end
-    end
-  end
-
-  newparam(:source) do
-    desc "The source URI for the repository"
-  end
-
-  newparam(:fstype, :required_features => [:filesystem_types]) do
-    desc "Filesystem type"
-  end
-
-  newproperty(:revision) do
-    desc "The revision of the repository"
-    newvalue(/^\S+$/)
-  end
-
-  newparam(:owner) do
-    desc "The user/uid that owns the repository files"
-  end
-
-  newparam(:group) do
-    desc "The group/gid that owns the repository files"
-  end
-
-  newparam(:excludes) do
-    desc "Files to be excluded from the repository"
-  end
-
-  newparam(:force) do
-    desc "Force repository creation, destroying any files on the path in the process."
-    newvalues(:true, :false)
-    defaultto false
-  end
-
-  newparam :compression, :required_features => [:gzip_compression] do
-    desc "Compression level"
-    validate do |amount|
-      unless Integer(amount).between?(0, 6)
-        raise ArgumentError, "Unsupported compression level: #{amount} (expected 0-6)"
-      end
-    end
-  end
-
-end
-- 
cgit 


From b85f57cc227df16a6f0beedbd4f10befa55b9b3c Mon Sep 17 00:00:00 2001
From: Jacob Helwig <jacob@puppetlabs.com>
Date: Tue, 2 Aug 2011 17:34:10 -0700
Subject: Add document outlining preferred contribution methods

We have historically had the preferred contribution process on the
Redmine wiki, however this is not obvious to people that don't already
know it is there.  By adding this document to the repository itself,
it becomes much easier for new contributors to find what the preferred
contribution methods are.

By having the preferred contribution method in the repository also
means that it becomes a "curated" document, which must go through the
same submission/review process that other changes to the repositories
go through.

Reviewed-by: Nick Fagerlund <nick.fagerlund@puppetlabs.com>
Reviewed-by: Nick Lewis <nick@puppetlabs.com>
---
 CONTRIBUTING.md | 299 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 299 insertions(+)
 create mode 100644 CONTRIBUTING.md

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
new file mode 100644
index 000000000..dd8e89720
--- /dev/null
+++ b/CONTRIBUTING.md
@@ -0,0 +1,299 @@
+Checklist (and a short version for the impatient)
+=================================================
+
+  * Commits:
+
+    - Make commits of logical units.
+
+    - Check for unnecessary whitespace with "git diff --check" before
+      committing.
+
+    - Commit using Unix line endings (check the settings around "crlf" in
+      git-config(1)).
+
+    - Do not check in commented out code or unneeded files.
+
+    - The first line of the commit message should be a short
+      description (50 characters is the soft limit, excluding ticket
+      number(s)), and should skip the full stop.
+
+    - If there is an associated Redmine ticket then the first line
+      should include the ticket number in the form "(#XXXX) Rest of
+      message".
+
+    - The body should provide a meaningful commit message, which:
+
+      - uses the imperative, present tense: "change", not "changed" or
+        "changes".
+
+      - includes motivation for the change, and contrasts its
+        implementation with the previous behavior.
+
+    - Make sure that you have tests for the bug you are fixing, or
+      feature you are adding.
+
+    - Make sure the test suite passes after your commit (rake spec unit).
+
+  * Submission:
+
+    * Pre-requisites:
+
+      - Make sure you have a [Redmine account](http://projects.puppetlabs.com)
+
+      - Sign the [Contributor License Agreement](https://projects.puppetlabs.com/contributor_licenses/sign)
+
+    * Preferred method:
+
+      - Fork the repository on GitHub.
+
+      - Push your changes to a topic branch in your fork of the
+        repository.
+
+      - Submit a pull request to the repository in the puppetlabs
+        organization.
+
+    * Alternate methods:
+
+      - Mail patches to puppet-dev mailing list using `rake mail_patches`,
+        or `git-format-patch(1)` & `git-send-email(1)`.
+
+      - Attach patches to Redmine ticket.
+
+The long version
+================
+
+  0.  Decide what to base your work on.
+
+      In general, you should always base your work on the oldest
+      branch that your change is relevant to.
+
+      - A bug fix should be based on the current stable series. If the
+        bug is not present in the current stable release, then base it on
+        `master`.
+
+      - A new feature should be based on `master`.
+
+      - Security fixes should be based on the current maintenance series
+        (that is, the previous stable series).  If the security issue
+        was not present in the maintenance series, then it should be
+        based on the current stable series if it was introduced there,
+        or on `master` if it is not yet present in a stable release.
+
+      The current stable series is 2.7.x, and the current maintenance
+      series is 2.6.x.
+
+  1.  Make separate commits for logically separate changes.
+
+      Please break your commits down into logically consistent units
+      which include new or changed tests relevent to the rest of the
+      change.  The goal of doing this is to make the diff easier to
+      read for whoever is reviewing your code.  In general, the easier
+      your diff is to read, the more likely someone will be happy to
+      review it and get it into the code base.
+
+      If you're going to refactor a piece of code, please do so as a
+      separate commit from your feature or bug fix changes.
+
+      We also really appreciate changes that include tests to make
+      sure the bug isn't re-introduced, and that the feature isn't
+      accidentally broken.
+
+      Describe the technical detail of the change(s).  If your
+      description starts to get too long, that's a good sign that you
+      probably need to split up your commit into more finely grained
+      pieces.
+
+      Commits which plainly describe the the things which help
+      reviewers check the patch and future developers understand the
+      code are much more likely to be merged in with a minimum of
+      bike-shedding or requested changes.  Ideally, the commit message
+      would include information, and be in a form suitable for
+      inclusion in the release notes for the version of Puppet that
+      includes them.
+
+      Please also check that you are not introducing any trailing
+      whitespaces or other "whitespace errors".  You can do this by
+      running "git diff --check" on your changes before you commit.
+
+  2.  Sign the Contributor License Agreement
+
+      Before we can accept your changes, we do need a signed Puppet
+      Labs Contributor License Agreement (CLA).
+
+      You can access the CLA via the
+      [Contributor License Agreement link](https://projects.puppetlabs.com/contributor_licenses/sign)
+      in the top menu bar of our Redmine instance.  Once you've signed
+      the CLA, a badge will show up next to your name on the
+      [Puppet Project Overview Page](http://projects.puppetlabs.com/projects/puppet?jump=welcome),
+      and your name will be listed under "Contributor License Signers"
+      section.
+
+      If you have any questions about the CLA, please feel free to
+      contact Puppet Labs via email at cla-submissions@puppetlabs.com.
+
+  3.  Sending your patches
+
+      We accept multiple ways of submitting your changes for
+      inclusion.  They are listed below in order of preference.
+
+      Please keep in mind that any method that involves sending email
+      to the mailing list directly requires you to be subscribed to
+      the mailing list, and that your first post to the list will be
+      held in a moderation queue.
+
+      * GitHub Pull Requests
+
+        To submit your changes via a GitHub pull request, we _highly_
+        recommend that you have them on a topic branch, instead of
+        directly on "master" or one of the release, or RC branches.
+        It makes things much easier to keep track of, especially if
+        you decide to work on another thing before your first change
+        is merged in.
+
+        GitHub has some pretty good
+        [general documentation](http://help.github.com/) on using
+        their site.  They also have documentation on
+        [creating pull requests](http://help.github.com/send-pull-requests/).
+
+        In general, after pushing your topic branch up to your
+        repository on GitHub, you'll switch to the branch in the
+        GitHub UI and click "Pull Request" towards the top of the page
+        in order to open a pull request.
+
+        You'll want to make sure that you have the appropriate
+        destination branch in the repository under the puppetlabs
+        organization.  This should be the same branch that you based
+        your changes off of.
+
+      * Other pull requests
+
+        If you already have a publicly accessible version of the
+        repository hosted elsewhere, and don't wish to or cannot use
+        GitHub, you can submit your change by requesting that we pull
+        the changes from your repository by sending an email to the
+        puppet-dev Google Groups mailing list.
+
+        `git-request-pull(1)` provides a handy way to generate the text
+        for the email requesting that we pull your changes (and does
+        some helpful sanity checks in the process).
+
+      * Mailing patches to the mailing list
+
+        If neither of the previous methods works for you, then you can
+        also mail the patches inline to the puppet-dev Google Group
+        using either `rake mail_patches`, or by using
+        `git-format-patch(1)`, and `git-send-email(1)` directly.
+
+        `rake mail_patches` handles setting the appropriate flags to
+        `git-format-patch(1)` and `git-send-email(1)` for you, but
+        doesn't allow adding any commentary between the '---', and the
+        diffstat in the resulting email.  It also requires that you
+        have created your topic branch in the form
+        `<type>/<parent>/<name>`.
+
+        If you decide to use `git-format-patch(1)` and
+        `git-send-email(1)` directly, please be sure to use the
+        following flags for `git-format-patch(1)`: -C -M -s -n
+        --subject-prefix='PATCH/puppet'
+
+      * Attaching patches to Redmine
+
+        As a method of last resort you can also directly attach the
+        output of `git-format-patch(1)`, or `git-diff(1)` to a Redmine
+        ticket.
+
+        If you are generating the diff outside of Git, please be sure
+        to generate a unified diff.
+
+  4.  Update the related Redmine ticket.
+
+      If there's a Redmine ticket associated with the change you
+      submitted, then you should update the ticket to include the
+      location of your branch, and change the status to "In Topic
+      Branch Pending Merge", along with any other commentary you may
+      wish to make.
+
+How to track the status of your change after it's been submitted
+================================================================
+
+Shortly after opening a pull request on GitHub, there should be an
+automatic message sent to the puppet-dev Google Groups mailing list
+notifying people of this.  This notification is used to let the Puppet
+development community know about your requested change to give them a
+chance to review, test, and comment on the change(s).
+
+If you submitted your change via manually sending a pull request or
+mailing the patches, then we keep track of these using
+[patchwork](https://patchwork.puppetlabs.com).  When code is merged
+into the project it is automatically removed from patchwork, and the
+Redmine ticket is manually updated with the commit SHA1.  In addition,
+the ticket status must be updated by the person who merges the topic
+branch to a status of "Merged - Pending Release"
+
+We do our best to comment on or merge submitted changes within a week.
+However, if there hasn't been any commentary on the pull request or
+mailed patches, and it hasn't been merged in after a week, then feel
+free to ask for an update by replying on the mailing list to the
+automatic notification or mailed patches. It probably wasn't
+intentional, and probably just slipped through the cracks.
+
+Additional Resources
+====================
+
+* [Getting additional help](http://projects.puppetlabs.com/projects/puppet/wiki/Getting_Help)
+
+* [Writing tests](http://projects.puppetlabs.com/projects/puppet/wiki/Development_Writing_Tests)
+
+* [Bug tracker (Redmine)](http://projects.puppetlabs.com)
+
+* [Patchwork](https://patchwork.puppetlabs.com)
+
+* [Contributor License Agreement](https://projects.puppetlabs.com/contributor_licenses/sign)
+
+* [General GitHub documentation](http://help.github.com/)
+
+* [GitHub pull request documentation](http://help.github.com/send-pull-requests/)
+
+If you have commit access to the repository
+===========================================
+
+Even if you have commit access to the repository, you'll still need to
+go through the process above, and have someone else review and merge
+in your changes.  The rule is that all changes must be reviewed by a
+developer on the project (that didn't write the code) to ensure that
+all changes go through a code review process.
+
+Having someone other than the author of the topic branch recorded as
+performing the merge is the record that they performed the code
+review.
+
+  * Merging topic branches
+
+    When merging code from a topic branch into the integration branch
+    (Ex: master, 2.7.x, 1.6.x, etc.), there should always be a merge
+    commit.  You can accomplish this by always providing the `--no-ff`
+    flag to `git merge`.
+
+        git merge --no-ff --log tickets/master/1234-fix-something-broken
+
+    The reason for always forcing this merge commit is that it
+    provides a consistent way to look up what changes & commits were
+    in a topic branch, whether that topic branch had one, or 500
+    commits.  For example, if the merge commit had an abbreviated
+    SHA-1 of `coffeebad`, then you could use the following `git log`
+    invocation to show you which commits it brought in:
+
+        git log coffeebad^1..coffeebad^2
+
+    The following would show you which changes were made on the topic
+    branch:
+
+        git diff coffeebad^1...coffeebad^2
+
+    Because we _always_ merge the topic branch into the integration
+    branch the first parent (`^1`) of a merge commit will be the most
+    recent commit on the integration branch from just before we merged
+    in the topic, and the second parent (`^2`) will always be the most
+    recent commit that was made in the topic branch.  This also serves
+    as the record of who performed the code review, as mentioned
+    above.
-- 
cgit 


From 711344836aa1e469876fc511be14e8159e61b0b8 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Thu, 4 Aug 2011 10:49:59 -0700
Subject: (#4762) Ensure that clients on the moon can successfully connect.

Previously, we only allowed Puppet Clients at a maximum distance of somewhere
between 7,494 and 14,988 kilometers from the master, depending on the variance
in local conditions.

While this gave us good data security against hostile clients connecting from
the dark side of the moon, real world testing shows the moon folks are likely
to just take over a local staging host and attack that way.

So, instead, allow clients sufficient time they should be comfortable able to
connect to a master from the moon.  We still refuse clients further out, like
Mars, since it seems unlikely that Puppet management over that distance should
work.

We advise the manned Mars expedition to deploy a local Puppet Master to manage
infrastructure in their base, and to watch out for the martians.
---
 lib/puppet/network/http/webrick.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/puppet/network/http/webrick.rb b/lib/puppet/network/http/webrick.rb
index 54bcf30c2..52aec1bf1 100644
--- a/lib/puppet/network/http/webrick.rb
+++ b/lib/puppet/network/http/webrick.rb
@@ -40,7 +40,7 @@ class Puppet::Network::HTTP::WEBrick
       @listening = true
       @thread = Thread.new {
         @server.start { |sock|
-          raise "Client disconnected before connection could be established" unless IO.select([sock],nil,nil,0.1)
+          raise "Client disconnected before connection could be established" unless IO.select([sock],nil,nil,6.2)
           sock.accept
           @server.run(sock)
         }
-- 
cgit