From 66e2d3766cfce784e249111dbcdde474fa2a2f7f Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Wed, 2 May 2018 17:16:21 +0100 Subject: [PATCH 1/2] (PDK-946) Passes ensure values to puppet as symbols. Providers still deal with ensure values as strings, but puppet is always passed symbols. --- lib/puppet/resource_api.rb | 36 ++++- spec/acceptance/device_spec.rb | 2 +- spec/acceptance/validation_spec.rb | 12 +- spec/puppet/resource_api_spec.rb | 249 ++++++++++++++++------------- 4 files changed, 179 insertions(+), 120 deletions(-) diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index ce2ca94a..1d24145b 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -10,6 +10,7 @@ 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 + validate_ensure(definition) definition[:features] ||= [] supported_features = %w[supports_noop canonicalize remote_resource simple_get_filter].freeze @@ -82,6 +83,7 @@ def type_definition @missing_params = [] definition[:attributes].each do |name, options| type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type]) + # skip read only vars and the namevar next if [:read_only, :namevar].include? options[:behaviour] @@ -145,12 +147,20 @@ def type_definition end end + if name == :ensure + def insync?(is) + rs_value.to_s == is.to_s + end + end + type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type]) if param_or_property == :newproperty define_method(:should) do 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 + elsif name == :ensure && rs_value.is_a?(String) + rs_value.to_sym else rs_value end @@ -205,12 +215,11 @@ def type_definition Puppet::ResourceApi.def_newvalues(self, param_or_property, %r{}) when Puppet::Pops::Types::PBooleanType Puppet::ResourceApi.def_newvalues(self, param_or_property, 'true', 'false') - # rubocop:disable Lint/BooleanSymbol aliasvalue true, 'true' aliasvalue false, 'false' - aliasvalue :true, 'true' - aliasvalue :false, 'false' - # rubocop:enable Lint/BooleanSymbol + aliasvalue :true, 'true' # rubocop:disable Lint/BooleanSymbol + aliasvalue :false, 'false' # rubocop:disable Lint/BooleanSymbol + when Puppet::Pops::Types::PIntegerType Puppet::ResourceApi.def_newvalues(self, param_or_property, %r{^-?\d+$}) when Puppet::Pops::Types::PFloatType, Puppet::Pops::Types::PNumericType @@ -266,9 +275,12 @@ def call_provider(value); end end else result[namevar_name] = title - result[:ensure] = 'absent' if definition[:attributes].key?(:ensure) + result[:ensure] = :absent if type_definition.ensurable? end + # puppet needs ensure to be a symbol + result[:ensure] = result[:ensure].to_sym if type_definition.ensurable? && result[:ensure].is_a?(String) + raise_missing_attrs @rapi_current_state = current_state @@ -286,7 +298,6 @@ def call_provider(value); end retrieve unless @rapi_current_state - # require 'pry'; binding.pry return if @rapi_current_state == target_state Puppet.debug("Target State: #{target_state.inspect}") @@ -315,7 +326,7 @@ def call_provider(value); end define_method(:raise_missing_attrs) do error_msg = "The following mandatory attributes were not provided:\n * " + @missing_attrs.join(", \n * ") - raise Puppet::ResourceError, error_msg if @missing_attrs.any? && (value(:ensure) != 'absent' && !value(:ensure).nil?) + raise Puppet::ResourceError, error_msg if @missing_attrs.any? && (value(:ensure) != :absent && !value(:ensure).nil?) end define_method(:raise_missing_params) do @@ -489,6 +500,15 @@ def self.try_mungify(type, value, error_msg_prefix) # an error :-( inferred_type = Puppet::Pops::Types::TypeCalculator.infer_set(value) error_msg = Puppet::Pops::Types::TypeMismatchDescriber.new.describe_mismatch(error_msg_prefix, type, inferred_type) - return [nil, error_msg] # the entire function is using returns for clarity # rubocop:disable Style/RedundantReturn + [nil, error_msg] + end + + def self.validate_ensure(definition) + return unless definition[:attributes].key? :ensure + options = definition[:attributes][:ensure] + type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type]) + + return if type.is_a?(Puppet::Pops::Types::PEnumType) && type.values.sort == %w[absent present].sort + raise Puppet::DevError, '`:ensure` attribute must have a type of: `Enum[present, absent]`' end end diff --git a/spec/acceptance/device_spec.rb b/spec/acceptance/device_spec.rb index c19a63c7..9271c1f7 100644 --- a/spec/acceptance/device_spec.rb +++ b/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: ensure changed 'absent' to 'present'} + expect(stdout_str).to match %r{Notice: /Device_provider\[foo\]/ensure: defined 'ensure' as 'present'} expect(status).to eq 0 end diff --git a/spec/acceptance/validation_spec.rb b/spec/acceptance/validation_spec.rb index c3cf2981..28784bf6 100644 --- a/spec/acceptance/validation_spec.rb +++ b/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: ensure changed 'present' to 'absent'} + expect(output.strip).to match %r{Test_validation\[foo\]/ensure: undefined 'ensure' from 'present'} expect(status.exitstatus).to eq 0 end @@ -52,6 +52,12 @@ expect(status.exitstatus).to eq 1 end + it 'does not change attribute values on delete' do + output, status = Open3.capture2e("puppet resource #{common_args} test_validation foo ensure=absent param=2 prop=4") + expect(output.strip).not_to match %r{prop changed} + expect(status.exitstatus).to eq 0 + end + context 'when passing a value to a read_only property' do context 'with an existing resource' do it 'throws' do @@ -80,7 +86,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: ensure changed 'absent' to 'present'} + expect(output.strip).to match %r{Test_validation\[new\]/ensure: defined 'ensure' as 'present'} expect(status.exitstatus).to eq 0 end @@ -92,7 +98,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: ensure changed 'present' to 'absent'} + expect(output.strip).to match %r{Test_validation\[foo\]/ensure: undefined 'ensure' from 'present'} expect(status.exitstatus).to eq 0 end diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb index ca226636..00a36ed3 100644 --- a/spec/puppet/resource_api_spec.rb +++ b/spec/puppet/resource_api_spec.rb @@ -73,10 +73,6 @@ type: 'Float', desc: 'a floating point value', }, - test_ensure: { - type: 'Enum[present, absent]', - desc: 'a ensure value', - }, test_enum: { type: 'Enum[a, b, c]', desc: 'an enumeration', @@ -162,7 +158,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_enum: 'a', test_variant_pattern: '0xAEF123FF', test_url: 'http://example.com' } end @@ -176,7 +172,6 @@ def extract_values(function) test_boolean: true, test_integer: -1, test_float: -1.5, - test_ensure: 'present', test_enum: 'a', test_variant_pattern: 'a' * 8, test_url: 'hkp://example.com', @@ -186,7 +181,6 @@ 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_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 @@ -199,7 +193,6 @@ def extract_values(function) test_boolean: 'true', test_integer: '-1', test_float: '-1.5', - test_ensure: 'present', test_enum: 'a', test_variant_pattern: 'a' * 8, test_url: 'hkp://example.com', @@ -209,7 +202,6 @@ 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_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') } @@ -223,7 +215,6 @@ def extract_values(function) test_boolean: the_boolean, test_integer: '-1', test_float: '-1.5', - test_ensure: 'present', test_enum: 'a', test_variant_pattern: 'a' * 8, test_url: 'http://example.com', @@ -303,118 +294,167 @@ def set(_context, _changes); end end context 'when registering a type that is ensurable', agent_test: true do - let(:definition) do - { - name: 'with_ensure', - attributes: { - name: { - type: 'String', - behaviour: :namevar, - desc: 'the title', - }, - ensure: { - type: 'Enum[present, absent]', - desc: 'a ensure value', - }, - test_string: { - type: 'String', - desc: 'the description', - default: 'default value', - }, - test_boolean: { - type: 'Boolean', - desc: 'a boolean value', - }, - test_integer: { - type: 'Integer', - desc: 'an integer value', - }, - test_float: { - type: 'Float', - desc: 'a floating point value', - }, - test_enum: { - type: 'Enum[a, b, c]', - desc: 'an enumeration', - }, - test_variant_pattern: { - type: 'Variant[Pattern[/\A(0x)?[0-9a-fA-F]{8}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{16}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{40}\Z/]]', - desc: 'a pattern value', - }, - test_url: { - type: 'Pattern[/\A((hkp|http|https):\/\/)?([a-z\d])([a-z\d-]{0,61}\.)+[a-z\d]+(:\d{2,5})?$/]', - desc: 'a hkp or http(s) url', - }, - test_optional_string: { - type: 'Optional[String]', - desc: 'a optional string value', + context 'when ensurable is correctly declared' do + let(:definition) do + { + name: 'with_ensure', + attributes: { + name: { + type: 'String', + behaviour: :namevar, + desc: 'the title', + }, + ensure: { + type: 'Enum[present, absent]', + desc: 'a ensure value', + }, + test_string: { + type: 'String', + desc: 'the description', + default: 'default value', + }, + test_boolean: { + type: 'Boolean', + desc: 'a boolean value', + }, + test_integer: { + type: 'Integer', + desc: 'an integer value', + }, + test_float: { + type: 'Float', + desc: 'a floating point value', + }, + test_enum: { + type: 'Enum[a, b, c]', + desc: 'an enumeration', + }, + test_variant_pattern: { + type: 'Variant[Pattern[/\A(0x)?[0-9a-fA-F]{8}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{16}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{40}\Z/]]', + desc: 'a pattern value', + }, + test_url: { + type: 'Pattern[/\A((hkp|http|https):\/\/)?([a-z\d])([a-z\d-]{0,61}\.)+[a-z\d]+(:\d{2,5})?$/]', + desc: 'a hkp or http(s) url', + }, + test_optional_string: { + type: 'Optional[String]', + desc: 'a optional string value', + }, }, - }, - } - end + } + end - let(:provider_class) do - Class.new do - def get(_context) - [] - end + let(:provider_class) do + Class.new do + def get(_context) + [] + end - def set(_context, _changes); end + def set(_context, _changes); end + end end - end - it { expect { described_class.register_type(definition) }.not_to raise_error } + it { expect { described_class.register_type(definition) }.not_to raise_error } - describe 'the registered type' do - subject(:type) { Puppet::Type.type(:with_ensure) } + describe 'the registered type' do + subject(:type) { Puppet::Type.type(:with_ensure) } - it { is_expected.not_to be_nil } - end + it { is_expected.not_to be_nil } + end - before(:each) do - stub_const('Puppet::Provider::WithEnsure', Module.new) - stub_const('Puppet::Provider::WithEnsure::WithEnsure', provider_class) - end + before(:each) do + stub_const('Puppet::Provider::WithEnsure', Module.new) + stub_const('Puppet::Provider::WithEnsure::WithEnsure', provider_class) + end - describe 'an instance of this type' do - subject(:instance) { Puppet::Type.type(:with_ensure).new(params) } + describe 'an instance of this type' do + subject(:instance) { Puppet::Type.type(:with_ensure).new(params) } - context 'when mandatory attributes are missing, but ensure is present' do - let(:params) do - { - title: 'test', - ensure: 'present', - } + context 'when mandatory attributes are missing, but ensure is present' do + let(:params) do + { + title: 'test', + ensure: 'present', + } + end + + it { + expect { + instance.validate + instance.retrieve + }.to raise_exception Puppet::ResourceError, %r{The following mandatory attributes were not provided} } end - it { - expect { + describe 'an absent instance' do + subject(:retrieved_info) do instance.validate instance.retrieve - }.to raise_exception Puppet::ResourceError, %r{The following mandatory attributes were not provided} } - end + end - describe 'an absent instance' do - subject(:retrieved_info) do - instance.validate - instance.retrieve - end + let(:params) do + { + title: 'does_not_exist', + } + end - let(:params) do - { - title: 'does_not_exist', + it('its name is set correctly') { expect(retrieved_info[:name]).to eq 'does_not_exist' } + it('its properties are set correctly') { + expect(retrieved_info[:test_string]).to be_nil } + it { expect(retrieved_info[:ensure]).to eq(:absent) } + + it { expect { retrieved_info }.not_to raise_exception } end - it('its name is set correctly') { expect(retrieved_info[:name]).to eq 'does_not_exist' } - it('its properties are set correctly') { - expect(retrieved_info[:test_string]).to be_nil - } - it { expect(retrieved_info[:ensure]).to eq('absent') } + context 'when setting values for the attributes' do + let(:params) do + { + title: 'test', + ensure: ensure_value, + test_string: 'somevalue', + test_boolean: true, + test_integer: -1, + test_float: -1.5, + test_enum: 'a', + test_variant_pattern: 'a' * 8, + test_url: 'hkp://example.com', + } + end - it { expect { retrieved_info }.not_to raise_exception } + %w[absent present].each do |value| + context "with ensure=#{value}" do + let(:ensure_value) { value } + + it('the ensure value is presented as a symbol') { expect(instance[:ensure]).to eq ensure_value.to_sym } + it('the ensure rs_value is a string') { expect(instance.parameters[:ensure].rs_value).to eq ensure_value } + + it { expect(instance.parameters[:ensure]).to be_insync(value) } + end + end + end end end + context 'when ensurable is not correctly declared' do + let(:definition) do + { + name: 'with_bad_ensure', + attributes: { + name: { + type: 'String', + behaviour: :namevar, + desc: 'the title', + }, + ensure: { + type: 'Enum[yes, no]', + desc: 'a bad ensure attribute', + }, + }, + } + end + + it { expect { described_class.register_type(definition) }.to raise_error Puppet::DevError, %r{`:ensure` attribute must have a type of: `Enum\[present, absent\]`} } + end end context 'when registering a type with multiple parameters' do @@ -448,11 +488,6 @@ def set(_context, _changes); end desc: 'a floating point parameter', behaviour: :parameter, }, - test_ensure: { - type: 'Enum[present, absent]', - desc: 'a ensure parameter', - behaviour: :parameter, - }, test_variant_pattern: { type: 'Variant[Pattern[/\A(0x)?[0-9a-fA-F]{8}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{16}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{40}\Z/]]', desc: 'a pattern parameter', @@ -485,7 +520,7 @@ def set(_context, _changes); end 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_variant_pattern: '0xAEF123FF', test_url: 'http://example.com' } end @@ -499,7 +534,6 @@ def set(_context, _changes); end test_boolean: 'true', test_integer: '-1', test_float: '-1.5', - test_ensure: 'present', test_variant_pattern: 'a' * 8, test_url: 'hkp://example.com', } @@ -508,7 +542,6 @@ def set(_context, _changes); end 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_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 From f66dcc78158853cdccaaba63b6997ab36025b5f3 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 3 May 2018 18:13:59 +0100 Subject: [PATCH 2/2] (maint) avoid loading mocha, and the deprecation warning about it --- spec/spec_helper.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 18da6220..07171f75 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -31,7 +31,6 @@ require 'bundler/setup' require 'puppet/resource_api' -require 'puppetlabs_spec_helper/module_spec_helper' RSpec.configure do |config| # Enable flags like --only-failures and --next-failure @@ -47,3 +46,5 @@ # override legacy default from puppetlabs_spec_helper config.mock_with :rspec end + +require 'puppetlabs_spec_helper/module_spec_helper'