From 0f2609a235a2b6665995aa22cac0d30ff572c805 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 12 Apr 2018 14:25:38 +0100 Subject: [PATCH] (PDK-919) Workaround PUP-2368 "using booleans result in unmanaged property" By unmunging booleans into `:true` and `:false` symbols, we trick puppet into not seeing the `false` value in the resource harness. This requires additional juggling to make sure we don't pass that value on to the provider. --- lib/puppet/resource_api.rb | 30 +++++++++-- spec/acceptance/boolean_spec.rb | 22 ++++++++ .../puppet/provider/test_bool/test_bool.rb | 34 +++++++++++++ .../test_module/lib/puppet/type/test_bool.rb | 30 +++++++++++ .../provider/test_bool/test_bool_spec.rb | 50 +++++++++++++++++++ .../spec/unit/puppet/type/test_bool_spec.rb | 8 +++ spec/puppet/resource_api_spec.rb | 18 ++++--- 7 files changed, 179 insertions(+), 13 deletions(-) create mode 100644 spec/acceptance/boolean_spec.rb create mode 100644 spec/fixtures/test_module/lib/puppet/provider/test_bool/test_bool.rb create mode 100644 spec/fixtures/test_module/lib/puppet/type/test_bool.rb create mode 100644 spec/fixtures/test_module/spec/unit/puppet/provider/test_bool/test_bool_spec.rb create mode 100644 spec/fixtures/test_module/spec/unit/puppet/type/test_bool_spec.rb diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index e1b2464f..c55fc757 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -147,7 +147,12 @@ def feature_support?(feature) type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type]) if param_or_property == :newproperty define_method(:should) do - @should ? @should.first : @should + if type.is_a? Puppet::Pops::Types::PBooleanType + # work around https://tickets.puppetlabs.com/browse/PUP-2368 + rs_value ? :true : :false # rubocop:disable Lint/BooleanSymbol + else + rs_value + end end define_method(:should=) do |value| @@ -157,7 +162,17 @@ def feature_support?(feature) # @see Puppet::Property.should=(value) @should = [Puppet::ResourceApi.mungify(type, value, "#{definition[:name]}.#{name}")] end + + # used internally + # @returns the final mungified value of this property + define_method(:rs_value) do + @should ? @should.first : @should + end else + define_method(:value) do + @value + end + define_method(:value=) do |value| if options[:behaviour] == :read_only raise Puppet::ResourceError, "Attempting to set `#{name}` read_only attribute value to `#{value}`" @@ -165,13 +180,19 @@ def feature_support?(feature) @value = Puppet::ResourceApi.mungify(type, value, "#{definition[:name]}.#{name}") end + + # used internally + # @returns the final mungified value of this parameter + define_method(:rs_value) do + @value + end end if type.instance_of? Puppet::Pops::Types::POptionalType type = type.type end - # puppet symbolizes some values through puppet/paramter/value.rb (see .convert()), but (especially) Enums + # puppet symbolizes some values through puppet/parameter/value.rb (see .convert()), but (especially) 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 } @@ -256,9 +277,8 @@ def call_provider(value); end define_method(:flush) do # puts 'flush' - target_state = Hash[@parameters.map { |k, v| [k, v.value] }] - # remove puppet's injected metaparams - target_state.delete(:loglevel) + # skip puppet's injected metaparams + target_state = Hash[@parameters.reject { |k, _v| [:loglevel, :noop].include? k }.map { |k, v| [k, v.rs_value] }] target_state = my_provider.canonicalize(context, [target_state]).first if feature_support?('canonicalize') retrieve unless @rapi_current_state diff --git a/spec/acceptance/boolean_spec.rb b/spec/acceptance/boolean_spec.rb new file mode 100644 index 00000000..30e5d249 --- /dev/null +++ b/spec/acceptance/boolean_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' +require 'tempfile' + +RSpec.describe 'a provider using booleans' do + let(:common_args) { '--verbose --trace --strict=error --modulepath spec/fixtures' } + + describe 'using `puppet apply`' do + it 'applies a catalog with no changes' do + stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_bool { foo: test_bool => true, test_bool_param => true; bar: test_bool => false, test_bool_param => false }\"") + expect(stdout_str).not_to match %r{Updating|ensure|test_bool}i + expect(stdout_str).not_to match %r{Error:} + end + it 'applies a catalog with bool changes' do + stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_bool { foo: test_bool => false, test_bool_param => false; bar: test_bool => true, test_bool_param => true }\"") + expect(stdout_str).to match %r{Test_bool\[foo\]/test_bool: test_bool changed (true|'true') to 'false'} + expect(stdout_str).to match %r{Test_bool\[bar\]/test_bool: test_bool changed (false|'false') to 'true'} + expect(stdout_str).to match %r{Updating: Updating 'foo' with \{:name=>"foo", :test_bool=>false, :test_bool_param=>false, :ensure=>"present"\}} + expect(stdout_str).to match %r{Updating: Updating 'bar' with \{:name=>"bar", :test_bool=>true, :test_bool_param=>true, :ensure=>"present"\}} + expect(stdout_str).not_to match %r{Error:} + end + end +end diff --git a/spec/fixtures/test_module/lib/puppet/provider/test_bool/test_bool.rb b/spec/fixtures/test_module/lib/puppet/provider/test_bool/test_bool.rb new file mode 100644 index 00000000..175db8be --- /dev/null +++ b/spec/fixtures/test_module/lib/puppet/provider/test_bool/test_bool.rb @@ -0,0 +1,34 @@ +require 'puppet/resource_api' +require 'puppet/resource_api/simple_provider' + +# Implementation for the test_bool type using the Resource API. +class Puppet::Provider::TestBool::TestBool < Puppet::ResourceApi::SimpleProvider + def get(_context) + [ + { + name: 'foo', + ensure: 'present', + test_bool: true, + test_bool_param: true, + }, + { + name: 'bar', + ensure: 'present', + test_bool: false, + test_bool_param: false, + }, + ] + 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 diff --git a/spec/fixtures/test_module/lib/puppet/type/test_bool.rb b/spec/fixtures/test_module/lib/puppet/type/test_bool.rb new file mode 100644 index 00000000..f141be89 --- /dev/null +++ b/spec/fixtures/test_module/lib/puppet/type/test_bool.rb @@ -0,0 +1,30 @@ +require 'puppet/resource_api' + +Puppet::ResourceApi.register_type( + name: 'test_bool', + 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', + }, + name: { + type: 'String', + desc: 'The name of the resource you want to manage.', + behaviour: :namevar, + }, + test_bool: { + type: 'Boolean', + desc: 'A boolean property for testing.', + }, + test_bool_param: { + type: 'Boolean', + desc: 'A boolean parameter for testing.', + behaviour: :parameter, + }, + }, +) diff --git a/spec/fixtures/test_module/spec/unit/puppet/provider/test_bool/test_bool_spec.rb b/spec/fixtures/test_module/spec/unit/puppet/provider/test_bool/test_bool_spec.rb new file mode 100644 index 00000000..0368229f --- /dev/null +++ b/spec/fixtures/test_module/spec/unit/puppet/provider/test_bool/test_bool_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' + +# TODO: needs some cleanup/helper to avoid this misery +module Puppet::Provider::TestBool; end +require 'puppet/provider/test_bool/test_bool' + +RSpec.describe Puppet::Provider::TestBool::TestBool 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 diff --git a/spec/fixtures/test_module/spec/unit/puppet/type/test_bool_spec.rb b/spec/fixtures/test_module/spec/unit/puppet/type/test_bool_spec.rb new file mode 100644 index 00000000..72393d45 --- /dev/null +++ b/spec/fixtures/test_module/spec/unit/puppet/type/test_bool_spec.rb @@ -0,0 +1,8 @@ +require 'spec_helper' +require 'puppet/type/test_bool' + +RSpec.describe 'the test_bool type' do + it 'loads' do + expect(Puppet::Type.type(:test_bool)).not_to be_nil + end +end diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb index 464de9e9..7a900ad5 100644 --- a/spec/puppet/resource_api_spec.rb +++ b/spec/puppet/resource_api_spec.rb @@ -236,41 +236,43 @@ def extract_values(function) } end + # rubocop:disable Lint/BooleanSymbol context 'when using :true' do - let(:the_boolean) { :true } # rubocop:disable Lint/BooleanSymbol + let(:the_boolean) { :true } - it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq true } + it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq :true } end context 'when using :false' do - let(:the_boolean) { :false } # rubocop:disable Lint/BooleanSymbol + let(:the_boolean) { :false } - it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq false } + it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq :false } end context 'when using "true"' do let(:the_boolean) { 'true' } - it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq true } + it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq :true } end context 'when using "false"' do let(:the_boolean) { 'false' } - it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq false } + it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq :false } end context 'when using true' do let(:the_boolean) { true } - it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq true } + it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq :true } end context 'when using false' do let(:the_boolean) { false } - it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq false } + it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq :false } end context 'when using an unparsable value' do let(:the_boolean) { 'flubb' } it('the test_boolean value is set correctly') { expect { instance }.to raise_error Puppet::ResourceError, %r{test_boolean expect.* Boolean .* got String} } end + # rubocop:enable Lint/BooleanSymbol end end end