(PDK-911) Fix handling of ensure values from symbols to strings

Tripping over puppet's behaviour for handling newvalue values, rsapi
would accidentally pass symbols for `Enum[absent, present]` ensures.

The unit and integration tests did not agree on whether `absent` and
`present` should be strings or symbols. Puppet itself is calling for
a string as a value for `Enum`s, and in the implementation those two
were only symbols because of a special case in the setup of the two.

This commit fixes this behaviour to always use string. This makes it
consistent with the rest of the API, and reduces the amount of noise
developers have to remember when working with values passed from us.

README.md

@@ -118,11 +118,11 @@ class Puppet::Provider::Foo::Foo < Puppet::ResourceApi::SimpleProvider
     [
       {
         name: 'foo',
-        ensure: :present,
+        ensure: 'present',
       },
       {
         name: 'bar',
-        ensure: :present,
+        ensure: 'present',
       },
     ]
   end

lib/puppet/resource_api.rb

@@ -85,7 +85,7 @@ def feature_support?(feature)
 
           # skip properties if the resource is being deleted
           next if definition[:attributes][:ensure] &&
-                  value(:ensure) == :absent &&
+                  value(:ensure) == 'absent' &&
                   options[:behaviour].nil?
 
           if value(name).nil? && !(type.instance_of? Puppet::Pops::Types::POptionalType)
@@ -211,6 +211,10 @@ def feature_support?(feature)
           case options[:type]
           when 'Enum[present, absent]'
             Puppet::ResourceApi.def_newvalues(self, param_or_property, :absent, :present)
+            # puppet symbolizes these values through puppet/paramter/value.rb (see .convert()), but Enums are strings
+            # specifying a munge block here skips the value_collection fallback in puppet/parameter.rb's
+            # default .unsafe_munge() implementation
+            munge { |v| v }
           end
         end
       end
@@ -252,7 +256,7 @@ def feature_support?(feature)
           end
         else
           result[namevar_name] = title
-          result[:ensure] = :absent
+          result[:ensure] = 'absent'
         end
 
         @rapi_current_state = current_state
@@ -275,7 +279,7 @@ def feature_support?(feature)
         Puppet.debug("Target State: #{target_state.inspect}")
 
         # enforce init_only attributes
-        if Puppet.settings[:strict] != :off && @rapi_current_state && (@rapi_current_state[:ensure] == :present && target_state[:ensure] == :present)
+        if Puppet.settings[:strict] != :off && @rapi_current_state && (@rapi_current_state[:ensure] == 'present' && target_state[:ensure] == 'present')
           target_state.each do |name, value|
             next unless definition[:attributes][name][:behaviour] == :init_only && value != @rapi_current_state[name]
             message = "Attempting to change `#{name}` init_only attribute value from `#{@rapi_current_state[name]}` to `#{value}`"

spec/acceptance/device_spec.rb

@@ -20,7 +20,7 @@
     end
     it 'manages resources on the target system' do
       stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider foo ensure=present #{default_type_values}")
-      expect(stdout_str).to match %r{Notice: /Device_provider\[foo\]/ensure: defined 'ensure' as 'present'}
+      expect(stdout_str).to match %r{Notice: /Device_provider\[foo\]/ensure: ensure changed 'absent' to 'present'}
       expect(status).to eq 0
     end
 
@@ -30,8 +30,8 @@
       it 'deals with canonicalized resources correctly' do
         stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present #{default_type_values}")
         stdmatch = 'Error: /Device_provider\[wibble\]: Could not evaluate: device_provider\[wibble\]#get has not provided canonicalized values.\n'\
-                   'Returned values:       \{:name=>"wibble", :ensure=>:present, :string=>"sample", :string_ro=>"fixed"\}\n'\
-                   'Canonicalized values:  \{:name=>"wibble", :ensure=>:present, :string=>"changed", :string_ro=>"fixed"\}'
+                   'Returned values:       \{:name=>"wibble", :ensure=>"present", :string=>"sample", :string_ro=>"fixed"\}\n'\
+                   'Canonicalized values:  \{:name=>"wibble", :ensure=>"present", :string=>"changed", :string_ro=>"fixed"\}'
         expect(stdout_str).to match %r{#{stdmatch}}
         expect(status.success?).to be_falsey # rubocop:disable RSpec/PredicateMatcher
       end
@@ -43,8 +43,8 @@
       it 'deals with canonicalized resources correctly' do
         stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present  #{default_type_values}")
         stdmatch = 'Warning: device_provider\[wibble\]#get has not provided canonicalized values.\n'\
-                   'Returned values:       \{:name=>"wibble", :ensure=>:present, :string=>"sample", :string_ro=>"fixed"\}\n'\
-                   'Canonicalized values:  \{:name=>"wibble", :ensure=>:present, :string=>"changed", :string_ro=>"fixed"\}'
+                   'Returned values:       \{:name=>"wibble", :ensure=>"present", :string=>"sample", :string_ro=>"fixed"\}\n'\
+                   'Canonicalized values:  \{:name=>"wibble", :ensure=>"present", :string=>"changed", :string_ro=>"fixed"\}'
         expect(stdout_str).to match %r{#{stdmatch}}
         expect(status.success?).to be_truthy # rubocop:disable RSpec/PredicateMatcher
       end

spec/acceptance/validation_spec.rb

@@ -36,7 +36,7 @@
     it 'allows removing' do
       output, status = Open3.capture2e("puppet resource #{common_args} test_validation foo ensure=absent param=2")
       expect(output.strip).to match %r{^test_validation}
-      expect(output.strip).to match %r{Test_validation\[foo\]/ensure: undefined 'ensure' from 'present'}
+      expect(output.strip).to match %r{Test_validation\[foo\]/ensure: ensure changed 'present' to 'absent'}
       expect(status.exitstatus).to eq 0
     end
 
@@ -80,7 +80,7 @@
 
     it 'allows creating' do
       output, status = Open3.capture2e("puppet apply #{common_args} -e \"test_validation{ new: prop => 2, param => 3 }\"")
-      expect(output.strip).to match %r{Test_validation\[new\]/ensure: defined 'ensure' as 'present'}
+      expect(output.strip).to match %r{Test_validation\[new\]/ensure: ensure changed 'absent' to 'present'}
       expect(status.exitstatus).to eq 0
     end
 
@@ -92,7 +92,7 @@
 
     it 'allows removing' do
       output, status = Open3.capture2e("puppet apply #{common_args} -e \"test_validation{ foo: ensure => absent, param => 3 }\"")
-      expect(output.strip).to match %r{Test_validation\[foo\]/ensure: undefined 'ensure' from 'present'}
+      expect(output.strip).to match %r{Test_validation\[foo\]/ensure: ensure changed 'present' to 'absent'}
       expect(status.exitstatus).to eq 0
     end
 

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

@@ -3,7 +3,7 @@
 # A example/test provider for Device support
 class Puppet::Provider::DeviceProvider::DeviceProvider
   def get(_context)
-    [{:name => 'wibble', :ensure => :present, :string => 'sample', :string_ro => 'fixed' }]
+    [{:name => 'wibble', :ensure => 'present', :string => 'sample', :string_ro => 'fixed' }]
   end
 
   def set(context, changes); end

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

@@ -7,11 +7,11 @@ def get(_context)
     [
       {
         name: 'foo',
-        ensure: :present,
+        ensure: 'present',
       },
       {
         name: 'bar',
-        ensure: :present,
+        ensure: 'present',
       },
     ]
   end

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

@@ -7,11 +7,11 @@ def get(_context)
     [
       {
         name: 'foo',
-        ensure: :present,
+        ensure: 'present',
       },
       {
         name: 'bar',
-        ensure: :present,
+        ensure: 'present',
       },
     ]
   end

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

@@ -8,28 +8,28 @@ def get(_context, names = nil)
       [
           {
               name: 'bar',
-              ensure: :present,
+              ensure: 'present',
               test_string: 'default'
           },
           {
               name: 'foo',
-              ensure: :present,
+              ensure: 'present',
               test_string: 'default'
           },
       ]
     elsif names.include?('foo')
       [
           {
               name: 'foo',
-              ensure: :absent,
+              ensure: 'absent',
               test_string: 'foo found'
           }
       ]
     else
       [
           {
               name: 'foo',
-              ensure: :present,
+              ensure: 'present',
               test_string: 'not foo'
           }
       ]

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

@@ -7,13 +7,13 @@ def get(_context)
     [
       {
         name: 'foo',
-        ensure: :present,
+        ensure: 'present',
         prop: 2,
         prop_ro: 8,
       },
       {
         name: 'bar',
-        ensure: :present,
+        ensure: 'present',
         prop: 3,
         prop_ro: 9,
       },

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

@@ -7,11 +7,11 @@ def get(_context)
     [
       {
         namevar: 'foo',
-        ensure: :present,
+        ensure: 'present',
       },
       {
         namevar: 'bar',
-        ensure: :present,
+        ensure: 'present',
       },
     ]
   end

spec/fixtures/test_module/spec/unit/puppet/provider/test_autorequire/test_autorequire_spec.rb

@@ -14,11 +14,11 @@ module Puppet::Provider::TestAutorequire; end
       expect(provider.get(context)).to eq [
         {
           name: 'foo',
-          ensure: :present,
+          ensure: 'present',
         },
         {
           name: 'bar',
-          ensure: :present,
+          ensure: 'present',
         },
       ]
     end

spec/fixtures/test_module/spec/unit/puppet/provider/test_noop_support/test_noop_support_spec.rb

@@ -14,11 +14,11 @@ module Puppet::Provider::TestNoopSupport; end
       expect(provider.get(context)).to eq [
         {
           name: 'foo',
-          ensure: :present,
+          ensure: 'present',
         },
         {
           name: 'bar',
-          ensure: :present,
+          ensure: 'present',
         },
       ]
     end

spec/fixtures/test_module/spec/unit/puppet/provider/test_validation/test_validation_spec.rb

@@ -14,11 +14,11 @@ module Puppet::Provider::TestValidation; end
       expect(provider.get(context)).to eq [
         {
           name: 'foo',
-          ensure: :present,
+          ensure: 'present',
         },
         {
           name: 'bar',
-          ensure: :present,
+          ensure: 'present',
         },
       ]
     end

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

@@ -14,11 +14,11 @@ module Puppet::Provider::TitleProvider; end
       expect(provider.get(context)).to eq [
         {
           name: 'foo',
-          ensure: :present,
+          ensure: 'present',
         },
         {
           name: 'bar',
-          ensure: :present,
+          ensure: 'present',
         },
       ]
     end

spec/puppet/resource_api/base_context_spec.rb

@@ -236,8 +236,8 @@ def send_log(log, msg)
 
   describe '#processed(title, is, should)' do
     it 'logs the successful change of attributes' do
-      expect(context).to receive(:send_log).with(:notice, %r{Processed Thing\[one\] from {:ensure=>:absent} to {:ensure=>:present, :name=>\"thing one\"}})
-      context.processed('Thing[one]', { ensure: :absent }, { ensure: :present, name: 'thing one' })
+      expect(context).to receive(:send_log).with(:notice, %r{Processed Thing\[one\] from {:ensure=>"absent"} to {:ensure=>"present", :name=>"thing one"}})
+      context.processed('Thing[one]', { ensure: 'absent' }, { ensure: 'present', name: 'thing one' })
     end
 
     it 'raises if multiple titles are passed' do

spec/puppet/resource_api_spec.rb

@@ -162,7 +162,7 @@ def extract_values(function)
       subject(:instance) { Puppet::Type.type(:with_string).new(params) }
 
       let(:params) do
-        { title: 'test', test_boolean: true, test_integer: 15, test_float: 1.23, test_ensure: :present,
+        { title: 'test', test_boolean: true, test_integer: 15, test_float: 1.23, test_ensure: 'present',
           test_enum: 'a', test_variant_pattern: 0xAEF123FF, test_url: 'http://example.com' }
       end
 
@@ -186,7 +186,7 @@ def extract_values(function)
         it('the test_string value is set correctly') { expect(instance[:test_string]).to eq 'somevalue' }
         it('the test_integer value is set correctly') { expect(instance[:test_integer]).to eq(-1) }
         it('the test_float value is set correctly') { expect(instance[:test_float]).to eq(-1.5) }
-        it('the test_ensure value is set correctly') { expect(instance[:test_ensure]).to eq(:present) }
+        it('the test_ensure value is set correctly') { expect(instance[:test_ensure]).to eq('present') }
         it('the test_enum value is set correctly') { expect(instance[:test_enum]).to eq('a') }
         it('the test_variant_pattern value is set correctly') { expect(instance[:test_variant_pattern]).to eq('a' * 8) }
         it('the test_url value is set correctly') { expect(instance[:test_url]).to eq('hkp://example.com') }
@@ -206,7 +206,7 @@ def extract_values(function)
             test_boolean: the_boolean,
             test_integer: '-1',
             test_float: '-1.5',
-            test_ensure: :present,
+            test_ensure: 'present',
             test_enum: 'a',
             test_variant_pattern: 'a' * 8,
             test_url: 'http://example.com',
@@ -320,7 +320,7 @@ def extract_values(function)
       subject(:instance) { Puppet::Type.type(:with_parameters).new(params) }
 
       let(:params) do
-        { title: 'test', test_boolean: true, test_integer: 15, test_float: 1.23, test_ensure: :present,
+        { title: 'test', test_boolean: true, test_integer: 15, test_float: 1.23, test_ensure: 'present',
           test_variant_pattern: 0xAEF123FF, test_url: 'http://example.com' }
       end
 
@@ -343,7 +343,7 @@ def extract_values(function)
         it('the test_string value is set correctly') { expect(instance[:test_string]).to eq 'somevalue' }
         it('the test_integer value is set correctly') { expect(instance[:test_integer]).to eq(-1) }
         it('the test_float value is set correctly') { expect(instance[:test_float]).to eq(-1.5) }
-        it('the test_ensure value is set correctly') { expect(instance[:test_ensure]).to eq(:present) }
+        it('the test_ensure value is set correctly') { expect(instance[:test_ensure]).to eq('present') }
         it('the test_variant_pattern value is set correctly') { expect(instance[:test_variant_pattern]).to eq('a' * 8) }
         it('the test_url value is set correctly') { expect(instance[:test_url]).to eq('hkp://example.com') }
       end
@@ -432,7 +432,7 @@ def extract_values(function)
       let(:provider_class) do
         Class.new do
           def get(_context)
-            [{ name: 'init', ensure: :present, immutable: 'physics', mutable: 'bank balance' }]
+            [{ name: 'init', ensure: 'present', immutable: 'physics', mutable: 'bank balance' }]
           end
 
           def set(_context, _changes); end
@@ -445,7 +445,7 @@ def set(_context, _changes); end
       end
 
       context 'when a manifest wants to set the value of an init_only attribute' do
-        let(:instance) { Puppet::Type.type(:init_behaviour).new(name: 'new_init', ensure: :present, immutable: 'new', mutable: 'flexible') }
+        let(:instance) { Puppet::Type.type(:init_behaviour).new(name: 'new_init', ensure: 'present', immutable: 'new', mutable: 'flexible') }
 
         context 'when Puppet strict setting is :error' do
           let(:strict_level) { :error }
@@ -479,7 +479,7 @@ def set(_context, _changes); end
       end
 
       context 'when a manifest wants to change the value of an init_only attribute' do
-        let(:instance) { Puppet::Type.type(:init_behaviour).new(name: 'init', ensure: :present, immutable: 'lies', mutable: 'overdraft') }
+        let(:instance) { Puppet::Type.type(:init_behaviour).new(name: 'init', ensure: 'present', immutable: 'lies', mutable: 'overdraft') }
 
         context 'when Puppet strict setting is :error' do
           let(:strict_level) { :error }
@@ -542,7 +542,7 @@ def set(_context, _changes); end
       let(:provider_class) do
         Class.new do
           def get(_context)
-            [{ name: 'foo_ro', ensure: :present, immutable: 'physics' }]
+            [{ name: 'foo_ro', ensure: 'present', immutable: 'physics' }]
           end
 
           def set(_context, _changes); end
@@ -555,13 +555,13 @@ def set(_context, _changes); end
       end
 
       context 'when a manifest wants to set the value of a read_only attribute' do
-        let(:instance) { Puppet::Type.type(:read_only_behaviour).new(name: 'new_ro', ensure: :present, immutable: 'new') }
+        let(:instance) { Puppet::Type.type(:read_only_behaviour).new(name: 'new_ro', ensure: 'present', immutable: 'new') }
 
         it { expect { instance.flush }.to raise_error Puppet::ResourceError, %r{Attempting to set `immutable` read_only attribute value to} }
       end
 
       context 'when a manifest wants to change the value of a read_only attribute' do
-        let(:instance) { Puppet::Type.type(:read_only_behaviour).new(name: 'foo_ro', ensure: :present, immutable: 'change') }
+        let(:instance) { Puppet::Type.type(:read_only_behaviour).new(name: 'foo_ro', ensure: 'present', immutable: 'change') }
 
         it { expect { instance.flush }.to raise_error Puppet::ResourceError, %r{Attempting to set `immutable` read_only attribute value to} }
       end
@@ -928,7 +928,7 @@ def set(_context, changes)
             expect(resource[:test_string]).to be_nil
             expect(log_sink.last.message).to eq('Current State: nil')
           end
-          it('is set to absent') { expect(resource[:ensure]).to eq :absent }
+          it('is set to absent') { expect(resource[:ensure]).to eq 'absent' }
         end
       end
     end
@@ -1060,7 +1060,7 @@ def set(_context, changes)
             expect(resource[:test_string]).to be_nil
             expect(log_sink.last.message).to eq('Current State: nil')
           end
-          it('is set to absent') { expect(resource[:ensure]).to eq :absent }
+          it('is set to absent') { expect(resource[:ensure]).to eq 'absent' }
         end
       end
     end