-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
Remove validate_numeric
and validate_string
#144
Conversation
Boolean $openmanage_enable = $snmp::params::openmanage_enable, | ||
Boolean $master = $snmp::params::master, | ||
$agentx_perms = $snmp::params::agentx_perms, | ||
$agentx_ping_interval = $snmp::params::agentx_ping_interval, | ||
$agentx_socket = $snmp::params::agentx_socket, | ||
$agentx_timeout = $snmp::params::agentx_timeout, | ||
$agentx_retries = $snmp::params::agentx_retries, | ||
Integer[0] $agentx_timeout = $snmp::params::agentx_timeout, |
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.
Positive Integers only.
@@ -53,14 +53,14 @@ | |||
|
|||
$snmp_agentx_timeout = getvar('::snmp_agentx_timeout') | |||
if $snmp_agentx_timeout { | |||
$agentx_timeout = $::snmp_agentx_timeout | |||
$agentx_timeout = 0 + $::snmp_agentx_timeout |
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.
The rest of the params file says ENC vars are often strings and uses str2bool
a lot.
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.
ugh :(
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.
@ekohl could confirm, but I think this is the case in foreman (at least for global parameters??)
The code could be cleared up a lot though. eg I think it's unnecessary to check if an ENC parameter is a string before calling str2bool
on it. str2bool
is a noop if called with something that's already a boolean.
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 want to keep supporting top scoped (global) parameters? This could easily be solved in hiera?
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.
Given it's a major version bump anyway, it's a good time to get rid of them IMHO
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.
And Foreman can't set types for global parameters AFAIK., but can set them on class parameters in the ENC. Yet another reason to remove the global scoped parameters IMHO.
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.
Maybe merge this for now and remove all the global parameter cruft in a separate PR?
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.
That'd be #145
A breaking change as we now don't accept numeric strings (unless they've come from the ENC where you might not have a choice). |
$template_snmpd_sysconfig = $snmp::params::template_snmpd_sysconfig, | ||
$template_snmptrapd = $snmp::params::template_snmptrapd, | ||
$template_snmptrapd_sysconfig = $snmp::params::template_snmptrapd_sysconfig, | ||
String[1] $template_snmpd_conf = $snmp::params::template_snmpd_conf, |
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.
can you fix the formatting and align the =
?
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.
meh. I was in two minds. See @ekohl's comment here. theforeman/puppet-pulp#341 (comment)
Maybe realign it all at the end it you really want? (ie after getting rid of the validate_array
s)?
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.
We may align only the equal signs.
Replace with data types
123120e
to
c3c8a60
Compare
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'm fine merging this now. There's still more validations left to nuke.
Replace with data types