Fixing #800 by refactoring how configurations are retrieved

from the server.  The real problem was getting all of the validation
done before any caching, which required a good bit more refactoring
than I expected.

In actuality, this commit is relatively small even though it covers
many files; most of the changes just make the code clearer or shorter.

4 files changed, 460 insertions, 98 deletions

diff --git a/spec/unit/network/client/master.rb b/spec/unit/network/client/master.rb
new file mode 100755
index 000000000..dca923994
--- /dev/null
+++ b/spec/unit/network/client/master.rb
@@ -0,0 +1,365 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke Kanies on 2007-11-12.
+#  Copyright (c) 2007. All rights reserved.
+
+require File.dirname(__FILE__) + '/../../../spec_helper'
+require 'puppet/network/client/master'
+
+describe Puppet::Network::Client::Master, " when retrieving the configuration" do
+    before do
+        @master = mock 'master'
+        @client = Puppet::Network::Client.master.new(
+            :Master => @master
+        )
+        @facts = {"one" => "two", "three" => "four"}
+    end
+
+    it "should initialize the metadata store" do
+        @client.class.stubs(:facts).returns(@facts)
+        @client.expects(:dostorage)
+        @master.stubs(:getconfig).returns(nil)
+        @client.getconfig
+    end
+
+    it "should collect facts to use for configuration retrieval" do
+        @client.stubs(:dostorage)
+        @client.class.expects(:facts).returns(@facts)
+        @master.stubs(:getconfig).returns(nil)
+        @client.getconfig
+    end
+
+    it "should fail if no facts could be collected" do
+        @client.stubs(:dostorage)
+        @client.class.expects(:facts).returns({})
+        @master.stubs(:getconfig).returns(nil)
+        proc { @client.getconfig }.should raise_error(Puppet::Network::ClientError)
+    end
+
+    it "should use the cached configuration if it is up to date" do
+        file = "/path/to/cachefile"
+        @client.stubs(:cachefile).returns(file)
+        FileTest.expects(:exist?).with(file).returns(true)
+        @client.expects(:fresh?).with(@facts).returns true
+        @client.class.stubs(:facts).returns(@facts)
+        @client.expects(:use_cached_config).returns(true)
+        Puppet.stubs(:info)
+
+        @client.getconfig
+    end
+
+    it "should log that the configuration does not need a recompile" do
+        file = "/path/to/cachefile"
+        @client.stubs(:cachefile).returns(file)
+        FileTest.stubs(:exist?).with(file).returns(true)
+        @client.stubs(:fresh?).with(@facts).returns true
+        @client.stubs(:use_cached_config).returns(true)
+        @client.class.stubs(:facts).returns(@facts)
+        Puppet.expects(:info).with { |m| m.include?("up to date") }
+
+        @client.getconfig
+    end
+
+    it "should retrieve plugins if :pluginsync is enabled" do
+        file = "/path/to/cachefile"
+        @client.stubs(:cachefile).returns(file)
+        @client.stubs(:dostorage)
+        @client.class.stubs(:facts).returns(@facts)
+        Puppet.settings.expects(:value).with(:pluginsync).returns(true)
+        @client.expects(:getplugins)
+        @client.stubs(:get_actual_config).returns(nil)
+        FileTest.stubs(:exist?).with(file).returns(true)
+        @client.stubs(:fresh?).with(@facts).returns true
+        @client.stubs(:use_cached_config).returns(true)
+        @client.class.stubs(:facts).returns(@facts)
+        @client.getconfig
+    end
+
+    it "should use the cached configuration if no configuration could be retrieved" do
+        @client.stubs(:dostorage)
+        @client.class.stubs(:facts).returns(@facts)
+        @master.stubs(:getconfig).raises(ArgumentError.new("whev"))
+        @client.expects(:use_cached_config).with(true)
+        @client.getconfig
+    end
+
+    it "should load the retrieved configuration using YAML" do
+        @client.stubs(:dostorage)
+        @client.class.stubs(:facts).returns(@facts)
+        @master.stubs(:getconfig).returns("myconfig")
+
+        config = mock 'config'
+        YAML.expects(:load).with("myconfig").returns(config)
+
+        @client.stubs(:setclasses)
+
+        config.stubs(:classes)
+        config.stubs(:to_configuration).returns(config)
+        config.stubs(:host_config=)
+        config.stubs(:from_cache).returns(true)
+
+        @client.getconfig
+    end
+
+    it "should use the cached configuration if the retrieved configuration cannot be converted from YAML" do
+        @client.stubs(:dostorage)
+        @client.class.stubs(:facts).returns(@facts)
+        @master.stubs(:getconfig).returns("myconfig")
+
+        YAML.expects(:load).with("myconfig").raises(ArgumentError)
+
+        @client.expects(:use_cached_config).with(true)
+
+        @client.getconfig
+    end
+
+    it "should set the classes.txt file with the classes listed in the retrieved configuration" do
+        @client.stubs(:dostorage)
+        @client.class.stubs(:facts).returns(@facts)
+        @master.stubs(:getconfig).returns("myconfig")
+
+        config = mock 'config'
+        YAML.expects(:load).with("myconfig").returns(config)
+
+        config.expects(:classes).returns(:myclasses)
+        @client.expects(:setclasses).with(:myclasses)
+
+        config.stubs(:to_configuration).returns(config)
+        config.stubs(:host_config=)
+        config.stubs(:from_cache).returns(true)
+
+        @client.getconfig
+    end
+
+    it "should convert the retrieved configuration to a RAL configuration" do
+        @client.stubs(:dostorage)
+        @client.class.stubs(:facts).returns(@facts)
+        @master.stubs(:getconfig).returns("myconfig")
+
+        yamlconfig = mock 'yaml config'
+        YAML.stubs(:load).returns(yamlconfig)
+
+        @client.stubs(:setclasses)
+
+        config = mock 'config'
+
+        yamlconfig.stubs(:classes)
+        yamlconfig.expects(:to_configuration).returns(config)
+
+        config.stubs(:host_config=)
+        config.stubs(:from_cache).returns(true)
+
+        @client.getconfig
+    end
+
+    it "should use the cached configuration if the retrieved configuration cannot be converted to a RAL configuration" do
+        @client.stubs(:dostorage)
+        @client.class.stubs(:facts).returns(@facts)
+        @master.stubs(:getconfig).returns("myconfig")
+
+        yamlconfig = mock 'yaml config'
+        YAML.stubs(:load).returns(yamlconfig)
+
+        @client.stubs(:setclasses)
+
+        config = mock 'config'
+
+        yamlconfig.stubs(:classes)
+        yamlconfig.expects(:to_configuration).raises(ArgumentError)
+
+        @client.expects(:use_cached_config).with(true)
+
+        @client.getconfig
+    end
+
+    it "should clear the failed configuration if using the cached configuration after failing to instantiate the retrieved configuration" do
+        @client.stubs(:dostorage)
+        @client.class.stubs(:facts).returns(@facts)
+        @master.stubs(:getconfig).returns("myconfig")
+
+        yamlconfig = mock 'yaml config'
+        YAML.stubs(:load).returns(yamlconfig)
+
+        @client.stubs(:setclasses)
+
+        config = mock 'config'
+
+        yamlconfig.stubs(:classes)
+        yamlconfig.stubs(:to_configuration).raises(ArgumentError)
+
+        @client.stubs(:use_cached_config).with(true)
+
+        @client.expects(:clear)
+
+        @client.getconfig
+    end
+
+    it "should cache the retrieved yaml configuration if it is not from the cache and is valid" do
+        @client.stubs(:dostorage)
+        @client.class.stubs(:facts).returns(@facts)
+        @master.stubs(:getconfig).returns("myconfig")
+
+        yamlconfig = mock 'yaml config'
+        YAML.stubs(:load).returns(yamlconfig)
+
+        @client.stubs(:setclasses)
+
+        config = mock 'config'
+
+        yamlconfig.stubs(:classes)
+        yamlconfig.expects(:to_configuration).returns(config)
+
+        config.stubs(:host_config=)
+
+        config.expects(:from_cache).returns(false)
+
+        @client.expects(:cache).with("myconfig")
+
+        @client.getconfig
+    end
+
+    it "should mark the configuration as a host configuration" do
+        @client.stubs(:dostorage)
+        @client.class.stubs(:facts).returns(@facts)
+        @master.stubs(:getconfig).returns("myconfig")
+
+        yamlconfig = mock 'yaml config'
+        YAML.stubs(:load).returns(yamlconfig)
+
+        @client.stubs(:setclasses)
+
+        config = mock 'config'
+
+        yamlconfig.stubs(:classes)
+        yamlconfig.expects(:to_configuration).returns(config)
+
+        config.stubs(:from_cache).returns(true)
+
+        config.expects(:host_config=).with(true)
+
+        @client.getconfig
+    end
+end
+
+describe Puppet::Network::Client::Master, " when using the cached configuration" do
+    before do
+        @master = mock 'master'
+        @client = Puppet::Network::Client.master.new(
+            :Master => @master
+        )
+        @facts = {"one" => "two", "three" => "four"}
+    end
+
+    it "should return do nothing and true if there is already an in-memory configuration" do
+        @client.configuration = :whatever
+        Puppet::Network::Client::Master.publicize_methods :use_cached_config do
+            @client.use_cached_config.should be_true
+        end
+    end
+
+    it "should return do nothing and false if it has been told there is a failure and :nocacheonfailure is enabled" do
+        Puppet.settings.expects(:value).with(:usecacheonfailure).returns(false)
+        Puppet::Network::Client::Master.publicize_methods :use_cached_config do
+            @client.use_cached_config(true).should be_false
+        end
+    end
+
+    it "should return false if no cached configuration can be found" do
+        @client.expects(:retrievecache).returns(nil)
+        Puppet::Network::Client::Master.publicize_methods :use_cached_config do
+            @client.use_cached_config().should be_false
+        end
+    end
+
+    it "should return false if the cached configuration cannot be instantiated" do
+        YAML.expects(:load).raises(ArgumentError)
+        @client.expects(:retrievecache).returns("whatever")
+        Puppet::Network::Client::Master.publicize_methods :use_cached_config do
+            @client.use_cached_config().should be_false
+        end
+    end
+
+    it "should warn if the cached configuration cannot be instantiated" do
+        YAML.stubs(:load).raises(ArgumentError)
+        @client.stubs(:retrievecache).returns("whatever")
+        Puppet.expects(:warning).with { |m| m.include?("Could not load cache") }
+        Puppet::Network::Client::Master.publicize_methods :use_cached_config do
+            @client.use_cached_config().should be_false
+        end
+    end
+
+    it "should clear the client if the cached configuration cannot be instantiated" do
+        YAML.stubs(:load).raises(ArgumentError)
+        @client.stubs(:retrievecache).returns("whatever")
+        @client.expects(:clear)
+        Puppet::Network::Client::Master.publicize_methods :use_cached_config do
+            @client.use_cached_config().should be_false
+        end
+    end
+
+    it "should return true if the cached configuration can be instantiated" do
+        config = mock 'config'
+        YAML.stubs(:load).returns(config)
+
+        ral_config = mock 'ral config'
+        ral_config.stubs(:from_cache=)
+        ral_config.stubs(:host_config=)
+        config.expects(:to_configuration).returns(ral_config)
+
+        @client.stubs(:retrievecache).returns("whatever")
+        Puppet::Network::Client::Master.publicize_methods :use_cached_config do
+            @client.use_cached_config().should be_true
+        end
+    end
+
+    it "should set the configuration instance variable if the cached configuration can be instantiated" do
+        config = mock 'config'
+        YAML.stubs(:load).returns(config)
+
+        ral_config = mock 'ral config'
+        ral_config.stubs(:from_cache=)
+        ral_config.stubs(:host_config=)
+        config.expects(:to_configuration).returns(ral_config)
+
+        @client.stubs(:retrievecache).returns("whatever")
+        Puppet::Network::Client::Master.publicize_methods :use_cached_config do
+            @client.use_cached_config()
+        end
+
+        @client.configuration.should equal(ral_config)
+    end
+
+    it "should mark the configuration as a host_config if valid" do
+        config = mock 'config'
+        YAML.stubs(:load).returns(config)
+
+        ral_config = mock 'ral config'
+        ral_config.stubs(:from_cache=)
+        ral_config.expects(:host_config=).with(true)
+        config.expects(:to_configuration).returns(ral_config)
+
+        @client.stubs(:retrievecache).returns("whatever")
+        Puppet::Network::Client::Master.publicize_methods :use_cached_config do
+            @client.use_cached_config()
+        end
+
+        @client.configuration.should equal(ral_config)
+    end
+
+    it "should mark the configuration as from the cache if valid" do
+        config = mock 'config'
+        YAML.stubs(:load).returns(config)
+
+        ral_config = mock 'ral config'
+        ral_config.expects(:from_cache=).with(true)
+        ral_config.stubs(:host_config=)
+        config.expects(:to_configuration).returns(ral_config)
+
+        @client.stubs(:retrievecache).returns("whatever")
+        Puppet::Network::Client::Master.publicize_methods :use_cached_config do
+            @client.use_cached_config()
+        end
+
+        @client.configuration.should equal(ral_config)
+    end
+end
diff --git a/spec/unit/node/configuration.rb b/spec/unit/node/configuration.rb
index e9dc6b85d..5780d4fbb 100755
--- a/spec/unit/node/configuration.rb
+++ b/spec/unit/node/configuration.rb
@@ -301,9 +301,9 @@ end
 describe Puppet::Node::Configuration, " when functioning as a resource container" do
     before do
         @config = Puppet::Node::Configuration.new("host")
-        @one = stub 'resource1', :ref => "Me[you]", :configuration= => nil
-        @two = stub 'resource2', :ref => "Me[him]", :configuration= => nil
-        @dupe = stub 'resource3', :ref => "Me[you]", :configuration= => nil
+        @one = stub 'resource1', :ref => "Me[one]", :configuration= => nil
+        @two = stub 'resource2', :ref => "Me[two]", :configuration= => nil
+        @dupe = stub 'resource3', :ref => "Me[one]", :configuration= => nil
     end
 
     it "should provide a method to add one or more resources" do
@@ -376,7 +376,7 @@ describe Puppet::Node::Configuration, " when functioning as a resource container
 
     it "should be able to find resources by reference or by type/title tuple" do
         @config.add_resource @one
-        @config.resource("me", "you").should equal(@one)
+        @config.resource("me", "one").should equal(@one)
     end
 
     it "should have a mechanism for removing resources" do
@@ -386,6 +386,32 @@ describe Puppet::Node::Configuration, " when functioning as a resource container
         @config.resource(@one.ref).should be_nil
         @config.vertex?(@one).should be_false
     end
+
+    it "should have a method for creating aliases for resources" do
+        @config.add_resource @one
+        @config.alias(@one, "other")
+        @config.resource("me", "other").should equal(@one)
+    end
+
+    # This test is the same as the previous, but the behaviour should be explicit.
+    it "should alias using the class name from the resource reference, not the resource class name" do
+        @config.add_resource @one
+        @config.alias(@one, "other")
+        @config.resource("me", "other").should equal(@one)
+    end
+
+    it "should fail to add an alias if the aliased name already exists" do
+        @config.add_resource @one
+        proc { @config.alias @two, "one" }.should raise_error(ArgumentError)
+    end
+
+    it "should remove resource aliases when the target resource is removed" do
+        @config.add_resource @one
+        @config.alias(@one, "other")
+        @one.expects :remove
+        @config.remove_resource(@one)
+        @config.resource("me", "other").should be_nil
+    end
 end
 
 module ApplyingConfigurations
diff --git a/spec/unit/other/transbucket.rb b/spec/unit/other/transbucket.rb
index 8cb9abaa4..0da808460 100755
--- a/spec/unit/other/transbucket.rb
+++ b/spec/unit/other/transbucket.rb
@@ -102,6 +102,18 @@ describe Puppet::TransBucket, " when generating a configuration" do
         @top.to_configuration
     end
 
+    it "should set each TransObject's configuration before converting to a RAL resource" do
+        @middleobj.expects(:configuration=).with { |c| c.is_a?(Puppet::Node::Configuration) }
+        @top.to_configuration
+    end
+
+    it "should set each TransBucket's configuration before converting to a RAL resource" do
+        # each bucket is seen twice in the loop, so we have to handle the case where the config
+        # is set twice
+        @bottom.expects(:configuration=).with { |c| c.is_a?(Puppet::Node::Configuration) }.at_least_once
+        @top.to_configuration
+    end
+
     after do
         Puppet::Type.allclear
     end
diff --git a/spec/unit/other/transobject.rb b/spec/unit/other/transobject.rb
index 144940b7e..eaca855db 100755
--- a/spec/unit/other/transobject.rb
+++ b/spec/unit/other/transobject.rb
@@ -2,114 +2,73 @@
 
 require File.dirname(__FILE__) + '/../../spec_helper'
 
-describe Puppet::TransObject, " when building its search path" do
-end
-
-describe Puppet::TransObject, " when building its search path" do
-end
-#!/usr/bin/env ruby
-
-$:.unshift("../lib").unshift("../../lib") if __FILE__ =~ /\.rb$/
-
-require 'puppet'
 require 'puppet/transportable'
-require 'puppettest'
-require 'puppettest/parsertesting'
-require 'yaml'
 
-class TestTransportable < Test::Unit::TestCase
-    include PuppetTest::ParserTesting
-
-    def test_yamldumpobject
-        obj = mk_transobject
-        obj.to_yaml_properties
-        str = nil
-        assert_nothing_raised {
-            str = YAML.dump(obj)
-        }
-
-        newobj = nil
-        assert_nothing_raised {
-            newobj = YAML.load(str)
-        }
-
-        assert(newobj.name, "Object has no name")
-        assert(newobj.type, "Object has no type")
+describe Puppet::TransObject, " when serializing" do
+    before do
+        @resource = Puppet::TransObject.new("/my/file", "file")
+        @resource["one"] = "test"
+        @resource["two"] = "other"
     end
 
-    def test_yamldumpbucket
-        objects = %w{/etc/passwd /etc /tmp /var /dev}.collect { |d|
-            mk_transobject(d)
-        }
-        bucket = mk_transbucket(*objects)
-        str = nil
-        assert_nothing_raised {
-            str = YAML.dump(bucket)
-        }
-
-        newobj = nil
-        assert_nothing_raised {
-            newobj = YAML.load(str)
-        }
-
-        assert(newobj.name, "Bucket has no name")
-        assert(newobj.type, "Bucket has no type")
+    it "should be able to be dumped to yaml" do
+        proc { YAML.dump(@resource) }.should_not raise_error
     end
 
-    # Verify that we correctly strip out collectable objects, since they should
-    # not be sent to the client.
-    def test_collectstrip
-        top = mk_transtree do |object, depth, width|
-            if width % 2 == 1
-                object.collectable = true
-            end
-        end
+    it "should produce an equivalent yaml object" do
+        text = YAML.dump(@resource)
 
-        assert(top.flatten.find_all { |o| o.collectable }.length > 0,
-            "Could not find any collectable objects")
+        newresource = YAML.load(text)
+        newresource.name.should == "/my/file"
+        newresource.type.should == "file"
+        %w{one two}.each do |param|
+            newresource[param].should == @resource[param]
+        end
+    end
+end
 
-        # Now strip out the collectable objects
-        top.collectstrip!
+describe Puppet::TransObject, " when converting to a RAL resource" do
+    before do
+        @resource = Puppet::TransObject.new("/my/file", "file")
+        @resource["one"] = "test"
+        @resource["two"] = "other"
+    end
 
-        # And make sure they're actually gone
-        assert_equal(0, top.flatten.find_all { |o| o.collectable }.length,
-            "Still found collectable objects")
+    it "should use the resource type's :create method to create the resource" do
+        type = mock 'resource type'
+        type.expects(:create).with(@resource).returns(:myresource)
+        Puppet::Type.expects(:type).with("file").returns(type)
+        @resource.to_type.should == :myresource
     end
 
-    # Make sure our 'delve' command is working
-    def test_delve
-        top = mk_transtree do |object, depth, width|
-            if width % 2 == 1
-                object.collectable = true
-            end
-        end
+    it "should convert to a component instance if the resource type cannot be found" do
+        Puppet::Type.expects(:type).with("file").returns(nil)
+        @resource.expects(:to_component).returns(:mycomponent)
+        @resource.to_type.should == :mycomponent
+    end
+end
 
-        objects = []
-        buckets = []
-        collectable = []
+describe Puppet::TransObject, " when converting to a RAL component instance" do
+    before do
+        @resource = Puppet::TransObject.new("/my/file", "one::two")
+        @resource["one"] = "test"
+        @resource["noop"] = "other"
+    end
 
-        count = 0
-        assert_nothing_raised {
-            top.delve do |object|
-                count += 1
-                if object.is_a? Puppet::TransBucket
-                    buckets << object
-                else
-                    objects << object
-                    if object.collectable
-                        collectable << object
-                    end
-                end
-            end
-        }
+    it "should use a new TransObject whose name is a resource reference of the type and title of the original TransObject" do
+        Puppet::Type::Component.expects(:create).with { |resource| resource.type == :component and resource.name == "One::Two[/my/file]" }.returns(:yay)
+        @resource.to_component.should == :yay
+    end
 
-        top.flatten.each do |obj|
-            assert(objects.include?(obj), "Missing obj %s[%s]" % [obj.type, obj.name])
-        end
+    it "should pass the resource parameters on to the newly created TransObject" do
+        Puppet::Type::Component.expects(:create).with { |resource| resource["noop"] == "other" }.returns(:yay)
+        @resource.to_component.should == :yay
+    end
 
-        assert_equal(collectable.length,
-            top.flatten.find_all { |o| o.collectable }.length,
-            "Found incorrect number of collectable objects")
+    # LAK:FIXME This really isn't the design we want going forward, but it's
+    # good enough for now.
+    it "should not pass resource paramaters that are not metaparams" do
+        Puppet::Type::Component.expects(:create).with { |resource| resource["one"].nil? }.returns(:yay)
+        @resource.to_component.should == :yay
     end
 end
-