Significantly reworked the type => provider interface with respect to

    listing existing provider instances.  The class method on both
    class heirarchies has been renamed to 'instances', to start.  Providers
    are now expected to return provider instances, instead of creating
    resources, and the resource's 'instances' method is expected to
    find the matching resource, if any, and set the resource's
    provider appropriately.  This *significantly* reduces the reliance on
    effectively global state (resource references in the resource classes).
    This global state will go away soon.

    Along with this change, the 'prefetch' class method on providers now
    accepts the list of resources for prefetching.  This again reduces
    reliance on global state, and makes the execution path much easier
    to follow.



git-svn-id: https://reductivelabs.com/svn/puppet/trunk@2551 980ebf18-57e1-0310-9a29-db15c13687c0

11 files changed, 242 insertions, 89 deletions

diff --git a/test/network/handler/resource.rb b/test/network/handler/resource.rb
index 8d1fa1087..589d22d83 100755
--- a/test/network/handler/resource.rb
+++ b/test/network/handler/resource.rb
@@ -183,8 +183,8 @@ class TestResourceServer < Test::Unit::TestCase
         Puppet::Type.type(:schedule).mkdefaultschedules
 
         Puppet::Type.eachtype do |type|
-            unless type.respond_to? :list
-                Puppet.warning "%s does not respond to :list" % type.name
+            unless type.respond_to? :instances
+                Puppet.warning "%s does not respond to :instances" % type.name
                 next
             end
             next unless type.name == :package
@@ -212,7 +212,7 @@ class TestResourceServer < Test::Unit::TestCase
             count = 0
             described = {}
             Puppet.info "listing again"
-            type.list.each do |obj|
+            type.instances.each do |obj|
                 assert_instance_of(type, obj)
 
                 break if count > 5
diff --git a/test/other/transactions.rb b/test/other/transactions.rb
index 1e0b6377d..4cb65dbe3 100755
--- a/test/other/transactions.rb
+++ b/test/other/transactions.rb
@@ -124,8 +124,8 @@ class TestTransactions < Test::Unit::TestCase
 
         # Now create a provider
         type.provide(:prefetch) do
-            def self.prefetch
-                $prefetched = true
+            def self.prefetch(resources)
+                $prefetched = resources
             end
         end
 
@@ -140,7 +140,7 @@ class TestTransactions < Test::Unit::TestCase
             trans.prefetch
         end
 
-        assert_equal(true, $prefetched, "type prefetch was not called")
+        assert_equal({inst.title => inst}, $prefetched, "type prefetch was not called")
 
         # Now make sure it gets called from within evaluate()
         $prefetched = false
@@ -148,7 +148,7 @@ class TestTransactions < Test::Unit::TestCase
             trans.evaluate
         end
 
-        assert_equal(true, $prefetched, "evaluate did not call prefetch")
+        assert_equal({inst.title => inst}, $prefetched, "evaluate did not call prefetch")
     end
 
     def test_refreshes_generate_events
diff --git a/test/ral/manager/instances.rb b/test/ral/manager/instances.rb
new file mode 100755
index 000000000..142b85c5c
--- /dev/null
+++ b/test/ral/manager/instances.rb
@@ -0,0 +1,82 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke A. Kanies on 2007-06-10.
+#  Copyright (c) 2007. All rights reserved.
+
+$:.unshift("../../lib") if __FILE__ =~ /\.rb$/
+
+require 'puppettest'
+
+class TestTypeInstances < Test::Unit::TestCase
+    include PuppetTest
+
+    def setup
+        super
+        @type = Puppet::Type.newtype(:instance_test) do
+            newparam(:name) {}
+            ensurable
+        end
+        cleanup { Puppet::Type.rmtype(:instance_test) }
+    end
+
+    # Make sure the instances class method works as expected.
+    def test_instances
+        # First make sure it throws an error when there are no providers
+        assert_raise(Puppet::DevError, "Did not fail when no providers are present") do
+            @type.instances
+        end
+
+        # Now add a couple of providers
+
+        # The default
+        @type.provide(:default) do
+            defaultfor :operatingsystem => Facter.value(:operatingsystem)
+            mk_resource_methods
+            class << self
+                attr_accessor :names
+            end
+            def self.instance(name)
+                new(:name => name, :ensure => :present)
+            end
+            def self.instances
+                @instances ||= names.collect { |name| instance(name) }
+                @instances
+            end
+
+            @names = [:one]
+        end
+
+        # A provider with the same source
+        @type.provide(:sub, :source => :default, :parent => :default) do
+            @names = [:two]
+        end
+
+        # An unsuitable provider
+        @type.provide(:nope, :parent => :default) do
+            confine :exists => "/no/such/file"
+            @names = [:three]
+        end
+
+        # Another suitable, non-default provider
+        @type.provide(:yep, :parent => :default) do
+            @names = [:four]
+        end
+
+        result = nil
+        assert_nothing_raised("Could not get instance list") do
+            result = @type.instances
+        end
+
+        assert_equal(:one, result[0].name, "Did not get default instances first")
+        assert_equal(@type.provider(:default).instances[0], result[0].provider, "Provider instances were not maintained")
+
+        resources = result.inject({}) { |hash, res| hash[res.name] = res; hash }
+        assert(resources.include?(:four), "Did not get resources from other suitable providers")
+        assert(! resources.include?(:three), "Got resources from unsuitable providers")
+
+        # Now make sure the resources have an 'ensure' property to go with the value in the provider
+        assert(resources[:one].send(:instance_variable_get, "@parameters").include?(:ensure), "Did not create ensure property")
+    end
+end
+
+# $Id$
diff --git a/test/ral/manager/provider.rb b/test/ral/manager/provider.rb
index f3a7d719f..a9748e12a 100755
--- a/test/ral/manager/provider.rb
+++ b/test/ral/manager/provider.rb
@@ -8,47 +8,84 @@ require 'mocha'
 class TestTypeProviders < Test::Unit::TestCase
 	include PuppetTest
 
-    # Make sure default providers behave correctly
-    def test_defaultproviders
-        # Make a fake type
-        type = Puppet::Type.newtype(:defaultprovidertest) do
-            newparam(:name) do end
+    def setup
+        super
+        @type = Puppet::Type.newtype(:provider_test) do
+            newparam(:name) {}
+            ensurable
         end
+        cleanup { Puppet::Type.rmtype(:provider_test) }
+    end
 
-        cleanup { Puppet::Type.rmtype(:defaultprovidertest) }
-
-        basic = type.provide(:basic) do
+    # Make sure default providers behave correctly
+    def test_defaultproviders
+        basic = @type.provide(:basic) do
             defaultfor :operatingsystem => :somethingelse,
                 :operatingsystemrelease => :yayness
         end
 
-        assert_equal(basic, type.defaultprovider)
-        type.defaultprovider = nil
+        assert_equal(basic, @type.defaultprovider)
+        @type.defaultprovider = nil
 
-        greater = type.provide(:greater) do
+        greater = @type.provide(:greater) do
             defaultfor :operatingsystem => Facter.value("operatingsystem")
         end
 
-        assert_equal(greater, type.defaultprovider)
+        assert_equal(greater, @type.defaultprovider)
     end
 
     # Make sure the provider is always the first parameter created.
     def test_provider_sorting
-        type = Puppet::Type.newtype(:sorttest) do
-            newparam(:name) {}
-            ensurable
-        end
-        cleanup { Puppet::Type.rmtype(:sorttest) }
-
         should = [:name, :ensure]
-        assert_equal(should, type.allattrs.reject { |p| ! should.include?(p) },
+        assert_equal(should, @type.allattrs.reject { |p| ! should.include?(p) },
             "Got wrong order of parameters")
 
-        type.provide(:yay) { }
+        @type.provide(:yay) { }
         should = [:name, :provider, :ensure]
-        assert_equal(should, type.allattrs.reject { |p| ! should.include?(p) },
+        assert_equal(should, @type.allattrs.reject { |p| ! should.include?(p) },
             "Providify did not reorder parameters")
     end
+
+    # Make sure that provider instances can be passed in directly.
+    def test_name_or_provider
+        provider = @type.provide(:testing) do
+        end
+
+        # first make sure we can pass the name in
+        resource = nil
+        assert_nothing_raised("Could not create provider instance by name") do
+            resource = @type.create :name => "yay", :provider => :testing
+        end
+
+        assert_instance_of(provider, resource.provider, "Did not create provider instance")
+
+        # Now make sure we can pass in an instance
+        provinst = provider.new(:name => "foo")
+        assert_nothing_raised("Could not pass in provider instance") do
+            resource = @type.create :name => "foo", :provider => provinst
+        end
+
+        assert_equal(provinst, resource.provider, "Did not retain provider instance")
+
+        # Now make sure unsuitable provider instances still throw errors
+        provider = @type.provide(:badprov) do
+            confine :exists => "/no/such/file"
+        end
+
+        inst = provider.new(:name => "bar")
+        assert_raise(Puppet::Error, "Did not fail on unsuitable provider instance") do
+            resource = @type.create :name => "bar", :provider => inst
+        end
+
+        # And make sure the provider must be a valid provider type for this resource
+        pkgprov = Puppet::Type.type(:package).create(:name => "yayness").provider
+        assert(provider, "did not get package provider")
+
+        assert_raise(Puppet::Error, "Did not fail on invalid provider instance") do
+            resource = @type.create :name => "bar", :provider => pkgprov
+        end
+
+    end
 end
 
 # $Id$
diff --git a/test/ral/providers/package.rb b/test/ral/providers/package.rb
index 345fbb0e6..ab7c0130d 100755
--- a/test/ral/providers/package.rb
+++ b/test/ral/providers/package.rb
@@ -234,17 +234,17 @@ class TestPackageProvider < Test::Unit::TestCase
 
     # Make sure all of the suitable providers on our platform can successfully
     # list.
-    def test_listing
+    def test_instances
         Puppet::Type.type(:package).suitableprovider.each do |provider|
             result = nil
             assert_nothing_raised("Could not list %s packages" % provider.name) do
-                result = provider.list
+                result = provider.instances
             end
             result.each do |pkg|
-                assert_instance_of(Puppet::Type.type(:package), pkg,
-                    "%s returned non-package" % provider.name)
-                assert_equal(provider.name, pkg.provider.class.name,
-                    "%s did not set provider correctly" % provider.name)
+                assert_instance_of(provider, pkg,
+                    "%s returned non-provider" % provider.name)
+                assert_equal(provider.name, pkg.class.name,
+                    "Provider %s returned an instance of a different provider" %  provider.name)
             end
         end
     end
diff --git a/test/ral/providers/parsedfile.rb b/test/ral/providers/parsedfile.rb
index efe8105ca..2d9f7ae9d 100755
--- a/test/ral/providers/parsedfile.rb
+++ b/test/ral/providers/parsedfile.rb
@@ -90,8 +90,9 @@ class TestParsedFile < Test::Unit::TestCase
         fakeresource = fakeresource(:testparsedfiletype, "yay")
 
         file = prov.new(fakeresource)
+        assert(file, "Could not make provider")
 
-        assert_nothing_raised do
+        assert_nothing_raised("Could not set provider name") do
             file.name = :yayness
         end
 
@@ -206,31 +207,30 @@ class TestParsedFile < Test::Unit::TestCase
         resource = mkresource "bill", :target => :file1
         default = mkresource "will", :target => :default
 
+        resources = {"bill" => resource, "will" => default}
+
         assert_nothing_raised do
-            prov.prefetch
+            prov.prefetch(resources)
         end
 
         # Make sure we prefetched our resources.
-        assert_equal("b", resource.provider.one)
-        assert_equal("b", default.provider.one)
-        assert_equal("d", default.provider.two)
+        assert_equal("b", resource.provider.one, "did not prefetch resource from file1")
+        assert_equal("b", default.provider.one, "did not prefetch resource from default")
+        assert_equal("d", default.provider.two, "did not prefetch resource from default")
 
         # Now list all of them and make sure we get everything back
-        hashes = nil
+        providers = nil
         assert_nothing_raised do
-            hashes = prov.list
+            providers = prov.instances
         end
 
-        names = nil
-        assert_nothing_raised do
-            names = prov.list_by_name
+        providers.each do |provider|
+            assert_instance_of(prov, provider, "'instances' class method did not return providers")
         end
 
         %w{bill jill will}.each do |name|
-            assert(hashes.find { |r| r[:name] == name},
+            assert(providers.find { |provider| provider.name == name},
                 "Did not return %s in list" % name)
-            assert(names.include?(name),
-                "Did not return %s in list_by_name" % name)
         end
     end
 
@@ -242,25 +242,27 @@ class TestParsedFile < Test::Unit::TestCase
         target = :yayness
         prov.target_object(target).write "yay b d"
 
-        resource = mkresource "yay", :target => :yayness
-
+        records = nil
         assert_nothing_raised do
-            prov.prefetch_target(:yayness)
+            records = prov.prefetch_target(:yayness)
         end
 
         # Now make sure we correctly got the hash
-        mprov = resource.provider
-        assert_equal("b", mprov.one)
-        assert_equal("d", mprov.two)
+        record = records.find { |r| r[:name] == "yay" }
+        assert(record, "Did not get record in prefetch_target")
+        assert_equal("b", record[:one])
+        assert_equal("d", record[:two])
     end
 
     def test_prefetch_match
         prov = mkprovider
 
-        prov.meta_def(:match) do |record|
+        prov.meta_def(:match) do |record, resources|
             # Look for matches on :one
-            self.resource_type.find do |m|
-                m.should(:one).to_s == record[:one].to_s
+            if res = resources.find { |name, resource| resource.should(:one).to_s == record[:one].to_s }
+                res[1]
+            else
+                nil
             end
         end
 
@@ -271,7 +273,7 @@ class TestParsedFile < Test::Unit::TestCase
         resource = mkresource "yay", :target => :yayness, :one => "b"
 
         assert_nothing_raised do
-            prov.prefetch_target(:yayness)
+            prov.prefetch("yay" => resource)
         end
 
         # Now make sure we correctly got the hash
@@ -301,22 +303,35 @@ class TestParsedFile < Test::Unit::TestCase
 
         # Lastly, create a resource with separate is and should values
         mtarget = tempfile()
-        # istarget = tempfile()
         files[:resources] = mtarget
-        # files[:isresources] = istarget
         resource = mkresource "yay", :target => mtarget
-        # resource.is = [:target, istarget]
 
         assert(resource.should(:target), "Did not get a value for target")
-        # assert(resource.is(:target), "Did not get a value for target")
 
         list = nil
+
+        # First run it without the resource
         assert_nothing_raised do
             list = prov.targets
         end
 
+        # Make sure it got the first two, but not the resources file
         files.each do |name, file|
-            assert(list.include?(file), "Provider did not find %s file" % name)
+            if name == :resources
+                assert(! list.include?(file), "Provider somehow found resource target when no resource was passed")
+            else
+                assert(list.include?(file), "Provider did not find %s file" % name)
+            end
+        end
+
+        # Now list with the resource passed
+        assert_nothing_raised do
+            list = prov.targets("yay" => resource)
+        end
+
+        # And make sure we get all three files
+        files.each do |name, file|
+            assert(list.include?(file), "Provider did not find %s file when resource was passed" % name)
         end
     end
 
@@ -331,11 +346,12 @@ class TestParsedFile < Test::Unit::TestCase
         # Create some resources.
         one = mkresource "one", :one => "a", :two => "c", :target => :yayness
         two = mkresource "two", :one => "b", :two => "d", :target => :yayness
+        resources = {"one" => one, "two" => two}
 
         # Write out a file with different data.
         prov.target_object(:yayness).write "one b d\ntwo a c"
 
-        prov.prefetch
+        prov.prefetch(resources)
 
         # Apply and flush the first resource.
         assert_nothing_raised do
@@ -343,7 +359,7 @@ class TestParsedFile < Test::Unit::TestCase
         end
         assert_nothing_raised { one.flush }
 
-        # Make sure it changed our file
+        # Make sure it didn't clear out our property hash
         assert_equal(:a, one.provider.one)
         assert_equal(:c, one.provider.two)
 
@@ -360,7 +376,7 @@ class TestParsedFile < Test::Unit::TestCase
             "Wrote out other resource")
 
         # Now fetch the data again and make sure we're still right
-        assert_nothing_raised { prov.prefetch }
+        assert_nothing_raised { prov.prefetch(resources) }
         assert_equal("a", one.provider.one)
         assert_equal("a", two.provider.one)
 
@@ -368,9 +384,11 @@ class TestParsedFile < Test::Unit::TestCase
         assert_nothing_raised { apply(two) }
         assert_nothing_raised { two.flush }
 
+        # And make sure it didn't clear our hash
         assert_equal(:b, two.provider.one)
     end
 
+    # Make sure it works even if the file does not currently exist
     def test_creating_file
         prov = mkprovider
         prov.clear
@@ -387,7 +405,7 @@ class TestParsedFile < Test::Unit::TestCase
             resource.flush
         end
 
-        assert(prov.target_object(:basic).read.include?("yay a c"),
+        assert_equal("yay a c\n", prov.target_object(:basic).read,
             "Did not create file")
 
         # Make a change
@@ -399,8 +417,8 @@ class TestParsedFile < Test::Unit::TestCase
         end
 
         # And make sure our resource doesn't appear twice in the file.
-        assert_equal("yay b c\n",
-            prov.target_object(:basic).read)
+        assert_equal("yay b c\n", prov.target_object(:basic).read,
+            "Wrote record to file twice")
     end
 
     # Make sure a record can switch targets.
@@ -472,7 +490,7 @@ class TestParsedFile < Test::Unit::TestCase
         notdisk = mkresource "notdisk", :target => :first
 
         prov.target_object(:first).write "ondisk a c\n"
-        prov.prefetch
+        prov.prefetch("ondisk" => ondisk, "notdisk" => notdisk)
 
         assert_equal(:present, notdisk.should(:ensure),
             "Did not get default ensure value")
@@ -544,7 +562,7 @@ class TestParsedFile < Test::Unit::TestCase
 
         prov.target_object(:yayness).write "bill a c\njill b d"
 
-        list = @type.list
+        list = @type.instances
 
         bill = list.find { |r| r[:name] == "bill" }
         jill = list.find { |r| r[:name] == "jill" }
@@ -639,7 +657,6 @@ class TestParsedFile < Test::Unit::TestCase
         targeted = {:target => "target"}
         prov.send(:instance_variable_set, "@records", records)
         prov.expects(:retrieve).with(target).returns([targeted])
-        prov.expects(:target_records).with(target).returns([targeted])
 
         prov.expects(:prefetch_hook).with([targeted]).returns([targeted])
 
diff --git a/test/ral/providers/provider.rb b/test/ral/providers/provider.rb
index 9e59f2008..7fcf3f59d 100755
--- a/test/ral/providers/provider.rb
+++ b/test/ral/providers/provider.rb
@@ -293,6 +293,34 @@ class TestProvider < Test::Unit::TestCase
         assert_equal(:base, other.source, "source did not override")
         assert_equal(:base, other.source, "source did not override")
     end
+
+    # Make sure we can initialize with either a resource or a hash, or none at all.
+    def test_initialize
+        test = @type.provide(:test)
+
+        inst = @type.create :name => "boo"
+        prov = nil
+        assert_nothing_raised("Could not init with a resource") do
+            prov = test.new(inst)
+        end
+        assert_equal(prov.resource, inst, "did not set resource correctly")
+        assert_equal(inst.name, prov.name, "did not get resource name")
+
+        params = {:name => :one, :ensure => :present}
+        assert_nothing_raised("Could not init with a hash") do
+            prov = test.new(params)
+        end
+        assert_equal(params, prov.send(:instance_variable_get, "@property_hash"), "did not set resource correctly")
+        assert_equal(:one, prov.name, "did not get name from hash")
+
+        assert_nothing_raised("Could not init with no argument") do
+            prov = test.new()
+        end
+
+        assert_raise(Puppet::DevError, "did not fail when no name is present") do
+            prov.name
+        end
+    end
 end
 
 class TestProviderFeatures < Test::Unit::TestCase
diff --git a/test/ral/types/cron.rb b/test/ral/types/cron.rb
index ead815071..be94463d2 100755
--- a/test/ral/types/cron.rb
+++ b/test/ral/types/cron.rb
@@ -287,9 +287,11 @@ class TestCron < Test::Unit::TestCase
         # Write it to our file
         assert_apply(cron)
 
+        @crontype.clear
+
         crons = []
         assert_nothing_raised {
-            @crontype.list.each do |cron|
+            @crontype.instances.each do |cron|
                 crons << cron
             end
         }
diff --git a/test/ral/types/package.rb b/test/ral/types/package.rb
index 9aafc3468..e37cdfe06 100755
--- a/test/ral/types/package.rb
+++ b/test/ral/types/package.rb
@@ -11,7 +11,6 @@ class TestPackages < Test::Unit::TestCase
     include PuppetTest::FileTesting
     def setup
         super
-        #@list = Puppet.type(:package).getpkglist
         Puppet.type(:package).clear
         @type = Puppet::Type.type(:package)
     end
diff --git a/test/ral/types/resources.rb b/test/ral/types/resources.rb
index 56603c611..82d4e225d 100755
--- a/test/ral/types/resources.rb
+++ b/test/ral/types/resources.rb
@@ -13,7 +13,7 @@ class TestResources < Test::Unit::TestCase
 	def add_purge_lister
         # Now define the list method
         class << @purgetype
-            def list
+            def instances
                 $purgemembers.values
             end
         end
@@ -85,21 +85,9 @@ class TestResources < Test::Unit::TestCase
             purger = @type.create :name => "purgetest", :noop => true, :loglevel => :warning
         end
         assert(purger, "did not get purger manager")
-        
-        # Make sure we throw an error, because the purger type does
-        # not support listing.
-        
-        # It should work when we set it to false
-        assert_nothing_raised do
-            purger[:purge] = false
-        end
-        # but not true
-        assert_raise(ArgumentError) do
-            purger[:purge] = true
-        end
         add_purge_lister()
         
-        assert_equal($purgemembers.values.sort, @purgetype.list.sort)
+        assert_equal($purgemembers.values.sort, @purgetype.instances.sort)
         
         # and it should now succeed
         assert_nothing_raised do
diff --git a/test/ral/types/sshkey.rb b/test/ral/types/sshkey.rb
index e8ee575d9..9a20dc251 100755
--- a/test/ral/types/sshkey.rb
+++ b/test/ral/types/sshkey.rb
@@ -59,9 +59,9 @@ class TestSSHKey < Test::Unit::TestCase
         return key
     end
 
-    def test_list
+    def test_instances
         assert_nothing_raised {
-            Puppet.type(:sshkey).defaultprovider.list
+            Puppet.type(:sshkey).instances
         }
 
         count = 0