From 41605ca02407bec1ac0d19ae31d21894b063a795 Mon Sep 17 00:00:00 2001
From: Dave Armstrong <david.armstrong@puppet.com>
Date: Fri, 11 May 2018 11:39:14 +0100
Subject: [PATCH 1/5] (PDK-889) Write support for multiple namevars

---
 lib/puppet/resource_api.rb                    | 38 +++++++++------
 lib/puppet/resource_api/glue.rb               | 30 ++++++------
 lib/puppet/resource_api/type_definition.rb    | 10 ++++
 spec/acceptance/namevar_spec.rb               | 46 ++++++++++++++++---
 spec/acceptance/simple_get_filter_spec.rb     |  6 +--
 spec/acceptance/validation_spec.rb            | 10 +---
 .../multiple_namevar/multiple_namevar.rb      | 32 ++++++++-----
 .../test_simple_get_filter.rb                 |  4 +-
 spec/puppet/resource_api/glue_spec.rb         | 20 ++++----
 spec/puppet/resource_api_spec.rb              | 44 +++++++++---------
 10 files changed, 147 insertions(+), 93 deletions(-)

diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb
index b31f4ed5..55a03881 100644
--- a/lib/puppet/resource_api.rb
+++ b/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
@@ -77,6 +78,9 @@ def type_definition
         # puppet defines a name attribute, but this only works for types that support name
         if definition[:attributes][:name].nil? && attributes[:title].nil?
           attributes[:title] = attributes[:name]
+          if attributes[:title].nil? && !type_definition.namevars.empty?
+            attributes[:title] = @title
+          end
           attributes.delete(:name)
         end
 
@@ -106,7 +110,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 +125,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 +143,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 +256,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 +272,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 +282,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 +296,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 if @parameters[namevar].respond_to? :value
+        end
+      end
+
       define_method(:flush) do
         raise_missing_attrs
 
@@ -354,7 +362,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 +383,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
diff --git a/lib/puppet/resource_api/glue.rb b/lib/puppet/resource_api/glue.rb
index c431ed58..c080aa94 100644
--- a/lib/puppet/resource_api/glue.rb
+++ b/lib/puppet/resource_api/glue.rb
@@ -4,42 +4,42 @@ 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
 
       @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 +48,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 +61,12 @@ 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
+
+    def filtered_keys
+      values.keys.reject { |k| k == :title || attr_def[k][:behaviour] == :namevar && @namevars.size == 1 }
     end
   end
 end
diff --git a/lib/puppet/resource_api/type_definition.rb b/lib/puppet/resource_api/type_definition.rb
index 69349ef5..0a917aab 100644
--- a/lib/puppet/resource_api/type_definition.rb
+++ b/lib/puppet/resource_api/type_definition.rb
@@ -15,6 +15,16 @@ def ensurable?
     @definition[:attributes].key?(:ensure)
   end
 
+  def namevars
+    if @namevars.nil?
+      @namevars = []
+      @definition[:attributes].each do |name, options|
+        @namevars << name if options.key?(:behaviour) && options[:behaviour] == :namevar
+      end
+    end
+    @namevars
+  end
+
   # rubocop complains when this is named has_feature?
   def feature?(feature)
     supported = (definition[:features] && definition[:features].include?(feature))
diff --git a/spec/acceptance/namevar_spec.rb b/spec/acceptance/namevar_spec.rb
index f05f60ba..eace131e 100644
--- a/spec/acceptance/namevar_spec.rb
+++ b/spec/acceptance/namevar_spec.rb
@@ -10,16 +10,50 @@
       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 'properly identifies an 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*=> \'absent\'}
+      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
diff --git a/spec/acceptance/simple_get_filter_spec.rb b/spec/acceptance/simple_get_filter_spec.rb
index 2c39c7b4..090626aa 100644
--- a/spec/acceptance/simple_get_filter_spec.rb
+++ b/spec/acceptance/simple_get_filter_spec.rb
@@ -18,7 +18,7 @@
     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'}
@@ -26,11 +26,11 @@
       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
diff --git a/spec/acceptance/validation_spec.rb b/spec/acceptance/validation_spec.rb
index 28784bf6..b1d0a033 100644
--- a/spec/acceptance/validation_spec.rb
+++ b/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
 
diff --git a/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb b/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
index 3ab01f61..56c26876 100644
--- a/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
+++ b/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
@@ -1,10 +1,10 @@
 require 'puppet/resource_api'
-require 'puppet/resource_api/simple_provider'
 
 # Implementation for the title_provider type using the Resource API.
-class Puppet::Provider::MultipleNamevar::MultipleNamevar < Puppet::ResourceApi::SimpleProvider
-  def get(_context)
-    [
+class Puppet::Provider::MultipleNamevar::MultipleNamevar
+
+  def initialize
+    defaults = [
       { package: 'php', manager: 'yum', ensure: 'present' },
       { package: 'php', manager: 'gem', ensure: 'present' },
       { package: 'mysql', manager: 'yum', ensure: 'present' },
@@ -12,17 +12,27 @@ def get(_context)
       { package: 'foo', manager: 'bar', ensure: 'present' },
       { package: 'bar', manager: 'foo', ensure: 'present' },
     ]
+    @current_values ||= defaults
   end
 
-  def create(context, name, should)
-    context.notice("Creating '#{name}' with #{should.inspect}")
-  end
+  def set(context, changes)
+    changes.each do |name, change|
+        if change[:is] != change[:should]
 
-  def update(context, name, should)
-    context.notice("Updating '#{name}' with #{should.inspect}")
+          match = @current_values.find do |item|
+            context.type.namevars.all? do |namevar|
+              item[namevar] == change[:should][namevar]
+            end
+          end
+          match[:ensure] = change[:should][:ensure] if match
+
+        Puppet.notice("Unable to find matching resource.") if match.nil?
+        end
+    end
+    @current_values
   end
 
-  def delete(context, name)
-    context.notice("Deleting '#{name}'")
+  def get(_context)
+    @current_values
   end
 end
diff --git a/spec/fixtures/test_module/lib/puppet/provider/test_simple_get_filter/test_simple_get_filter.rb b/spec/fixtures/test_module/lib/puppet/provider/test_simple_get_filter/test_simple_get_filter.rb
index f54f2e31..bd602716 100644
--- a/spec/fixtures/test_module/lib/puppet/provider/test_simple_get_filter/test_simple_get_filter.rb
+++ b/spec/fixtures/test_module/lib/puppet/provider/test_simple_get_filter/test_simple_get_filter.rb
@@ -21,14 +21,14 @@ def get(_context, names = nil)
       [
         {
           name: 'foo',
-          ensure: 'absent',
+          ensure: 'present',
           test_string: 'foo found',
         },
       ]
     else
       [
         {
-          name: 'foo',
+          name: names.first,
           ensure: 'present',
           test_string: 'not foo',
         },
diff --git a/spec/puppet/resource_api/glue_spec.rb b/spec/puppet/resource_api/glue_spec.rb
index 01023f92..c674fbd2 100644
--- a/spec/puppet/resource_api/glue_spec.rb
+++ b/spec/puppet/resource_api/glue_spec.rb
@@ -7,48 +7,44 @@
 RSpec.describe 'the dirty bits' do
   describe Puppet::ResourceApi::TypeShim do
     subject(:instance) do
-      described_class.new('title', { attr: 'value', attr_ro: 'fixed' }, 'typename', :namevarname,
+      described_class.new({ attr: 'value', attr_ro: 'fixed' }, 'typename', [:namevarname],
                           namevarname: { type: 'String', behaviour: :namevar, desc: 'the title' },
                           attr: { type: 'String', desc: 'a string parameter' },
                           attr_ro: { type: 'String', desc: 'a string readonly', behaviour: :read_only })
     end
 
     describe '.values' do
-      it { expect(instance.values).to eq(namevarname: 'title', attr: 'value', attr_ro: 'fixed') }
+      it { expect(instance.values).to eq(attr: 'value', attr_ro: 'fixed') }
     end
 
     describe '.typename' do
       it { expect(instance.typename).to eq 'typename' }
     end
 
-    describe '.name' do
-      it { expect(instance.name).to eq 'title' }
-    end
-
-    describe '.namevar' do
-      it { expect(instance.namevar).to eq :namevarname }
+    describe '.namevars' do
+      it { expect(instance.namevars).to eq [:namevarname] }
     end
 
     describe '.to_resource' do
       it { expect(instance.to_resource).to be_a Puppet::ResourceApi::ResourceShim }
 
       describe '.values' do
-        it { expect(instance.to_resource.values).to eq(namevarname: 'title', attr: 'value', attr_ro: 'fixed') }
+        it { expect(instance.to_resource.values).to eq(attr: 'value', attr_ro: 'fixed') }
       end
 
       describe '.typename' do
         it { expect(instance.to_resource.typename).to eq 'typename' }
       end
 
-      describe '.namevar' do
-        it { expect(instance.to_resource.namevar).to eq :namevarname }
+      describe '.namevars' do
+        it { expect(instance.to_resource.namevars).to eq [:namevarname] }
       end
     end
   end
 
   describe Puppet::ResourceApi::ResourceShim do
     subject(:instance) do
-      described_class.new({ namevarname: title, attr: 'value', attr_ro: 'fixed' }, 'typename', :namevarname,
+      described_class.new({ namevarname: title, attr: 'value', attr_ro: 'fixed' }, 'typename', [:namevarname],
                           namevarname: { type: 'String', behaviour: :namevar, desc: 'the title' },
                           attr: { type: 'String', desc: 'a string parameter' },
                           attr_ro: { type: 'String', desc: 'a string readonly', behaviour: :read_only })
diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb
index 00a36ed3..bce762f8 100644
--- a/spec/puppet/resource_api_spec.rb
+++ b/spec/puppet/resource_api_spec.rb
@@ -111,8 +111,8 @@
       subject(:type) { Puppet::Type.type(:with_string) }
 
       it { is_expected.not_to be_nil }
-      it { expect(type.properties.first.doc).to match %r{the description} }
-      it { expect(type.properties.first.name).to eq :test_string }
+      it { expect(type.properties.first.doc).to match %r{the title} }
+      it { expect(type.properties.first.name).to eq :name }
 
       def extract_values(function)
         result = []
@@ -398,7 +398,7 @@ def set(_context, _changes); end
             }
           end
 
-          it('its name is set correctly') { expect(retrieved_info[:name]).to eq 'does_not_exist' }
+          it('its title is set correctly') { expect(retrieved_info[:title]).to eq 'does_not_exist' }
           it('its properties are set correctly') {
             expect(retrieved_info[:test_string]).to be_nil
           }
@@ -513,30 +513,30 @@ def set(_context, _changes); end
       subject(:type) { Puppet::Type.type(:with_parameters) }
 
       it { is_expected.not_to be_nil }
-      it { expect(type.parameters[1]).to eq :test_string }
+      it { expect(type.parameters[0]).to eq :test_string }
     end
 
     describe 'an instance of this type' do
-      subject(:instance) { Puppet::Type.type(:with_parameters).new(params) }
+      subject(:instance) { Puppet::Type.type(:with_parameters).new(Puppet::Resource.new('with_parameters', 'test', params)) }
 
       let(:params) do
-        { title: 'test', test_boolean: true, test_integer: 15, test_float: 1.23,
-          test_variant_pattern: '0xAEF123FF', test_url: 'http://example.com' }
+        { parameters: { test_boolean: true, test_integer: 15, test_float: 1.23,
+                        test_variant_pattern: '0xAEF123FF', test_url: 'http://example.com' } }
       end
 
       it('uses defaults correctly') { expect(instance[:test_string]).to eq 'default value' }
 
       context 'when setting a value for the parameters' do
         let(:params) do
-          {
-            title: 'test',
+          { parameters: {
+            name: 'test',
             test_string: 'somevalue',
             test_boolean: 'true',
             test_integer: '-1',
             test_float: '-1.5',
             test_variant_pattern: 'a' * 8,
             test_url: 'hkp://example.com',
-          }
+          } }
         end
 
         it('the test_string value is set correctly') { expect(instance[:test_string]).to eq 'somevalue' }
@@ -548,9 +548,7 @@ def set(_context, _changes); end
 
       context 'when mandatory parameters are missing' do
         let(:params) do
-          {
-            title: 'test',
-          }
+          { parameters: { name: 'test' } }
         end
 
         it { expect { instance }.to raise_exception Puppet::ResourceError, %r{The following mandatory parameters were not provided} }
@@ -577,7 +575,7 @@ def set(_context, _changes); end
       subject(:type) { Puppet::Type.type(:no_type) }
 
       it { is_expected.not_to be_nil }
-      it { expect(type.parameters[0]).to eq :name }
+      it { expect(type.parameters.size).to eq 0 }
     end
   end
 
@@ -600,7 +598,7 @@ def set(_context, _changes); end
       subject(:type) { Puppet::Type.type(:behaviour) }
 
       it { is_expected.not_to be_nil }
-      it { expect(type.parameters[0]).to eq :name }
+      it { expect(type.parameters.size).to eq 0 }
     end
   end
 
@@ -633,7 +631,7 @@ def set(_context, _changes); end
       subject(:type) { Puppet::Type.type(:init_behaviour) }
 
       it { is_expected.not_to be_nil }
-      it { expect(type.parameters[0]).to eq :name }
+      it { expect(type.parameters.size).to eq 0 }
     end
 
     describe 'an instance of the type' do
@@ -743,7 +741,7 @@ def set(_context, _changes); end
       subject(:type) { Puppet::Type.type(:read_only_behaviour) }
 
       it { is_expected.not_to be_nil }
-      it { expect(type.parameters[0]).to eq :name }
+      it { expect(type.parameters[0]).to eq :immutable }
     end
 
     describe 'an instance of the type' do
@@ -826,36 +824,36 @@ def set(_context, _changes); end
     it { expect { described_class.register_type(definition) }.not_to raise_error }
 
     describe 'an instance of this type' do
-      subject(:instance) { Puppet::Type.type(:not_name_namevar).new(params) }
+      subject(:instance) { Puppet::Type.type(:not_name_namevar) }
 
       context 'with only a :title' do
         let(:params) { { title: 'test' } }
 
-        it('the namevar is set to the title') { expect(instance[:not_name]).to eq 'test' }
+        it('the namevar is set to the title') { expect(instance.new(params)[:not_name]).to eq 'test' }
       end
 
       context 'with only a :name' do
         let(:params) { { name: 'test' } }
 
-        it('the namevar is set to the name') { expect(instance[:not_name]).to eq 'test' }
+        it('the namevar is set to the name') { expect(instance.new(params)[:not_name]).to eq 'test' }
       end
 
       context 'with only the namevar' do
         let(:params) { { not_name: 'test' } }
 
-        it('the namevar is set to the name') { expect(instance[:not_name]).to eq 'test' }
+        it('the namevar is set to the name') { expect { instance.new(params) }.to raise_error Puppet::Error, %r{Title or name must be provided} }
       end
 
       context 'with :title, and the namevar' do
         let(:params) { { title: 'some title', not_name: 'test' } }
 
-        it('the namevar is set to the name') { expect(instance[:not_name]).to eq 'test' }
+        it('the namevar is set to the name') { expect(instance.new(params)[:not_name]).to eq 'test' }
       end
 
       context 'with :name, and the namevar' do
         let(:params) { { name: 'some title', not_name: 'test' } }
 
-        it('the namevar is set to the name') { expect(instance[:not_name]).to eq 'test' }
+        it('the namevar is set to the name') { expect(instance.new(params)[:not_name]).to eq 'test' }
       end
     end
 

From 5b31f19a148a71ec8f35520d881681c1eb57b091 Mon Sep 17 00:00:00 2001
From: David Schmitt <david.schmitt@puppet.com>
Date: Mon, 21 May 2018 12:05:39 +0100
Subject: [PATCH 2/5] Fix rubocop.yml Include pattern to work with rubocop
 0.56.0

---
 .rubocop.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.rubocop.yml b/.rubocop.yml
index 9022236f..4ad704e2 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -4,7 +4,7 @@ inherit_from: .rubocop_todo.yml
 AllCops:
   TargetRubyVersion: '2.1'
   Include:
-  - "./**/*.rb"
+  - "**/*.rb"
   Exclude:
   - bin/*
   - ".vendor/**/*"

From 2b82eb5de968ae2909b0117eb261bda83c5224e3 Mon Sep 17 00:00:00 2001
From: David Schmitt <david.schmitt@puppet.com>
Date: Mon, 21 May 2018 12:11:51 +0100
Subject: [PATCH 3/5] pdk validate -a on the fixture module

---
 .../multiple_namevar/multiple_namevar.rb      | 20 +++++++++----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb b/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
index 56c26876..3568c72c 100644
--- a/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
+++ b/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
@@ -2,7 +2,6 @@
 
 # Implementation for the title_provider type using the Resource API.
 class Puppet::Provider::MultipleNamevar::MultipleNamevar
-
   def initialize
     defaults = [
       { package: 'php', manager: 'yum', ensure: 'present' },
@@ -16,18 +15,17 @@ def initialize
   end
 
   def set(context, changes)
-    changes.each do |name, change|
-        if change[:is] != change[:should]
-
-          match = @current_values.find do |item|
-            context.type.namevars.all? do |namevar|
-              item[namevar] == change[:should][namevar]
-            end
-          end
-          match[:ensure] = change[:should][:ensure] if match
+    changes.each do |_name, change|
+      next unless change[:is] != change[:should]
 
-        Puppet.notice("Unable to find matching resource.") if match.nil?
+      match = @current_values.find do |item|
+        context.type.namevars.all? do |namevar|
+          item[namevar] == change[:should][namevar]
         end
+      end
+      match[:ensure] = change[:should][:ensure] if match
+
+      Puppet.notice('Unable to find matching resource.') if match.nil?
     end
     @current_values
   end

From e6e68c7de707695c0476c3d96b3969687f0ede51 Mon Sep 17 00:00:00 2001
From: David Schmitt <david.schmitt@puppet.com>
Date: Mon, 21 May 2018 13:39:40 +0100
Subject: [PATCH 4/5] Minor cleanups, improved comments

---
 lib/puppet/resource_api.rb                             |  9 +++++----
 lib/puppet/resource_api/glue.rb                        |  5 ++---
 lib/puppet/resource_api/type_definition.rb             | 10 +++-------
 .../provider/multiple_namevar/multiple_namevar.rb      |  3 +--
 4 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb
index 55a03881..4a067500 100644
--- a/lib/puppet/resource_api.rb
+++ b/lib/puppet/resource_api.rb
@@ -75,13 +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[:title] = attributes.delete(:name)
           if attributes[:title].nil? && !type_definition.namevars.empty?
             attributes[:title] = @title
           end
-          attributes.delete(:name)
         end
 
         super(attributes)
@@ -298,7 +299,7 @@ def call_provider(value); end
 
       define_method(:namevar_match?) do |item|
         context.type.namevars.all? do |namevar|
-          item[namevar] == @parameters[namevar].value if @parameters[namevar].respond_to? :value
+          item[namevar] == @parameters[namevar].value
         end
       end
 
diff --git a/lib/puppet/resource_api/glue.rb b/lib/puppet/resource_api/glue.rb
index c080aa94..cbe29bb2 100644
--- a/lib/puppet/resource_api/glue.rb
+++ b/lib/puppet/resource_api/glue.rb
@@ -8,9 +8,7 @@ class TypeShim
 
     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.freeze
+      @values = resource_hash.dup.freeze
 
       @typename = typename
       @namevars = namevars
@@ -65,6 +63,7 @@ def to_hierayaml
       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
diff --git a/lib/puppet/resource_api/type_definition.rb b/lib/puppet/resource_api/type_definition.rb
index 0a917aab..c3ea845f 100644
--- a/lib/puppet/resource_api/type_definition.rb
+++ b/lib/puppet/resource_api/type_definition.rb
@@ -16,13 +16,9 @@ def ensurable?
   end
 
   def namevars
-    if @namevars.nil?
-      @namevars = []
-      @definition[:attributes].each do |name, options|
-        @namevars << name if options.key?(:behaviour) && options[:behaviour] == :namevar
-      end
-    end
-    @namevars
+    @namevars ||= @definition[:attributes].select { |_name, options|
+      options.key?(:behaviour) && options[:behaviour] == :namevar
+    }.keys
   end
 
   # rubocop complains when this is named has_feature?
diff --git a/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb b/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
index 3568c72c..0a9d903b 100644
--- a/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
+++ b/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
@@ -3,7 +3,7 @@
 # Implementation for the title_provider type using the Resource API.
 class Puppet::Provider::MultipleNamevar::MultipleNamevar
   def initialize
-    defaults = [
+    @current_values ||= [
       { package: 'php', manager: 'yum', ensure: 'present' },
       { package: 'php', manager: 'gem', ensure: 'present' },
       { package: 'mysql', manager: 'yum', ensure: 'present' },
@@ -11,7 +11,6 @@ def initialize
       { package: 'foo', manager: 'bar', ensure: 'present' },
       { package: 'bar', manager: 'foo', ensure: 'present' },
     ]
-    @current_values ||= defaults
   end
 
   def set(context, changes)

From 1b0ce3d1aa58f4f7ebee75b7f22b0d946dfd159a Mon Sep 17 00:00:00 2001
From: David Schmitt <david.schmitt@puppet.com>
Date: Mon, 21 May 2018 14:09:13 +0100
Subject: [PATCH 5/5] Add `apply`-based acceptance tests

This also fixes the multiple_namevar provider to remember newly added
resources.
---
 spec/acceptance/namevar_spec.rb               | 52 +++++++++++++++++--
 .../multiple_namevar/multiple_namevar.rb      | 12 +++--
 2 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/spec/acceptance/namevar_spec.rb b/spec/acceptance/namevar_spec.rb
index eace131e..e189d4da 100644
--- a/spec/acceptance/namevar_spec.rb
+++ b/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
@@ -30,10 +30,12 @@
       expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'}
       expect(status).to eq 0
     end
-    it 'properly identifies an absent resource if all namevars are provided' do
+    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*=> \'absent\'}
+      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
@@ -57,4 +59,48 @@
       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
diff --git a/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb b/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
index 0a9d903b..857e73b9 100644
--- a/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
+++ b/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb
@@ -14,7 +14,7 @@ def initialize
   end
 
   def set(context, changes)
-    changes.each do |_name, change|
+    changes.each do |name, change|
       next unless change[:is] != change[:should]
 
       match = @current_values.find do |item|
@@ -22,11 +22,13 @@ def set(context, changes)
           item[namevar] == change[:should][namevar]
         end
       end
-      match[:ensure] = change[:should][:ensure] if match
-
-      Puppet.notice('Unable to find matching resource.') if match.nil?
+      if match
+        match[:ensure] = change[:should][:ensure]
+      else
+        context.created([name], message: 'Adding new record')
+        @current_values << change[:should].dup
+      end
     end
-    @current_values
   end
 
   def get(_context)