(CLOUD_1505) validate functions #99
Conversation
manifests/registry.pp
Outdated
| @@ -73,7 +73,7 @@ | |||
| $local_user_strip = regsubst($local_user, '-', '', 'G') | |||
|
|
|||
| $_pass_hash = $pass_hash ? { | |||
| Undef => pw_hash("${title}${auth_environment}${auth_cmd}${local_user}", 'SHA-512', $local_user_strip), | |||
| 'Undef' => pw_hash("${title}${auth_environment}${auth_cmd}${local_user}", 'SHA-512', $local_user_strip), | |||
There was a problem hiding this comment.
@sheenaajay have you run this through travis without the qoutes? The PR to remove them was valid and needs to as is.
manifests/init.pp
Outdated
| if $icc != undef { | ||
| validate_bool($icc) | ||
| if $version { | ||
| assert_type(String[1], $version) |
There was a problem hiding this comment.
Not 100% sure, but I think you can put this check in the type declaration itself, like - Optional[String[1]] $version - Will save you the extra couple of lines of the assert type here :)
manifests/init.pp
Outdated
| fail('This module only works on Debian or Red Hat based systems or on Archlinux as on Gentoo.') | ||
| } | ||
| } | ||
| assert_type(Boolean, $manage_kernel) |
There was a problem hiding this comment.
When you define the params at the top with the Data Type it's pretty much the same as doing an assert type on the param - If it comes through as anything other than the specified Data Type it'd fail further up at the param definition, so you can take all these assert types out!
manifests/init.pp
Outdated
| validate_string($tls_cacert) | ||
| validate_string($tls_cert) | ||
| validate_string($tls_key) | ||
| assert_type(String[1], $tls_cacert) |
There was a problem hiding this comment.
Again, if it makes sense, I'd remove these assert types and add the string lower limit to the data type - Optional[String[1]] $tls_cacert
manifests/registry.pp
Outdated
| ) { | ||
| include docker::params | ||
|
|
||
| validate_re($ensure, '^(present|absent)$') | ||
| assert_type(Pattern[/^present$|^absent$/], $ensure) |
There was a problem hiding this comment.
You can use Pattern[] as a data type! So you could remove this line and have the definition further up as -
Optional[Pattern[/^present$|^absent$/]] $ensure - https://puppet.com/docs/puppet/5.3/lang_data_abstract.html#pattern
manifests/run.pp
Outdated
| validate_re($memory_limit, '^[\d]*(b|k|m|g)$') | ||
| validate_re($ensure, '^(present|absent)') | ||
| if $image { | ||
| assert_type(Pattern[/^[\S]*$/], $image) |
There was a problem hiding this comment.
As above - I'd consider moving the params here to data type Pattern to be able to remove the assert_types here.
|
@davejrt Thanks alot Dave.Will check the local environment and reexecute the unit testcases.Travis jobs are running clean. |
|
@HelenCampbell Thanks alot for the review. |
|
@davejrt @HelenCampbell Thanks for the reviews.All comments are incorporated. |
manifests/compose.pp
Outdated
| Optional[Pattern[/^present$|^absent$/]] $ensure = 'present', | ||
| Optional[String] $version = $docker::params::compose_version, | ||
| Optional[String] $install_path = $docker::params::compose_install_path, | ||
| Optional[Pattern[/^(https?:\/\/)?([^:^@]+:[^:^@]+@|)([\da-z\.-]+)\.([\da-z\.]{2,6})(:[\d])?([\/\w \.-]*)*\/?$/]] $proxy = undef |
There was a problem hiding this comment.
I have to scroll to see what the parameter names are, maybe a custom type alias is a good idea here
There was a problem hiding this comment.
Same comment for every large data type
There was a problem hiding this comment.
@jeefberkey Thank you for the valuable comment
There was a problem hiding this comment.
@jeefberkey The large data types are not repeating alot to create a data type.So will specify the assert_type function for all large data types outside the variable declaration area so keep it short.
|
LGTM |
|
We are having issues upgrading past this commit as well due to dm_use_deferred_deletion and dm_use_deferred_removal being set to String instead of Boolean. Perhaps it was set to String because the default was set to 'undef' (converted to false)? My concern is that while the String update breaks our config, I don't know if other installations might be affected the other way (although lint doesn't like a quoted true/false, so I'm assuming most probably used booleans by default). |
Request to review the changes for replacing validate functions with datatypes and assert_type function.
Following jobs are running clean
bundle exec rake test
bundle exec rake spec
bundle exec rake acceptance