Merge pull request #36 from DavidS/pdk-822-namevar-handling

(PDK-822) Implement proper namevar handling

lib/puppet/resource_api.rb

@@ -80,18 +80,6 @@ def my_provider
 
           # read-only values do not need type checking, but can have default values
           if options[:behaviour] != :read_only
-            # TODO: this should use Pops infrastructure to avoid hardcoding stuff, and enhance type fidelity
-            # validate do |v|
-            #   type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type]).normalize
-            #   if type.instance?(v)
-            #     return true
-            #   else
-            #     inferred_type = Puppet::Pops::Types::TypeCalculator.infer_set(v)
-            #     error_msg = Puppet::Pops::Types::TypeMismatchDescriber.new.describe_mismatch("#{DEFINITION[:name]}.#{name}", type, inferred_type)
-            #     raise Puppet::ResourceError, error_msg
-            #   end
-            # end
-
             if options.key? :default
               defaultto options[:default]
             end
@@ -165,7 +153,7 @@ def my_provider
         # force autoloading of the provider
         provider(name)
         my_provider.get(context).map do |resource_hash|
-          Puppet::ResourceApi::TypeShim.new(resource_hash[namevar_name], resource_hash, name)
+          Puppet::ResourceApi::TypeShim.new(resource_hash[namevar_name], resource_hash, name, namevar_name)
         end
       end
 
@@ -183,7 +171,7 @@ def my_provider
             result[k] = v
           end
         else
-          result[:name] = title
+          result[namevar_name] = title
           result[:ensure] = :absent
         end
 
@@ -225,7 +213,7 @@ def my_provider
         #:nocov:
         # codecov fails to register this multiline as covered, even though simplecov does.
         message = <<MESSAGE.strip
-#{definition[:name]}[#{current_state[:name]}]#get has not provided canonicalized values.
+#{definition[:name]}[#{current_state[namevar_name]}]#get has not provided canonicalized values.
 Returned values:       #{current_state.inspect}
 Canonicalized values:  #{state_clone.inspect}
 MESSAGE

lib/puppet/resource_api/glue.rb

@@ -2,37 +2,40 @@
 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
+    attr_reader :values, :typename, :namevar
 
-    def initialize(title, resource_hash, typename)
+    def initialize(title, resource_hash, typename, namevarname)
       # internalize and protect - needs to go deeper
-      @values        = resource_hash.dup
+      @values = resource_hash.dup
       # "name" is a privileged key
-      @values[:name] = title
-      @typename = typename
+      @values[namevarname] = title
       @values.freeze
+
+      @typename = typename
+      @namevar = namevarname
     end
 
     def to_resource
-      ResourceShim.new(@values, @typename)
+      ResourceShim.new(@values, @typename, @namevar)
     end
 
     def name
-      values[:name]
+      values[@namevar]
     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
+    attr_reader :values, :typename, :namevar
 
-    def initialize(resource_hash, typename)
+    def initialize(resource_hash, typename, namevarname)
       @values = resource_hash.dup.freeze # whatevs
       @typename = typename
+      @namevar = namevarname
     end
 
     def title
-      values[:name]
+      values[@namevar]
     end
 
     def prune_parameters(*_args)
@@ -41,12 +44,12 @@ def prune_parameters(*_args)
     end
 
     def to_manifest
-      (["#{@typename} { #{values[:name].inspect}: "] + values.keys.reject { |k| k == :name }.map { |k| "  #{k} => #{Puppet::Parameter.format_value_for_display(values[k])}," } + ['}']).join("\n")
+      (["#{@typename} { #{values[@namevar].inspect}: "] + values.keys.reject { |k| k == @namevar }.map { |k| "  #{k} => #{Puppet::Parameter.format_value_for_display(values[k])}," } + ['}']).join("\n")
     end
 
     # Convert our resource to yaml for Hiera purposes.
     def to_hierayaml
-      (["  #{values[:name]}: "] + values.keys.reject { |k| k == :name }.map { |k| "    #{k}: #{Puppet::Parameter.format_value_for_display(values[k])}" }).join("\n") + "\n"
+      (["  #{values[@namevar]}: "] + values.keys.reject { |k| k == @namevar }.map { |k| "    #{k}: #{Puppet::Parameter.format_value_for_display(values[k])}" }).join("\n") + "\n"
     end
   end
 end

spec/classes/titles_spec.rb

@@ -0,0 +1,5 @@
+require 'spec_helper'
+
+RSpec.describe 'test_module::titles' do
+  it { is_expected.to compile }
+end

spec/fixtures/test_module/lib/puppet/provider/title_provider/title_provider.rb

@@ -0,0 +1,30 @@
+require 'puppet/resource_api'
+require 'puppet/resource_api/simple_provider'
+
+# Implementation for the title_provider type using the Resource API.
+class Puppet::Provider::TitleProvider::TitleProvider < Puppet::ResourceApi::SimpleProvider
+  def get(_context)
+    [
+      {
+        name: 'foo',
+        ensure: :present,
+      },
+      {
+        name: 'bar',
+        ensure: :present,
+      },
+    ]
+  end
+
+  def create(context, name, should)
+    context.notice("Creating '#{name}' with #{should.inspect}")
+  end
+
+  def update(context, name, should)
+    context.notice("Updating '#{name}' with #{should.inspect}")
+  end
+
+  def delete(context, name)
+    context.notice("Deleting '#{name}'")
+  end
+end

spec/fixtures/test_module/lib/puppet/type/title_provider.rb

@@ -0,0 +1,21 @@
+require 'puppet/resource_api'
+
+Puppet::ResourceApi.register_type(
+  name: 'title_provider',
+  docs: <<-EOS,
+      This type provides Puppet with the capabilities to manage ...
+    EOS
+  features: [],
+  attributes:   {
+    ensure:      {
+      type:    'Enum[present, absent]',
+      desc:    'Whether this resource should be present or absent on the target system.',
+      default: 'present',
+    },
+    namevar:        {
+      type:      'String',
+      desc:      'The name of the resource you want to manage.',
+      behaviour: :namevar,
+    },
+  },
+)

spec/fixtures/test_module/manifests/titles.pp

@@ -0,0 +1,13 @@
+# test_module::titles
+#
+# A description of what this class does
+#
+# @summary A short summary of the purpose of this class
+#
+# @example
+#   include test_module::titles
+class test_module::titles {
+  title_provider { 'some title':
+    namevar => 'some id'
+  }
+}

spec/fixtures/test_module/spec/classes/titles_spec.rb

@@ -0,0 +1,11 @@
+require 'spec_helper'
+
+describe 'test_module::titles' do
+  on_supported_os.each do |os, os_facts|
+    context "on #{os}" do
+      let(:facts) { os_facts }
+
+      it { is_expected.to compile }
+    end
+  end
+end

spec/fixtures/test_module/spec/unit/puppet/provider/title_provider/title_provider_spec.rb

@@ -0,0 +1,50 @@
+require 'spec_helper'
+
+# TODO: needs some cleanup/helper to avoid this misery
+module Puppet::Provider::TitleProvider; end
+require 'puppet/provider/title_provider/title_provider'
+
+RSpec.describe Puppet::Provider::TitleProvider::TitleProvider do
+  subject(:provider) { described_class.new }
+
+  let(:context) { instance_double('Puppet::ResourceApi::BaseContext', 'context') }
+
+  describe '#get' do
+    it 'processes resources' do
+      expect(provider.get(context)).to eq [
+        {
+          name: 'foo',
+          ensure: :present,
+        },
+        {
+          name: 'bar',
+          ensure: :present,
+        },
+      ]
+    end
+  end
+
+  describe 'create(context, name, should)' do
+    it 'creates the resource' do
+      expect(context).to receive(:notice).with(%r{\ACreating 'a'})
+
+      provider.create(context, 'a', name: 'a', ensure: 'present')
+    end
+  end
+
+  describe 'update(context, name, should)' do
+    it 'updates the resource' do
+      expect(context).to receive(:notice).with(%r{\AUpdating 'foo'})
+
+      provider.update(context, 'foo', name: 'foo', ensure: 'present')
+    end
+  end
+
+  describe 'delete(context, name, should)' do
+    it 'deletes the resource' do
+      expect(context).to receive(:notice).with(%r{\ADeleting 'foo'})
+
+      provider.delete(context, 'foo')
+    end
+  end
+end

spec/puppet/resource_api/glue_spec.rb

@@ -6,10 +6,10 @@
 # funky happening with new puppet versions.
 RSpec.describe 'the dirty bits' do
   describe Puppet::ResourceApi::TypeShim do
-    subject(:instance) { described_class.new('title', { attr: 'value' }, 'typename') }
+    subject(:instance) { described_class.new('title', { attr: 'value' }, 'typename', :namevarname) }
 
     describe '.values' do
-      it { expect(instance.values).to eq(name: 'title', attr: 'value') }
+      it { expect(instance.values).to eq(namevarname: 'title', attr: 'value') }
     end
 
     describe '.typename' do
@@ -20,23 +20,32 @@
       it { expect(instance.name).to eq 'title' }
     end
 
+    describe '.namevar' do
+      it { expect(instance.namevar).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(name: 'title', attr: 'value') }
+        it { expect(instance.to_resource.values).to eq(namevarname: 'title', attr: 'value') }
       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 }
+      end
     end
   end
 
   describe Puppet::ResourceApi::ResourceShim do
-    subject(:instance) { described_class.new({ name: 'title', attr: 'value' }, 'typename') }
+    subject(:instance) { described_class.new({ namevarname: 'title', attr: 'value' }, 'typename', :namevarname) }
 
     describe '.values' do
-      it { expect(instance.values).to eq(name: 'title', attr: 'value') }
+      it { expect(instance.values).to eq(namevarname: 'title', attr: 'value') }
     end
 
     describe '.typename' do