Merge branch '0.24.x'

Conflicts:

	lib/puppet/executables/client/certhandler.rb
	spec/unit/executables/client/certhandler.rb

4 files changed, 86 insertions, 68 deletions

diff --git a/lib/puppet/executables/client/certhandler.rb b/lib/puppet/executables/client/certhandler.rb
index 6f23de0e3..b73642941 100644
--- a/lib/puppet/executables/client/certhandler.rb
+++ b/lib/puppet/executables/client/certhandler.rb
@@ -4,8 +4,7 @@ module Puppet
         module Client
             class CertHandler
                 attr_writer :wait_for_cert, :one_time
-
-                attr_reader :caclient
+                attr_reader :caclient, :new_cert
                 
                 def initialize(wait_time, is_one_time)
                     @wait_for_cert = wait_time
@@ -14,7 +13,14 @@ module Puppet
 
                     @caclient = Puppet::Network::Client.ca.new()
                 end
+
+                # Did we just read a cert?
+                def new_cert?
+                    new_cert
+                end
                 
+                # Read, or retrieve if necessary, our certificate.  Returns true if we retrieved
+                # a new cert, false if the cert already exists.
                 def read_retrieve 
                     #NOTE: ACS this is checking that a file exists, maybe next time just do that?
                     unless read_cert 
@@ -23,7 +29,7 @@ module Puppet
                         retrieve_cert
                     end
 
-                    !@new_cert
+                    ! new_cert?
                 end
 
                 def retrieve_cert
@@ -54,7 +60,8 @@ module Puppet
                 def read_new_cert
                     if caclient.read_cert
                         # If we read it in, then we need to get rid of our existing http connection.
-                        # The @new_cert flag will help us do that
+                        # The @new_cert flag will help us do that, in that it provides a way
+                        # to notify that the cert status has changed.
                         @new_cert = true
                         Puppet.notice "Got signed certificate"
                     else
diff --git a/lib/puppet/parser/interpreter.rb b/lib/puppet/parser/interpreter.rb
index 04ca41494..423c34a4e 100644
--- a/lib/puppet/parser/interpreter.rb
+++ b/lib/puppet/parser/interpreter.rb
@@ -50,23 +50,8 @@ class Puppet::Parser::Interpreter
     # Return the parser for a specific environment.
     def parser(environment)
         if ! @parsers[environment] or @parsers[environment].reparse?
-            # This will throw an exception if it does not succeed.  We only
-            # want to get rid of the old parser if we successfully create a new
-            # one.
-            begin
-                tmp = create_parser(environment)
-                @parsers[environment].clear if @parsers[environment]
-                @parsers[environment] = tmp
-            rescue => detail
-                # If a parser already exists, than assume that we logged the
-                # exception elsewhere and reuse the parser.  If one doesn't
-                # exist, then reraise.
-                if @parsers[environment]
-                    Puppet.err(detail.to_s + "; using previously parsed manifests")
-                else
-                    raise detail
-                end
-            end
+            # This will throw an exception if it does not succeed.
+            @parsers[environment] = create_parser(environment)
         end
         @parsers[environment]
     end
diff --git a/spec/unit/executables/client/certhandler.rb b/spec/unit/executables/client/certhandler.rb
index 3737c9a57..5950aa4dc 100755
--- a/spec/unit/executables/client/certhandler.rb
+++ b/spec/unit/executables/client/certhandler.rb
@@ -27,62 +27,109 @@ describe cert_handler, "when handling certificates" do
         ch.read_retrieve.should be_false
     end
 
+    describe "when reading or retrieving the certificate" do
+        before do
+            @handler = cert_handler.new(1,true)
+        end
+
+        it "should attempt to read the certificate" do
+            @handler.expects(:read_cert).returns true
+            @handler.read_retrieve
+        end
+
+        it "should delegate to the ca client to read the certificate" do
+            @caclient.expects(:read_cert).returns(true)
+            @handler.read_retrieve
+        end
+
+        it "should not attempt to retrieve a certificate if one can be read" do
+            @handler.stubs(:read_cert).returns true
+            @handler.expects(:retrieve_cert).never
+            @handler.read_retrieve
+        end
+
+        it "should attempt to retrieve a certificate if none can be read" do
+            @handler.stubs(:read_cert).returns false
+            @handler.expects(:retrieve_cert)
+            @handler.read_retrieve
+        end
+
+        it "should delegate to caclient to retrieve a certificate" do
+            @handler.stubs(:read_cert).returns false
+            @caclient.expects(:request_cert).returns(true)
+            @handler.stubs(:read_new_cert).returns(true)
+            @handler.read_retrieve
+        end
+
+        it "should return true if the certificate exists" do
+            @handler.stubs(:read_cert).returns true
+            @handler.read_retrieve.should be_true
+        end
+
+        it "should return false when getting a new cert" do
+            #This is the second call to httppool that happens in 'read_new_cert'
+            @caclient.expects(:read_cert).returns(true)
+            @caclient.stubs(:request_cert).returns(true)
+            @handler.stubs(:read_cert).returns(false)
+            @handler.read_retrieve.should be_false
+        end
+    end
+
     describe "when waiting for cert" do
         before do
-            @caclient.stubs(:read_cert).returns false
+            @handler = cert_handler.new(1,false)
+            @handler.stubs(:read_cert).returns false
+            #all waiting for cert tests should loop, which will always happen if sleep is called
+            #yeah, I put the expectation in the setup, deal with it
+            @handler.expects(:sleep).with(1)
+
+            #This is needed to get out of the loop
+            @handler.stubs(:read_new_cert).returns(true)
         end
 
         it "should loop when the cert request does not return a certificate" do
             @caclient.stubs(:request_cert).times(2).returns(false).then.returns(true)
-            ch = cert_handler.new(1,false)
-            ch.expects(:sleep)
-            ch.expects(:read_new_cert).returns(true)
-            ch.read_retrieve
+            @handler.retrieve_cert
         end
 
         it "should loop when the cert request raises an Error" do
             @caclient.stubs(:request_cert).times(2).raises(StandardError, 'Testing').then.returns(true)
-            ch = cert_handler.new(1,false)
-            ch.expects(:sleep)
-            ch.expects(:read_new_cert).returns(true)
-            ch.read_retrieve
+            @handler.retrieve_cert
         end
         
         it "should loop when the new cert can't be read" do
             @caclient.stubs(:request_cert).returns(true)
-            ch = cert_handler.new(1,false)
-            ch.expects(:sleep)
-            ch.expects(:read_new_cert).times(2).returns(false).then.returns(true)
-            ch.read_retrieve
+            @handler.stubs(:read_new_cert).times(2).returns(false).then.returns(true)
+            @handler.retrieve_cert
         end
     end
 
     describe "when in one time mode" do
         before do
-            @caclient.stubs(:read_cert).returns false
+            #true puts us in onetime mode
+            @handler = cert_handler.new(1,true)
+            @handler.stubs(:read_cert).returns false
         end
 
         it "should exit if the cert request does not return a certificate" do
             @caclient.stubs(:request_cert).returns(false)
-            ch = cert_handler.new(1,true)
-            ch.expects(:exit).with(1).raises(SystemExit)
-            lambda { ch.read_retrieve }.should raise_error(SystemExit)
+            @handler.expects(:exit).with(1).raises(SystemExit)
+            lambda { @handler.retrieve_cert }.should raise_error(SystemExit)
         end
 
 
         it "should exit if the cert request raises an exception" do
             @caclient.stubs(:request_cert).raises(StandardError, 'Testing')
-            ch = cert_handler.new(1,true)
-            ch.expects(:exit).with(23).raises(SystemExit)
-            lambda { ch.read_retrieve }.should raise_error(SystemExit)
+            @handler.expects(:exit).with(23).raises(SystemExit)
+            lambda { @handler.retrieve_cert }.should raise_error(SystemExit)
         end
         
         it "should exit if the new cert can't be read" do
             @caclient.stubs(:request_cert).returns(true)
+            #this is the second, call to httppool inside read_new_cert
             @caclient.stubs(:read_cert).returns(false)
-            ch = cert_handler.new(1,true)
-            ch.expects(:exit).with(34).raises(SystemExit)
-            lambda { ch.read_retrieve }.should raise_error(SystemExit)
+            @handler.expects(:exit).with(34).raises(SystemExit)
+            lambda { @handler.retrieve_cert }.should raise_error(SystemExit)
         end
     end
 end
diff --git a/spec/unit/parser/interpreter.rb b/spec/unit/parser/interpreter.rb
index f2526c73d..27cf7056d 100755
--- a/spec/unit/parser/interpreter.rb
+++ b/spec/unit/parser/interpreter.rb
@@ -84,7 +84,6 @@ describe Puppet::Parser::Interpreter do
                 oldparser = mock('oldparser')
                 newparser = mock('newparser')
                 oldparser.expects(:reparse?).returns(true)
-                oldparser.expects(:clear)
 
                 @interp.expects(:create_parser).with(:myenv).returns(oldparser)
                 @interp.send(:parser, :myenv).should equal(oldparser)
@@ -92,36 +91,16 @@ describe Puppet::Parser::Interpreter do
                 @interp.send(:parser, :myenv).should equal(newparser)
             end
 
-            it "should keep the old parser if a new parser cannot be created" do
+            it "should raise an exception if a new parser cannot be created" do
                 # Get the first parser in the hash.
                 @interp.expects(:create_parser).with(:myenv).returns(@parser)
                 @interp.send(:parser, :myenv).should equal(@parser)
 
-                # Have it indicate something has changed
                 @parser.expects(:reparse?).returns(true)
 
-                # But fail to create a new parser
-                @interp.expects(:create_parser).with(:myenv).raises(ArgumentError)
+                @interp.expects(:create_parser).with(:myenv).raises(Puppet::Error, "Could not parse")
 
-                # And make sure we still get the old valid parser
-                @interp.send(:parser, :myenv).should equal(@parser)
-            end
-
-            it "should log syntax errors when using the old parser" do
-                # Get the first parser in the hash.
-                @interp.stubs(:create_parser).with(:myenv).returns(@parser)
-                @interp.send(:parser, :myenv)
-
-                # Have it indicate something has changed
-                @parser.stubs(:reparse?).returns(true)
-
-                # But fail to create a new parser
-                @interp.stubs(:create_parser).with(:myenv).raises(ArgumentError)
-
-                Puppet.expects(:err)
-
-                # And make sure we still get the old valid parser
-                @interp.send(:parser, :myenv)
+                lambda { @interp.parser(:myenv) }.should raise_error(Puppet::Error) 
             end
         end
     end