timeout: switch from String to Integer datatype #2241
Conversation
|
Cocker Koch seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
bastelfreak
changed the title
manifests/init.pp
Outdated
| @@ -491,7 +491,7 @@ | |||
| Optional[Enum['On', 'Off', 'on', 'off']] $ldap_verify_server_cert = undef, | |||
| Optional[String] $ldap_trusted_mode = undef, | |||
| Boolean $error_documents = false, | |||
| String $timeout = '60', | |||
| Integer $timeout = 60, | |||
There was a problem hiding this comment.
IMHO an integer would be better, but it is a breaking change. Would it be better to make it
| Integer $timeout = 60, | |
| Variant[Integer[0], String[1]] $timeout = 60, |
(and then properly align it, that's hard in GH)
There was a problem hiding this comment.
What do you think of Variant[Integer[0], Pattern[/^[0-9]+$/]]?
There was a problem hiding this comment.
Probably even better since it's stricter.
There was a problem hiding this comment.
FYI: The next release will be a major one, so if we want to break things, now is a good time
There was a problem hiding this comment.
FYI: The next release will be a major one, so if we want to break things, now is a good time
Adding Datatypes to the Parameters actually was a good Idea, but IMHO it was done only half-hearted. This should be improved for the next Major-Release.
|
@cocker-cc Did not see this PR prior to putting up my own fix sorry, a bugfix which has been merged in to set the type to |
|
I'm not sure I agree with this being closed. It actually narrows the data types further to disallow misconfigurations. |
Put Parameter $apache::timeout on Datatype Integer instead of String.