From 4255f5ccbb009d58bc81731d673122f899cbc8c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Thu, 27 Apr 2023 12:23:20 -1000 Subject: [PATCH 1/2] Rework the tests to check actual values Rather that testing that the function return the same or different values, test the actual returned value so that we are sure the behavior does not change unexpectedly. --- spec/functions/fqdn_rand_string_spec.rb | 60 +++++++++---------------- 1 file changed, 21 insertions(+), 39 deletions(-) diff --git a/spec/functions/fqdn_rand_string_spec.rb b/spec/functions/fqdn_rand_string_spec.rb index 9eed4ad04..6ff30a980 100644 --- a/spec/functions/fqdn_rand_string_spec.rb +++ b/spec/functions/fqdn_rand_string_spec.rb @@ -25,49 +25,31 @@ it { is_expected.to run.with_params(100, 'ab').and_return(%r{\A[ab]{100}\z}) } it { is_expected.to run.with_params(100, 'ãβ').and_return(%r{\A[ãβ]{100}\z}) } - it "provides the same 'random' value on subsequent calls for the same host" do - expect(fqdn_rand_string(10)).to eql(fqdn_rand_string(10)) - end - - it 'considers the same host and same extra arguments to have the same random sequence' do - first_random = fqdn_rand_string(10, extra_identifier: [1, 'same', 'host']) - second_random = fqdn_rand_string(10, extra_identifier: [1, 'same', 'host']) - - expect(first_random).to eql(second_random) - end - - it 'allows extra arguments to control the random value on a single host' do - first_random = fqdn_rand_string(10, extra_identifier: [1, 'different', 'host']) - second_different_random = fqdn_rand_string(10, extra_identifier: [2, 'different', 'host']) - - expect(first_random).not_to eql(second_different_random) - end - - it 'returns different strings for different hosts' do - val1 = fqdn_rand_string(10, host: 'first.host.com') - val2 = fqdn_rand_string(10, host: 'second.host.com') - - expect(val1).not_to eql(val2) - end + context 'produce predictible and reproducible results' do + before(:each) do + if Gem::Version.new(Puppet::PUPPETVERSION) < Gem::Version.new('7.23.0') + allow(scope).to receive(:lookupvar).with('::fqdn', {}).and_return(fqdn) + else + allow(scope).to receive(:lookupvar).with('facts', {}).and_return({ 'networking' => { 'fqdn' => fqdn } }) + end + end - def fqdn_rand_string(max, args = {}) - host = args[:host] || '127.0.0.1' - charset = args[:charset] - extra = args[:extra_identifier] || [] + context 'on a node named example.com' do + let(:fqdn) { 'example.com' } - # workaround not being able to use let(:facts) because some tests need - # multiple different hostnames in one context - if Gem::Version.new(Puppet::PUPPETVERSION) < Gem::Version.new('7.23.0') - allow(scope).to receive(:lookupvar).with('::fqdn', {}).and_return(host) - else - allow(scope).to receive(:lookupvar).with('facts', {}).and_return({ 'networking' => { 'fqdn' => host } }) + it { is_expected.to run.with_params(5).and_return('Pw5NP') } + it { is_expected.to run.with_params(10, 'abcd').and_return('cdadaaacaa') } + it { is_expected.to run.with_params(20, '', 'custom seed').and_return('3QKQHP4wmEObY3a6hkeg') } + it { is_expected.to run.with_params(20, '', 'custom seed', 1, 'extra').and_return('OA19SVDoc3QPY5NlSQ28') } end - function_args = [max] - if args.key?(:charset) || !extra.empty? - function_args << charset + context 'on a node named desktop-fln40kq.lan' do + let(:fqdn) { 'desktop-fln40kq.lan' } + + it { is_expected.to run.with_params(5).and_return('bgQsB') } + it { is_expected.to run.with_params(10, 'abcd').and_return('bcdbcdacad') } + it { is_expected.to run.with_params(20, '', 'custom seed').and_return('KaZsFlWkUo5SeA3gBEf0') } + it { is_expected.to run.with_params(20, '', 'custom seed', 1, 'extra').and_return('dcAzn1e8AA7hhoLpxAD6') } end - function_args += extra - scope.function_fqdn_rand_string(function_args) end end From 42ad4f6f24eff6320f8668e0e81787ba16d72ca1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Thu, 27 Apr 2023 12:27:02 -1000 Subject: [PATCH 2/2] Rewrite fqdn_rand_string() as a Puppet 4.x function The 3.x function rely on is_integer() which is deprecated. Rewrite it using the more modern puppet 4.x function to rely on data types for better parameters validation. --- lib/puppet/functions/fqdn_rand_string.rb | 37 +++++++++++++++++ .../parser/functions/fqdn_rand_string.rb | 41 ------------------- spec/functions/fqdn_rand_string_spec.rb | 26 ++++++------ 3 files changed, 50 insertions(+), 54 deletions(-) create mode 100644 lib/puppet/functions/fqdn_rand_string.rb delete mode 100644 lib/puppet/parser/functions/fqdn_rand_string.rb diff --git a/lib/puppet/functions/fqdn_rand_string.rb b/lib/puppet/functions/fqdn_rand_string.rb new file mode 100644 index 000000000..003549649 --- /dev/null +++ b/lib/puppet/functions/fqdn_rand_string.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +# @summary +# Generates a random alphanumeric string. Combining the `$fqdn` fact and an +# optional seed for repeatable randomness. +# +# Optionally, you can specify a character set for the function (defaults to alphanumeric). +Puppet::Functions.create_function(:fqdn_rand_string) do + # @param length The length of the resulting string. + # @param charset The character set to use. + # @param The seed for repeatable randomness. + # + # @return [String] + # + # @example Example Usage: + # fqdn_rand_string(10) + # fqdn_rand_string(10, 'ABCDEF!@$%^') + # fqdn_rand_string(10, '', 'custom seed') + dispatch :fqdn_rand_string do + param 'Integer[1]', :length + optional_param 'String', :charset + optional_repeated_param 'Any', :seed + end + + def fqdn_rand_string(length, charset = '', *seed) + charset = charset.chars.to_a + + charset = (0..9).map { |i| i.to_s } + ('A'..'Z').to_a + ('a'..'z').to_a if charset.empty? + + rand_string = '' + length.times do |current| + rand_string += charset[call_function('fqdn_rand', charset.size, (seed + [current + 1]).join(':'))] + end + + rand_string + end +end diff --git a/lib/puppet/parser/functions/fqdn_rand_string.rb b/lib/puppet/parser/functions/fqdn_rand_string.rb deleted file mode 100644 index 20248ae1a..000000000 --- a/lib/puppet/parser/functions/fqdn_rand_string.rb +++ /dev/null @@ -1,41 +0,0 @@ -# frozen_string_literal: true - -Puppet::Parser::Functions.newfunction(:fqdn_rand_string, arity: -2, type: :rvalue, doc: <<-DOC - @summary - Generates a random alphanumeric string. Combining the `$fqdn` fact and an - optional seed for repeatable randomness. - - Optionally, you can specify a character set for the function (defaults to alphanumeric). - - Arguments - * An integer, specifying the length of the resulting string. - * Optionally, a string specifying the character set. - * Optionally, a string specifying the seed for repeatable randomness. - - @return [String] - - @example Example Usage: - fqdn_rand_string(10) - fqdn_rand_string(10, 'ABCDEF!@$%^') - fqdn_rand_string(10, '', 'custom seed') - DOC -) do |args| - raise(ArgumentError, 'fqdn_rand_string(): wrong number of arguments (0 for 1)') if args.empty? - Puppet::Parser::Functions.function('is_integer') - raise(ArgumentError, 'fqdn_rand_string(): first argument must be a positive integer') unless function_is_integer([args[0]]) && args[0].to_i > 0 - raise(ArgumentError, 'fqdn_rand_string(): second argument must be undef or a string') unless args[1].nil? || args[1].is_a?(String) - - Puppet::Parser::Functions.function('fqdn_rand') - - length = args.shift.to_i - charset = args.shift.to_s.chars.to_a - - charset = (0..9).map { |i| i.to_s } + ('A'..'Z').to_a + ('a'..'z').to_a if charset.empty? - - rand_string = '' - for current in 1..length # rubocop:disable Style/For : An each loop would not work correctly in this circumstance - rand_string += charset[function_fqdn_rand([charset.size, (args + [current.to_s]).join(':')]).to_i] - end - - rand_string -end diff --git a/spec/functions/fqdn_rand_string_spec.rb b/spec/functions/fqdn_rand_string_spec.rb index 6ff30a980..2ba0779cd 100644 --- a/spec/functions/fqdn_rand_string_spec.rb +++ b/spec/functions/fqdn_rand_string_spec.rb @@ -6,20 +6,20 @@ let(:default_charset) { %r{\A[a-zA-Z0-9]{100}\z} } it { is_expected.not_to eq(nil) } - it { is_expected.to run.with_params.and_raise_error(ArgumentError, %r{wrong number of arguments}i) } - it { is_expected.to run.with_params(0).and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params(1.5).and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params(-10).and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params('-10').and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params('string').and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params([]).and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params({}).and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params(1, 1).and_raise_error(ArgumentError, %r{second argument must be undef or a string}) } - it { is_expected.to run.with_params(1, []).and_raise_error(ArgumentError, %r{second argument must be undef or a string}) } - it { is_expected.to run.with_params(1, {}).and_raise_error(ArgumentError, %r{second argument must be undef or a string}) } + it { is_expected.to run.with_params.and_raise_error(ArgumentError, %r{expects at least 1 argument, got none}i) } + it { is_expected.to run.with_params(0).and_raise_error(ArgumentError, %r{parameter 'length' expects an Integer\[1\] value, got Integer\[0, 0\]}) } + it { is_expected.to run.with_params(1.5).and_raise_error(ArgumentError, %r{parameter 'length' expects an Integer\ value, got Float}) } + it { is_expected.to run.with_params(-10).and_raise_error(ArgumentError, %r{parameter 'length' expects an Integer\[1\] value, got Integer\[-10, -10\]}) } + it { is_expected.to run.with_params('-10').and_raise_error(ArgumentError, %r{parameter 'length' expects an Integer\ value, got String}) } + it { is_expected.to run.with_params('string').and_raise_error(ArgumentError, %r{parameter 'length' expects an Integer\ value, got String}) } + it { is_expected.to run.with_params([]).and_raise_error(ArgumentError, %r{parameter 'length' expects an Integer value, got Array}) } + it { is_expected.to run.with_params({}).and_raise_error(ArgumentError, %r{parameter 'length' expects an Integer value, got Hash}) } + it { is_expected.to run.with_params(1, 1).and_raise_error(ArgumentError, %r{parameter 'charset' expects a String value, got Integer}) } + it { is_expected.to run.with_params(1, []).and_raise_error(ArgumentError, %r{parameter 'charset' expects a String value, got Array}) } + it { is_expected.to run.with_params(1, {}).and_raise_error(ArgumentError, %r{parameter 'charset' expects a String value, got Hash}) } + it { is_expected.to run.with_params('100').and_raise_error(ArgumentError, %r{parameter 'length' expects an Integer value, got String}) } + it { is_expected.to run.with_params(100, nil).and_raise_error(ArgumentError, %r{parameter 'charset' expects a String value, got Undef}) } it { is_expected.to run.with_params(100).and_return(default_charset) } - it { is_expected.to run.with_params('100').and_return(default_charset) } - it { is_expected.to run.with_params(100, nil).and_return(default_charset) } it { is_expected.to run.with_params(100, '').and_return(default_charset) } it { is_expected.to run.with_params(100, 'a').and_return(%r{\Aa{100}\z}) } it { is_expected.to run.with_params(100, 'ab').and_return(%r{\A[ab]{100}\z}) }