(#1886) Clean up `node clean` for merge.

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

Paired-With: Matt Robinson

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