Rewrite fqdn_rand_string() as a Puppet 4.x function #1343
Conversation
smortex
force-pushed
the
fqdn_rand_string
branch
2 times, most recently
from
ea6fb65
to
0353cae
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
smortex
requested review from
a team,
alexjfisher,
b4ldr,
bastelfreak and
ekohl
as code owners
| 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}) } |
There was a problem hiding this comment.
'100' was allowed by is_integer() and was removed bellow.
| 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}) } |
There was a problem hiding this comment.
nil was accepted with 3.x function and is rejected by 4.x function. It was remove bellow.
There was a problem hiding this comment.
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.
| # 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 |
There was a problem hiding this comment.
Yeah, it is the plan 😁 I am currently playing three-cushion billiards:
- Adjust non-deprecated functions so that they do not rely on deprecated functions; <--- we are here
- Remove deprecated functions (since they have been deprecated for a long time);
- "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!
There was a problem hiding this comment.
ack, feel free to close all comments related to this and we can deal with the general issue in #1346
There was a problem hiding this comment.
Great! I subscribed to the issue. Thanks!
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.
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.
smortex
force-pushed
the
fqdn_rand_string
branch
from
0353cae
to
4255f5c
Compare
| # fqdn_rand_string(10, '', 'custom seed') | ||
| dispatch :fqdn_rand_string do | ||
| param 'Integer[1]', :length | ||
| optional_param 'String', :charset |
There was a problem hiding this comment.
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')
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.fqdn_rand_string()as a Puppet 4.x function.