Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rewrite fqdn_rand_string() as a Puppet 4.x function #1343

Merged
merged 2 commits into from
May 4, 2023
Merged

Rewrite fqdn_rand_string() as a Puppet 4.x function #1343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4255f5c
Rework the tests to check actual values
smortex Apr 27, 2023
42ad4f6
Rewrite fqdn_rand_string() as a Puppet 4.x function
smortex Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions lib/puppet/functions/fqdn_rand_string.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto re namespacing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is the plan 😁 I am currently playing three-cushion billiards:

  1. Adjust non-deprecated functions so that they do not rely on deprecated functions; <--- we are here
  2. Remove deprecated functions (since they have been deprecated for a long time);
  3. "Break things" and namespace functions. <--- what you are talking about

The above is pretty clean in my head but I guess I should dump this somewhere where we can track progress, see what is coming next and so on. I am really not good at this, and any suggestion is welcome!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, feel free to close all comments related to this and we can deal with the general issue in #1346

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I subscribed to the issue. Thanks!

# @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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it necessary to make this a breaking change? I have code that called the function like this fqdn_rand_string(20, undef, 'MY_SEED')

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #1367

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
41 changes: 0 additions & 41 deletions lib/puppet/parser/functions/fqdn_rand_string.rb
View file
Open in desktop

This file was deleted.

86 changes: 34 additions & 52 deletions spec/functions/fqdn_rand_string_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,68 +6,50 @@
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}) }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'100' was allowed by is_integer() and was removed bellow.

it { is_expected.to run.with_params(100, nil).and_raise_error(ArgumentError, %r{parameter 'charset' expects a String value, got Undef}) }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil was accepted with 3.x function and is rejected by 4.x function. It was remove bellow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might create a PR to revert this though. fqdn_rand_string(20, undef, 'MY_SEED') was a perfectly acceptable way of calling the function IMO.

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}) }
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