Merge pull request puppetlabs#3353 from MikaelSmith/bug/master/PUP-1073-pkg-composite-namevar

(PUP-1073) Make package use a composite namevar

Author: Kylo Ginsberg

acceptance/config/git/options.rb

@@ -6,6 +6,7 @@
   ],
   :pre_suite => [
     'setup/git/pre-suite/000_EnvSetup.rb',
+    'setup/common/pre-suite/001_PkgBuildSetup.rb',
     'setup/git/pre-suite/010_TestSetup.rb',
     'setup/git/pre-suite/020_PuppetUserAndGroup.rb',
     'setup/common/pre-suite/025_StopFirewall.rb',

acceptance/config/packages/options.rb

@@ -1,6 +1,7 @@
 {
   :type => 'foss-packages',
   :pre_suite => [
+    'setup/common/pre-suite/001_PkgBuildSetup.rb',
     'setup/packages/pre-suite/010_Install.rb',
     'setup/packages/pre-suite/015_PackageHostsPresets.rb',
     'setup/common/pre-suite/025_StopFirewall.rb',

acceptance/lib/puppet/acceptance/rpm_util.rb

@@ -0,0 +1,94 @@
+module Puppet
+  module Acceptance
+    module RpmUtils
+      # Utilities for creating a basic rpm package and using it in tests
+      @@defaults = {:repo => '/tmp/rpmrepo', :pkg => 'mypkg', :publisher => 'tstpub.lan', :version => '1.0'}
+
+      def clean_rpm(agent, o={})
+        o = @@defaults.merge(o)
+        on agent, "rm -rf #{o[:repo]}"
+        on agent, "yum remove -y #{o[:pkg]}"
+        on agent, "rm -f /etc/yum.repos.d/#{o[:publisher]}.repo"
+      end
+
+      def setup_rpm(agent, o={})
+        o = @@defaults.merge(o)
+        on agent, "mkdir -p #{o[:repo]}/{RPMS,SRPMS,BUILD,SOURCES,SPECS}"
+        on agent, "echo '%_topdir #{o[:repo]}' > ~/.rpmmacros"
+        on agent, "createrepo #{o[:repo]}"
+        on agent, "cat <<EOF > /etc/yum.repos.d/#{o[:publisher]}.repo
+[#{o[:publisher]}]
+name=#{o[:publisher]}
+baseurl=file://#{o[:repo]}/
+enabled=1
+gpgcheck=0
+EOF
+"
+      end
+
+      def send_rpm(agent, o={})
+        o = @@defaults.merge(o)
+        on agent, "mkdir -p #{o[:repo]}/#{o[:pkg]}-#{o[:version]}/usr/bin"
+        on agent, "cat <<EOF > #{o[:repo]}/#{o[:pkg]}
+#!/bin/bash
+echo Hello World
+EOF
+"
+        pkg_name = "#{o[:pkg]}-#{o[:version]}"
+        on agent, "install -m 755 #{o[:repo]}/#{o[:pkg]} #{o[:repo]}/#{pkg_name}/usr/bin"
+        on agent, "tar -zcvf #{o[:repo]}/SOURCES/#{pkg_name}.tar.gz -C #{o[:repo]} #{pkg_name}"
+        on agent, "cat <<EOF > #{o[:repo]}/SPECS/#{o[:pkg]}.spec
+# Don't try fancy stuff like debuginfo, which is useless on binary-only packages. Don't strip binary too
+# Be sure buildpolicy set to do nothing
+%define        __spec_install_post %{nil}
+%define          debug_package %{nil}
+%define        __os_install_post %{_dbpath}/brp-compress
+
+Summary: A very simple toy bin rpm package
+Name: #{o[:pkg]}
+Version: #{o[:version]}
+Release: 1
+License: GPL+
+Group: Development/Tools
+SOURCE0 : %{name}-%{version}.tar.gz
+URL: http://www.puppetlabs.com/
+
+BuildRoot: %{_topdir}/BUILD/%{name}-%{version}-%{release}-root
+
+%description
+%{summary}
+
+%prep
+%setup -q
+
+%build
+# Empty section.
+
+%install
+rm -rf %{buildroot}
+mkdir -p  %{buildroot}
+
+# in builddir
+cp -a * %{buildroot}
+
+
+%clean
+rm -rf %{buildroot}
+
+%files
+%defattr(-,root,root,-)
+%{_bindir}/*
+
+%changelog
+* Mon Dec 01 2014  Michael Smith <[email protected]> #{o[:version]}-1
+- First Build
+
+EOF
+"
+        on agent, "rpmbuild -ba #{o[:repo]}/SPECS/#{o[:pkg]}.spec"
+        on agent, "createrepo --update #{o[:repo]}"
+      end
+    end
+  end
+end
+

acceptance/setup/common/pre-suite/001_PkgBuildSetup.rb

@@ -0,0 +1,16 @@
+test_name "Setup acceptance environment"
+
+step "Ensure package build tools"
+
+require 'puppet/acceptance/install_utils'
+extend Puppet::Acceptance::InstallUtils
+
+PACKAGES = {
+  :redhat => [
+    'rpm-build',
+    'createrepo',
+  ],
+}
+
+install_packages_on(hosts, PACKAGES, :check_if_exists => true)
+

acceptance/tests/resource/package/common_package_name_in_different_providers.rb

@@ -0,0 +1,108 @@
+test_name "ticket 1073: common package name in two different providers should be allowed"
+
+hosts_to_test = agents.select do |agent|
+  agent['platform'].match /(?:centos|el-|fedora)/
+end
+skip_test "No suitable hosts found" if hosts_to_test.empty?
+
+require 'puppet/acceptance/rpm_util'
+extend Puppet::Acceptance::RpmUtils
+
+rpm_options = {:pkg => 'guid', :version => '1.0'}
+
+teardown do
+  step "cleanup"
+  agents.each do |agent|
+    clean_rpm agent, rpm_options
+  end
+end
+
+def verify_state(hosts, pkg, state, match)
+  hosts.each do |agent|
+    # Note yum lists packages as <name>.<arch>
+    on agent, 'yum list installed' do
+      method(match).call(/^#{pkg}\./, stdout)
+    end
+
+    on agent, 'gem list --local' do
+      method(match).call(/^#{pkg} /, stdout)
+    end
+  end
+end
+
+def verify_present(hosts, pkg)
+  verify_state(hosts, pkg, '(?!purged|absent)[^\']+', :assert_match)
+end
+
+def verify_absent(hosts, pkg)
+  verify_state(hosts, pkg, '(?:purged|absent)', :assert_no_match)
+end
+
+# Setup repo and package
+hosts_to_test.each do |agent|
+  clean_rpm agent, rpm_options
+  setup_rpm agent, rpm_options
+  send_rpm agent, rpm_options
+end
+
+verify_absent hosts_to_test, 'guid'
+
+# Test error trying to install duplicate packages
+collide1_manifest = <<MANIFEST
+  package {'guid': ensure => installed}
+  package {'other-guid': name => 'guid', ensure => present}
+MANIFEST
+
+apply_manifest_on(hosts_to_test, collide1_manifest, :acceptable_exit_codes => [1]).each do |result|
+  assert_match(/Error while evaluating a Resource Statement, Cannot alias Package\[other-guid\] to \["guid", nil\]/, "#{result.host}: #{result.stderr}")
+end
+
+verify_absent hosts_to_test, 'guid'
+
+collide2_manifest = <<MANIFEST
+  package {'guid': ensure => '0.1.0', provider => gem}
+  package {'other-guid': name => 'guid', ensure => installed, provider => gem}
+MANIFEST
+
+apply_manifest_on(hosts_to_test, collide2_manifest, :acceptable_exit_codes => [1]).each do |result|
+  assert_match(/Error while evaluating a Resource Statement, Cannot alias Package\[other-guid\] to \["guid", "gem"\]/, "#{result.host}: #{result.stderr}")
+end
+
+verify_absent hosts_to_test, 'guid'
+
+# Test successful parallel installation
+install_manifest = <<MANIFEST
+  package {'guid': ensure => installed}
+
+  package {'gem-guid':
+    provider => gem,
+    name => 'guid',
+    ensure => installed,
+  }
+MANIFEST
+
+apply_manifest_on(hosts_to_test, install_manifest).each do |result|
+  assert_match('Package[guid]/ensure: created', "#{result.host}: #{result.stdout}")
+  assert_match('Package[gem-guid]/ensure: created', "#{result.host}: #{result.stdout}")
+end
+
+verify_present hosts_to_test, 'guid'
+
+# Test removal
+remove_manifest = <<MANIFEST
+  package {'gem-guid':
+    provider => gem,
+    name => 'guid',
+    ensure => absent,
+  }
+
+  package {'guid': ensure => absent}
+MANIFEST
+
+apply_manifest_on(hosts_to_test, remove_manifest).each do |result|
+  assert_match('Package[guid]/ensure: removed', "#{result.host}: #{result.stdout}")
+  assert_match('Package[gem-guid]/ensure: removed', "#{result.host}: #{result.stdout}")
+end
+
+verify_absent hosts_to_test, 'guid'
+

lib/puppet/resource.rb

@@ -314,10 +314,11 @@ def to_s
   end
 
   def uniqueness_key
-    # Temporary kludge to deal with inconsistent use patters
+    # Temporary kludge to deal with inconsistent use patterns; ensure we don't return nil for namevar/:name
     h = self.to_hash
-    h[namevar] ||= h[:name]
-    h[:name]   ||= h[namevar]
+    name = h[namevar] || h[:name] || self.name
+    h[namevar] ||= name
+    h[:name]   ||= name
     h.values_at(*key_attributes.sort_by { |k| k.to_s })
   end
 

lib/puppet/resource/catalog.rb

@@ -111,8 +111,13 @@ def add_resource_to_graph(resource)
   private :add_resource_to_graph
 
   def create_resource_aliases(resource)
-    if resource.respond_to?(:isomorphic?) and resource.isomorphic? and resource.name != resource.title
-      self.alias(resource, resource.uniqueness_key)
+    # Skip creating aliases and checking collisions for non-isomorphic resources.
+    return unless resource.respond_to?(:isomorphic?) and resource.isomorphic?
+
+    # Add an alias if the uniqueness key is valid and not the title, which has already been checked.
+    ukey = resource.uniqueness_key
+    if ukey.any? and ukey != [resource.title]
+      self.alias(resource, ukey)
     end
   end
   private :create_resource_aliases

lib/puppet/type/package.rb

@@ -245,6 +245,37 @@ def should_to_s(newvalue = @should)
       end
     end
 
+    # We call providify here so that we can set provider as a namevar.
+    # Normally this method is called after newtype finishes constructing this
+    # Type class.
+    providify
+    paramclass(:provider).isnamevar
+
+    # We have more than one namevar, so we need title_patterns. However, we
+    # cheat and set the patterns to map to name only and completely ignore
+    # provider. So far, the logic that determines uniqueness appears to just
+    # "Do The Right Thing™" when the provider is explicitly set by the user.
+    #
+    # The following resources will be seen as uniqe by puppet:
+    #
+    #     # Uniqueness Key: ['mysql', nil]
+    #     package{'mysql': }
+    #
+    #     # Uniqueness Key: ['mysql', 'gem']
+    #     package{'gem-mysql':
+    #       name     => 'mysql,
+    #       provider => gem
+    #     }
+    #
+    # This does not handle the case where providers like 'yum' and 'rpm' should
+    # clash. Also, declarations that implicitly use the default provider will
+    # clash with those that explicitly use the default.
+    def self.title_patterns
+      # This is the default title pattern for all types, except hard-wired to
+      # set only name.
+      [ [ /(.*)/m, [ [:name] ] ] ]
+    end
+
     newproperty(:package_settings, :required_features=>:package_settings) do
       desc "Settings that can change the contents or configuration of a package.
 

spec/integration/type/package_spec.rb

@@ -26,3 +26,77 @@ def provider_name(os)
     Puppet::Type.type(:package).defaultprovider.name.should == default_provider
   end
 end
+
+describe Puppet::Type.type(:package), "when packages with the same name are sourced" do
+  before :each do
+    @provider = stub(
+      'provider',
+      :class           => Puppet::Type.type(:package).defaultprovider,
+      :clear           => nil,
+      :satisfies?      => true,
+      :name            => :mock,
+      :validate_source => nil
+    )
+    Puppet::Type.type(:package).defaultprovider.stubs(:new).returns(@provider)
+    Puppet::Type.type(:package).defaultprovider.stubs(:instances).returns([])
+    @package = Puppet::Type.type(:package).new(:name => "yay", :ensure => :present)
+
+    @catalog = Puppet::Resource::Catalog.new
+    @catalog.add_resource(@package)
+  end
+
+  describe "with same title" do
+    before {
+      @alt_package = Puppet::Type.type(:package).new(:name => "yay", :ensure => :present)
+    }
+    it "should give an error" do
+      expect {
+        @catalog.add_resource(@alt_package)
+      }.to raise_error Puppet::Resource::Catalog::DuplicateResourceError, 'Duplicate declaration: Package[yay] is already declared; cannot redeclare'
+    end
+  end
+
+  describe "with different title" do
+    before :each do
+      @alt_package = Puppet::Type.type(:package).new(:name => "yay", :title => "gem-yay", :ensure => :present)
+    end
+
+    it "should give an error" do
+      provider_name = Puppet::Type.type(:package).defaultprovider.name
+      expect {
+        @catalog.add_resource(@alt_package)
+      }.to raise_error ArgumentError, "Cannot alias Package[gem-yay] to [\"yay\", :#{provider_name}]; resource [\"Package\", \"yay\", :#{provider_name}] already declared"
+    end
+  end
+
+  describe "from multiple providers" do
+    provider_class = Puppet::Type.type(:package).provider(:gem)
+
+    before :each do
+      @alt_provider = provider_class.new
+      @alt_package = Puppet::Type.type(:package).new(:name => "yay", :title => "gem-yay", :provider => @alt_provider, :ensure => :present)
+      @catalog.add_resource(@alt_package)
+    end
+
+    describe "when it should be present" do
+      [:present, :latest, "1.0"].each do |state|
+        it "should do nothing if it is #{state.to_s}" do
+          @provider.expects(:properties).returns(:ensure => state).at_least_once
+          @alt_provider.expects(:properties).returns(:ensure => state).at_least_once
+          @catalog.apply
+        end
+      end
+
+      [:purged, :absent].each do |state|
+        it "should install if it is #{state.to_s}" do
+          @provider.stubs(:properties).returns(:ensure => state)
+          @provider.expects(:install)
+          @alt_provider.stubs(:properties).returns(:ensure => state)
+          @alt_provider.expects(:install)
+          @catalog.apply
+        end
+      end
+    end
+  end
+end
+