-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
replace validate_* with puppet4 datatypes #350
Conversation
this should be rebased after #349 got merged |
d1c5f47
to
5154fe1
Compare
I adjusted the types, there should be no backwards-incompatible changes. |
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.
It might also be a good time to remove leading colons as in manifests/dev/pp
L12's $::php::ensure
.
manifests/composer.pp
Outdated
$proxy_server = undef, | ||
Boolean $auto_update = true, | ||
Integer $max_age = $::php::params::composer_max_age, | ||
$root_group = $::php::params::root_group, |
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.
Variant[String, Integer]
?
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.
yep
manifests/config/setting.pp
Outdated
@@ -21,15 +21,13 @@ | |||
define php::config::setting( | |||
$key, | |||
$value, | |||
$file, | |||
String $file, |
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 should probably be a Stdlib::AbsolutePath
since it is passed to a path
parameter.
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.
yep
manifests/embedded.pp
Outdated
$ensure = $::php::ensure, | ||
$package = | ||
$ensure = $::php::ensure, | ||
$package = |
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.
Shouldn't these be strings as in php::dev
?
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.
yep
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.
Fix the alignment issues, and is there a reason this is done as 18 commits?
manifests/composer.pp
Outdated
$proxy_server = undef, | ||
Boolean $auto_update = true, | ||
Integer $max_age = $::php::params::composer_max_age, | ||
Variant[Integer, String] $root_group = $::php::params::root_group, |
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.
Alignment is off here (I don't care but I think our standard does).
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/globals.pp
Outdated
$fpm_pid_file = undef, | ||
Optional[Pattern[/^[57].[0-9]/]] $php_version = undef, | ||
Optional[Stdlib::Absolutepath] $config_root = undef, | ||
Optional[Stdlib::Absolutepath] $fpm_pid_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.
Alignment is 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.
fixed
manifests/packages.pp
Outdated
$names_to_prefix = prefix( | ||
String $ensure = $::php::ensure, | ||
Boolean $manage_repos = $::php::manage_repos, | ||
Array $names_to_prefix = prefix( | ||
$::php::params::common_package_suffixes, $::php::package_prefix # lint:ignore:parameter_documentation |
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 should be consistent about how we merge two values for a parameter name. Compare to manifests/embedded.pp
L18 or so.
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 reviewed all files, the format is now consistant
The changes for |
@dbeckham this is actually intentional. The goal for me is to get rid of all the legacy validate_* calls, because they are spamming the puppetserver logs and they get removed in the future. My goal (at the moment) is not to provide perfect types for all parameter, I don't have enough for it at the moment. However, we're happily accepting patches :) |
No description provided.