-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Migrate to puppet4 datatypes #1621
Conversation
4484ca0
to
e1c657f
Compare
manifests/init.pp
Outdated
| $default_type = 'none', | ||
| $dev_packages = $::apache::params::dev_packages, | ||
| $ip = undef, | ||
| Boolean$service_enable = true, |
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.
Missing space
manifests/mod/authnz_ldap.pp
Outdated
| $verify_server_cert = true, | ||
| $verifyServerCert = undef, | ||
| $package_name = undef, | ||
| Boolean$verify_server_cert = true, |
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.
Missing space
manifests/mod/dumpio.pp
Outdated
| @@ -1,10 +1,8 @@ | |||
| class apache::mod::dumpio( | |||
| $dump_io_input = 'Off', | |||
| $dump_io_output = 'Off', | |||
| Enum['Off', 'On'] $dump_io_input = 'Off', | |||
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.
Lowercase versions are also allowed
manifests/mod/ldap.pp
Outdated
| ::apache::mod { 'ldap': | ||
| package => $package_name, | ||
| package => $package_name, |
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.
??
manifests/mod/passenger.pp
Outdated
| $passenger_max_instances_per_app = undef, | ||
| $passenger_use_global_queue = undef, | ||
| $passenger_app_env = undef, | ||
| Optional[Stdlib::AbsolutAbsoluteppath] $passenger_log_file = 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.
Type typo?
manifests/mod/php.pp
Outdated
| ) inherits apache::params { | ||
| include ::apache | ||
| $mod = "php${php_version}" | ||
| $mod = "php${php_version}" |
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.
??
manifests/mod/status.pp
Outdated
| $apache_version = undef, | ||
| $status_path = '/server-status', | ||
| Array $allow_from = ['127.0.0.1','::1'], | ||
| Enum['On', 'Off'] $extended_status = 'On', |
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.
Lowercase is currently allowed too
manifests/service.pp
Outdated
| case $service_ensure { | ||
| true, false, 'running', 'stopped': { | ||
| $_service_ensure = $service_ensure | ||
| $_service_ensure = $service_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.
??
manifests/service.pp
Outdated
| } | ||
| default: { | ||
| $_service_ensure = undef | ||
| $_service_ensure = 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.
??
manifests/vhost.pp
Outdated
| $modsec_audit_log_file = undef, | ||
| $modsec_audit_log_pipe = undef, | ||
| $error_documents = [], | ||
| Variant[Stdlib::Absolutepath, Enum['disabled']] $fallbackresource = 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.
Also 'Optional'?
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.
fixed
manifests/init.pp
Outdated
| @@ -107,10 +99,6 @@ | |||
| validate_re($mpm_module, $valid_mpms_re) | |||
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.
Would assert_type(Pattern[$valid_mpms_re], $mpm_module) be better 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.
thanks, updated
manifests/mod/passenger.pp
Outdated
| $passenger_max_instances_per_app = undef, | ||
| $passenger_use_global_queue = undef, | ||
| $passenger_app_env = undef, | ||
| Optional[Stdlib::Absoluteppath] $passenger_log_file = 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.
typo in ppath
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.
fixed
metadata.json
Outdated
| {"name":"puppetlabs/stdlib","version_requirement":">= 4.2.0 < 5.0.0"}, | ||
| {"name":"puppetlabs/concat","version_requirement":">= 1.1.1 < 3.0.0"} | ||
| {"name":"puppetlabs/stdlib","version_requirement":">= 4.13.1 < 5.0.0"}, | ||
| {"name":"puppetlabs/concat","version_requirement":">= 2.2.1 < 3.0.0"} |
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.
Does this work with version 4.0.0 as well?
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.
stdlib 4.0? No. 4.13.1 was the first useable version with the pupept4 types uses 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 meant concat. It's now pinned to < 3.0.0 but I think it can be < 5.0.0 so it can use Puppet 4 types there as well.
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.
ah right. the apache module + concat4 works fine on my environment. But I'm not sure if bumping it should be part of this PR.
f4f30d7
to
b0ef81e
Compare
|
|
||
| if $allow_encoded_slashes { | ||
| validate_re($allow_encoded_slashes, '(^on$|^off$|^nodecode$)', "${allow_encoded_slashes} is not permitted for allow_encoded_slashes. Allowed values are 'on', 'off' or 'nodecode'.") | ||
| assert_type(Pattern[$valid_mpms_re], $mpm_module) |
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 wonder if this could be rewritten to an Enum. That's most likely an easier to read error.
| $apache_version = undef, | ||
| $package_name = undef, | ||
| $ldap_trusted_global_cert_file = undef, | ||
| Optional[String] $ldap_trusted_global_cert_type = 'CA_BASE64', |
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 there's a default value, does the Optional part even make sense?
|
|
||
| validate_array($ssl_proxy_protocol) | ||
| validate_string($ssl_sessioncache) | ||
|
|
||
| if is_bool($ssl_honorcipherorder) { |
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 this can be one big switch statement that includes true and false because I believe is_bool also emits a deprecation warning.
| ) { | ||
|
|
||
| # The base class must be included first because parameter defaults depend on it | ||
| if ! defined(Class['apache::params']) { |
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 think this can be removed if you use the String[1] type for $service_name.
5b19458
to
b285987
Compare
goal of this is to get rid of all the legacy valida_* function calls, and not to provide perfect datatypes for all parameters.