-
-
Notifications
You must be signed in to change notification settings - Fork 881
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.
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
BREAKING: Drop puppet 3 support. Replace validate_* with datatypes #1031
Conversation
fail('$port must be an integer.') | ||
} | ||
|
||
$ensure_real = $ensure ? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure $ensure is used at all at the moment. Use of $ensure_real
was removed in e40ce7c ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as mentioned in the other comment. We will drop the param. We can safely do this since the next release has to be a major one.
@@ -37,26 +37,11 @@ | |||
define nginx::resource::upstream::member ( | |||
$upstream, | |||
$server, | |||
$ensure = 'present', | |||
$port = 80, | |||
Enum['present', 'absent'] $ensure = 'present', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this parameter isn't used anymore, maybe default it to undef
instead?
Then
if $ensure { warning('DEPRECATION: some useful message here') }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer it to directly drop it in the next release, since this has to be a major one we can get rid of a lot of stuff.
reminder for @bastelfreak: drop the parameter in a separate PR to get it into the changelog. |
b1ce4a0
to
46973e6
Compare
This should be enough for the first run. I don't want to create a bigger diff. Let me know if it is already too big, I could cherry-pick the changes in each file to own PRs. |
Just picking out the breaking change(s) into their own PR(s) works for me. |
we should wait with merging until I opened PRs for all breaking changes |
validate_re($string, '^.{2,}$', | ||
"Invalid string value [${string}]. Expected a minimum of 2 characters.") | ||
if ! ( is_array($mappings) or is_hash($mappings) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one might be useful to keep around? The error message is a little more explanatory than the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed on IRC: we can kep the fail() call but this will never be reached. I wanted to move it as comments to the header documentation, but I jut noticed that it is already present.
Variant[Array, String] $ipv6_listen_ip = '::', | ||
Integer $ipv6_listen_port = 80, | ||
String $ipv6_listen_options = 'default ipv6only=on', | ||
$proxy = undef, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have data types for these handful of un-decorated parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. But I want to add them in another PR. I want to keep the focus here on replacing validate functions. and in the next step adding types for all parameters + maybe improve existing ones.
replace validate_* with datatypes
No description provided.