(PDK-889) Write support for multiple namevars

.rubocop.yml

@@ -4,7 +4,7 @@ inherit_from: .rubocop_todo.yml
 AllCops:
   TargetRubyVersion: '2.1'
   Include:
-  - "./**/*.rb"
+  - "**/*.rb"
   Exclude:
   - bin/*
   - ".vendor/**/*"

lib/puppet/resource_api.rb

@@ -10,6 +10,8 @@ def register_type(definition)
     raise Puppet::DevError, 'requires a Hash as definition, not %{other_type}' % { other_type: definition.class } unless definition.is_a? Hash
     raise Puppet::DevError, 'requires a name' unless definition.key? :name
     raise Puppet::DevError, 'requires attributes' unless definition.key? :attributes
+    raise Puppet::DevError, 'Type must not define an attribute called `title`' if definition[:attributes].key? :title
+
     validate_ensure(definition)
 
     definition[:features] ||= []
@@ -37,8 +39,6 @@ def register_type(definition)
 
     Puppet::Type.newtype(definition[:name].to_sym) do
       @docs = definition[:docs]
-      has_namevar = false
-      namevar_name = nil
 
       # Keeps a copy of the provider around. Weird naming to avoid clashes with puppet's own `provider` member
       define_singleton_method(:my_provider) do
@@ -65,6 +65,7 @@ def type_definition
       define_method(:initialize) do |attributes|
         # $stderr.puts "A: #{attributes.inspect}"
         if attributes.is_a? Puppet::Resource
+          @title = attributes.title
           attributes = attributes.to_hash
         else
           @called_from_resource = true
@@ -74,10 +75,14 @@ def type_definition
           attributes = my_provider.canonicalize(context, [attributes])[0]
         end
 
-        # puppet defines a name attribute, but this only works for types that support name
+        # the `Puppet::Resource::Ral.find` method, when `instances` does not return a match, uses a Hash with a `:name` key to create
+        # an "absent" resource. This is often hit by `puppet resource`. This needs to work, even if the namevar is not called `name`.
+        # This bit here relies on the default `title_patterns` (see below) to match the title back to the first (and often only) namevar
         if definition[:attributes][:name].nil? && attributes[:title].nil?
-          attributes[:title] = attributes[:name]
-          attributes.delete(:name)
+          attributes[:title] = attributes.delete(:name)
+          if attributes[:title].nil? && !type_definition.namevars.empty?
+            attributes[:title] = @title
+          end
         end
 
         super(attributes)
@@ -106,7 +111,7 @@ def type_definition
 
         @missing_attrs -= [:ensure] if @called_from_resource
 
-        raise_missing_params if @missing_params.any?
+        raise_missing_params if @missing_params.any? && @called_from_resource != true
       end
 
       definition[:attributes].each do |name, options|
@@ -121,7 +126,7 @@ def type_definition
         # TODO: using newparam everywhere would suppress change reporting
         #       that would allow more fine-grained reporting through context,
         #       but require more invest in hooking up the infrastructure to emulate existing data
-        param_or_property = if [:read_only, :parameter, :namevar].include? options[:behaviour]
+        param_or_property = if [:read_only, :parameter].include? options[:behaviour]
                               :newparam
                             else
                               :newproperty
@@ -139,11 +144,7 @@ def type_definition
           end
 
           if options[:behaviour] == :namevar
-            # puts 'setting namevar'
-            # raise Puppet::DevError, "namevar must be called 'name', not '#{name}'" if name.to_s != 'name'
             isnamevar
-            has_namevar = true
-            namevar_name = name
           end
 
           # read-only values do not need type checking, but can have default values
@@ -256,10 +257,12 @@ def call_provider(value); end
             property = definition[:attributes][key.first]
             attr_def[key.first] = property
           end
-          if resource_hash[namevar_name].nil?
-            raise Puppet::ResourceError, "`#{name}.get` did not return a value for the `#{namevar_name}` namevar attribute"
+          context.type.namevars.each do |namevar|
+            if resource_hash[namevar].nil?
+              raise Puppet::ResourceError, "`#{name}.get` did not return a value for the `#{namevar}` namevar attribute"
+            end
           end
-          Puppet::ResourceApi::TypeShim.new(resource_hash[namevar_name], resource_hash, name, namevar_name, attr_def)
+          Puppet::ResourceApi::TypeShim.new(resource_hash, name, context.type.namevars, attr_def)
         end
       end
 
@@ -270,7 +273,7 @@ def call_provider(value); end
         current_state = if type_definition.feature?('simple_get_filter')
                           my_provider.get(context, [title]).first
                         else
-                          my_provider.get(context).find { |h| h[namevar_name] == title }
+                          my_provider.get(context).find { |h| namevar_match?(h) }
                         end
 
         strict_check(current_state) if current_state && type_definition.feature?('canonicalize')
@@ -280,7 +283,7 @@ def call_provider(value); end
             result[k] = v
           end
         else
-          result[namevar_name] = title
+          result[:title] = title
           result[:ensure] = :absent if type_definition.ensurable?
         end
 
@@ -294,6 +297,12 @@ def call_provider(value); end
         result
       end
 
+      define_method(:namevar_match?) do |item|
+        context.type.namevars.all? do |namevar|
+          item[namevar] == @parameters[namevar].value
+        end
+      end
+
       define_method(:flush) do
         raise_missing_attrs
 
@@ -354,7 +363,7 @@ def call_provider(value); end
         #:nocov:
         # codecov fails to register this multiline as covered, even though simplecov does.
         message = <<MESSAGE.strip
-#{definition[:name]}[#{current_state[namevar_name]}]#get has not provided canonicalized values.
+#{definition[:name]}[#{@title}]#get has not provided canonicalized values.
 Returned values:       #{current_state.inspect}
 Canonicalized values:  #{state_clone.inspect}
 MESSAGE
@@ -375,7 +384,7 @@ def call_provider(value); end
       end
 
       define_singleton_method(:title_patterns) do
-        [[%r{\A(.*)\Z}m, [[namevar_name]]]]
+        [[%r{(.*)}m, [[type_definition.namevars.first]]]]
       end
 
       def context

lib/puppet/resource_api/glue.rb

@@ -4,42 +4,40 @@ module Puppet; end # rubocop:disable Style/Documentation
 module Puppet::ResourceApi
   # A trivial class to provide the functionality required to push data through the existing type/provider parts of puppet
   class TypeShim
-    attr_reader :values, :typename, :namevar, :attr_def
+    attr_reader :values, :typename, :namevars, :attr_def
 
-    def initialize(title, resource_hash, typename, namevarname, attr_def)
+    def initialize(resource_hash, typename, namevars, attr_def)
       # internalize and protect - needs to go deeper
-      @values = resource_hash.dup
-      # "name" is a privileged key
-      @values[namevarname] = title
-      @values.freeze
+      @values = resource_hash.dup.freeze
 
       @typename = typename
-      @namevar = namevarname
+      @namevars = namevars
       @attr_def = attr_def
+      @resource = ResourceShim.new(@values, @typename, @namevars, @attr_def)
     end
 
     def to_resource
-      ResourceShim.new(@values, @typename, @namevar, @attr_def)
+      @resource
     end
 
     def name
-      values[@namevar]
+      @resource.title
     end
   end
 
   # A trivial class to provide the functionality required to push data through the existing type/provider parts of puppet
   class ResourceShim
-    attr_reader :values, :typename, :namevar, :attr_def
+    attr_reader :values, :typename, :namevars, :attr_def
 
-    def initialize(resource_hash, typename, namevarname, attr_def)
+    def initialize(resource_hash, typename, namevars, attr_def)
       @values = resource_hash.dup.freeze # whatevs
       @typename = typename
-      @namevar = namevarname
+      @namevars = namevars
       @attr_def = attr_def
     end
 
     def title
-      values[@namevar]
+      values[@namevars.first]
     end
 
     def prune_parameters(*_args)
@@ -48,7 +46,7 @@ def prune_parameters(*_args)
     end
 
     def to_manifest
-      (["#{@typename} { #{Puppet::Parameter.format_value_for_display(values[@namevar])}: "] + values.keys.reject { |k| k == @namevar }.map do |k|
+      (["#{@typename} { #{Puppet::Parameter.format_value_for_display(title)}: "] + filtered_keys.map do |k|
         cs = ' '
         ce = ''
         if attr_def[k][:behaviour] && attr_def[k][:behaviour] == :read_only
@@ -61,8 +59,13 @@ def to_manifest
 
     # Convert our resource to yaml for Hiera purposes.
     def to_hierayaml
-      attributes = Hash[values.keys.reject { |k| k == @namevar }.map { |k| [k.to_s, values[k]] }]
-      YAML.dump('type' => { values[@namevar] => attributes }).split("\n").drop(2).join("\n") + "\n"
+      attributes = Hash[filtered_keys.map { |k| [k.to_s, values[k]] }]
+      YAML.dump('type' => { title => attributes }).split("\n").drop(2).join("\n") + "\n"
+    end
+
+    # attribute names that are not title or namevars
+    def filtered_keys
+      values.keys.reject { |k| k == :title || attr_def[k][:behaviour] == :namevar && @namevars.size == 1 }
     end
   end
 end

lib/puppet/resource_api/type_definition.rb

@@ -15,6 +15,12 @@ def ensurable?
     @definition[:attributes].key?(:ensure)
   end
 
+  def namevars
+    @namevars ||= @definition[:attributes].select { |_name, options|
+      options.key?(:behaviour) && options[:behaviour] == :namevar
+    }.keys
+  end
+
   # rubocop complains when this is named has_feature?
   def feature?(feature)
     supported = (definition[:features] && definition[:features].include?(feature))

spec/acceptance/namevar_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper'
 require 'open3'
 
-RSpec.describe 'type with multiple namevars' do
+RSpec.describe 'a type with multiple namevars' do
   let(:common_args) { '--verbose --trace --modulepath spec/fixtures' }
 
   describe 'using `puppet resource`' do
@@ -10,17 +10,97 @@
       expect(stdout_str.strip).to match %r{^multiple_namevar}
       expect(status).to eq 0
     end
-    it 'is returns the required resource correctly' do
-      stdout_str, status = Open3.capture2e("puppet resource #{common_args} multiple_namevar yum")
-      expect(stdout_str.strip).to match %r{^multiple_namevar \{ \'yum\'}
-      expect(stdout_str.strip).to match %r{ensure => \'present\'}
+    it 'returns the required resource correctly' do
+      stdout_str, status = Open3.capture2e("puppet resource #{common_args} multiple_namevar php-yum package=php manager=yum")
+      expect(stdout_str.strip).to match %r{^multiple_namevar \{ \'php-yum\'}
+      expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'}
+      expect(stdout_str.strip).to match %r{package\s*=> \'php\'}
+      expect(stdout_str.strip).to match %r{manager\s*=> \'yum\'}
       expect(status).to eq 0
     end
-    it 'does not match the first namevar' do
+    it 'returns the match if first namevar is used as title and other namevars are present' do
+      stdout_str, status = Open3.capture2e("puppet resource #{common_args} multiple_namevar php manager=gem")
+      expect(stdout_str.strip).to match %r{^multiple_namevar \{ \'php\'}
+      expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'}
+      expect(status).to eq 0
+    end
+    it 'returns the match if title matches a namevar value' do
       stdout_str, status = Open3.capture2e("puppet resource #{common_args} multiple_namevar php")
       expect(stdout_str.strip).to match %r{^multiple_namevar \{ \'php\'}
-      expect(stdout_str.strip).to match %r{ensure => \'absent\'}
+      expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'}
+      expect(status).to eq 0
+    end
+    it 'creates a previously absent resource if all namevars are provided' do
+      stdout_str, status = Open3.capture2e("puppet resource #{common_args} multiple_namevar php manager=wibble")
+      expect(stdout_str.strip).to match %r{^multiple_namevar \{ \'php\'}
+      expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'}
+      expect(stdout_str.strip).to match %r{package\s*=> \'php\'}
+      expect(stdout_str.strip).to match %r{manager\s*=> \'wibble\'}
+      expect(status).to eq 0
+    end
+    it 'properly identifies an absent resource if only the title is provided' do
+      stdout_str, status = Open3.capture2e("puppet resource #{common_args} multiple_namevar wibble")
+      expect(stdout_str.strip).to match %r{^multiple_namevar \{ \'wibble\'}
+      expect(stdout_str.strip).to match %r{ensure\s*=> \'absent\'}
       expect(status).to eq 0
     end
+    it 'will remove an existing resource' do
+      stdout_str, status = Open3.capture2e("puppet resource #{common_args} multiple_namevar php manager=gem ensure=absent")
+      expect(stdout_str.strip).to match %r{^multiple_namevar \{ \'php\'}
+      expect(stdout_str.strip).to match %r{ensure\s*=> \'absent\'}
+      expect(status).to eq 0
+    end
+    it 'will ignore the title if namevars are provided' do
+      stdout_str, status = Open3.capture2e("puppet resource #{common_args} multiple_namevar whatever package=php manager=gem")
+      expect(stdout_str.strip).to match %r{^multiple_namevar \{ \'whatever\'}
+      expect(stdout_str.strip).to match %r{package\s*=> \'php\'}
+      expect(stdout_str.strip).to match %r{manager\s*=> \'gem\'}
+      expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'}
+      expect(status).to eq 0
+    end
+  end
+
+  describe 'using `puppet apply`' do
+    require 'tempfile'
+
+    let(:common_args) { super() + ' --detailed-exitcodes' }
+
+    # run Open3.capture2e only once to get both output, and exitcode # rubocop:disable RSpec/InstanceVariable
+    before(:each) do
+      Tempfile.create('acceptance') do |f|
+        f.write(manifest)
+        f.close
+        @stdout_str, @status = Open3.capture2e("puppet apply #{common_args} #{f.path}")
+      end
+    end
+
+    context 'when managing a present instance' do
+      let(:manifest) { 'multiple_namevar { foo: package => php, manager => yum }' }
+
+      it { expect(@stdout_str).not_to match %r{multiple_namevar} }
+      it { expect(@status.exitstatus).to eq 0 }
+    end
+
+    context 'when creating a previously absent instance' do
+      let(:manifest) { 'multiple_namevar { foo: package => php, manager => yum2 }' }
+
+      it { expect(@stdout_str).to match %r{Multiple_namevar\[foo\]/ensure: defined 'ensure' as 'present'} }
+      it { expect(@status.exitstatus).to eq 2 }
+    end
+
+    context 'when managing an absent instance' do
+      let(:manifest) { 'multiple_namevar { foo: package => php, manager => yum2, ensure => absent }' }
+
+      it { expect(@stdout_str).not_to match %r{multiple_namevar} }
+      it { expect(@status.exitstatus).to eq 0 }
+    end
+
+    context 'when removing a previously present instance' do
+      let(:manifest) { 'multiple_namevar { foo: package => php, manager => yum, ensure => absent }' }
+
+      it { expect(@stdout_str).to match %r{Multiple_namevar\[foo\]/ensure: undefined 'ensure' from 'present'} }
+      it { expect(@status.exitstatus).to eq 2 }
+    end
+    # rubocop:enable RSpec/InstanceVariable
   end
 end

spec/acceptance/simple_get_filter_spec.rb

@@ -18,19 +18,19 @@
     end
 
     context 'when using `get` to access a specific resource' do
-      it 'does not receive names array' do
+      it 'returns resource' do
         stdout_str, status = Open3.capture2e("puppet resource #{common_args} test_simple_get_filter foo")
 
         expect(stdout_str.strip).to match %r{test_string\s*=>\s*'default'}
         expect(status).to eq 0
       end
     end
 
-    context 'when using `get` to access a specific resource' do
+    context 'when using `get` to remove a specific resource' do
       it 'receives names array' do
         stdout_str, status = Open3.capture2e("puppet resource #{common_args} test_simple_get_filter foo ensure=absent")
 
-        expect(stdout_str.strip).to match %r{ensure\s*=>\s*'absent'}
+        expect(stdout_str.strip).to match %r{undefined 'ensure' from 'present'}
         expect(stdout_str.strip).to match %r{test_string\s*=>\s*'foo found'}
         expect(status).to eq 0
       end

spec/acceptance/validation_spec.rb

@@ -22,14 +22,8 @@
     context 'when listing an absent instance' do
       it 'requires params' do
         output, status = Open3.capture2e("puppet resource #{common_args} test_validation nope")
-        expect(output.strip).to match %r{Test_validation\[nope\] failed: The following mandatory parameters were not provided}
-        expect(status.exitstatus).to eq 1
-      end
-
-      it 'is not satisfied with params only' do
-        output, status = Open3.capture2e("puppet resource #{common_args} test_validation nope param=2")
-        expect(output.strip).to match %r{Test_validation\[nope\].*The following mandatory attributes were not provided}
-        expect(status.exitstatus).to eq 1
+        expect(output.strip).to match %r{ensure => 'absent'}
+        expect(status.exitstatus).to eq 0
       end
     end