Fixing the instances method. It now works when there are already managed resources.

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

2 files changed, 36 insertions, 11 deletions

diff --git a/lib/puppet/metatype/instances.rb b/lib/puppet/metatype/instances.rb
index d1c6d7732..7c951058c 100644
--- a/lib/puppet/metatype/instances.rb
+++ b/lib/puppet/metatype/instances.rb
@@ -257,17 +257,30 @@ class Puppet::Type
         end
 
         # Put the default provider first, then the rest of the suitable providers.
-        packages = {}
+        provider_instances = {}
         providers_by_source.collect do |provider|
             provider.instances.collect do |instance|
-                if other = packages[instance.name]
-                    Puppet.warning "Package %s found in both %s and %s; skipping the %s version" %
-                        [instance.name, other.class.name, instance.class.name, instance.class.name]
+                # First try to get the resource if it already exists
+                if resource = self[instance.name] and resource.provider.class != instance.class
+                    # Skip instances that map to a managed resource with a different provider
                     next
                 end
-                packages[instance.name] = instance
 
-                create(:name => instance.name, :provider => instance, :check => :all)
+                # We always want to use the "first" provider instance we find, unless the resource
+                # is already managed and has a different provider set
+                if other = provider_instances[instance.name]
+                    Puppet.warning "%s %s found in both %s and %s; skipping the %s version" %
+                        [self.name.to_s.capitalize, instance.name, other.class.name, instance.class.name, instance.class.name]
+                    next
+                end
+                provider_instances[instance.name] = instance
+
+                if resource
+                    resource.provider = instance
+                    resource
+                else
+                    create(:name => instance.name, :provider => instance, :check => :all)
+                end
             end
         end.flatten.compact
     end
diff --git a/test/ral/manager/instances.rb b/test/ral/manager/instances.rb
index 142b85c5c..759a5884a 100755
--- a/test/ral/manager/instances.rb
+++ b/test/ral/manager/instances.rb
@@ -43,37 +43,49 @@ class TestTypeInstances < Test::Unit::TestCase
                 @instances
             end
 
-            @names = [:one]
+            @names = [:one, :five, :six]
         end
 
         # A provider with the same source
         @type.provide(:sub, :source => :default, :parent => :default) do
-            @names = [:two]
+            @names = [:two, :seven, :eight]
         end
 
         # An unsuitable provider
         @type.provide(:nope, :parent => :default) do
             confine :exists => "/no/such/file"
-            @names = [:three]
+            @names = [:three, :nine, :ten]
         end
 
         # Another suitable, non-default provider
         @type.provide(:yep, :parent => :default) do
-            @names = [:four]
+            @names = [:four, :seven, :ten]
         end
 
+        # Now make a couple of instances, so we know we correctly match instead of always
+        # trying to create new ones.
+        one = @type.create(:name => :one, :ensure => :present)
+        three = @type.create(:name => :three, :ensure => :present, :provider => :sub)
+        five = @type.create(:name => :five, :ensure => :present, :provider => :yep)
+
         result = nil
         assert_nothing_raised("Could not get instance list") do
             result = @type.instances
         end
 
+        result.each do |resource|
+            assert_instance_of(@type, resource, "Returned non-resource")
+        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 we didn't change the provider type for :five
+        assert_equal(:yep, five.provider.class.name, "Changed provider type when listing resources")
+
         # 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