Skip to content
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 #852

Merged
merged 27 commits into from Jun 5, 2017
Merged

Migrate to puppet4 datatypes #852

merged 27 commits into from Jun 5, 2017

Conversation

@bastelfreak
Copy link
Contributor

@bastelfreak bastelfreak commented Mar 17, 2017

The goal of this is to get rid of most of the validate_* calls, not to introduce datatypes to all params.

@@ -49,40 +49,40 @@
user => $user,
auth_method => 'ident',
auth_option => $local_auth_option,
order => '001',
order => 001,

This comment has been minimized.

@binford2k

binford2k Mar 21, 2017
Member

This is probably a bad pattern to get into. The leading 0 makes it octal, which is fine by this, but not for other numbers.

$ puppet apply -e 'notice(002)'
Notice: Scope(Class[main]): 2
Notice: Compiled catalog for ganymede.local in environment production in 0.15 seconds
Notice: Applied catalog in 0.03 seconds
$ puppet apply -e 'notice(009)'
Error: Could not parse for environment production: Not a valid octal number 009  at line 1:8 on node ganymede.local
$validcon_script_path = $postgresql::params::validcon_script_path,
$package_name = $postgresql::params::client_package_name,
$package_ensure = 'present'
Enum['file', 'absent'] $file_ensure = 'file',

This comment has been minimized.

@alexjfisher

alexjfisher May 12, 2017
Contributor

Bad style, but some users might be using 'present'?

This comment has been minimized.

@bastelfreak

bastelfreak May 15, 2017
Author Contributor

the next release will be a major one, so should be prohibit the bad practice?

This comment has been minimized.

$package_ensure = 'present',
$link_pg_config = $postgresql::params::link_pg_config
String $package_name = $postgresql::params::devel_package_name,
Enum['present', 'absent', 'latest']$package_ensure = 'present',

This comment has been minimized.

@alexjfisher

alexjfisher May 12, 2017
Contributor

Missing space?

This comment has been minimized.

@alexjfisher

alexjfisher May 12, 2017
Contributor

Also need 'installed'?

This comment has been minimized.

@bastelfreak

bastelfreak May 15, 2017
Author Contributor

fixed

$package_name = $postgresql::params::docs_package_name,
$package_ensure = 'present',
String $package_name = $postgresql::params::docs_package_name,
Enum['present', 'absent', 'latest'] $package_ensure = 'present',

This comment has been minimized.

@alexjfisher

alexjfisher May 12, 2017
Contributor

And 'installed'?

$package_name = $postgresql::params::java_package_name,
$package_ensure = 'present'
String $package_name = $postgresql::params::java_package_name,
Enum['present', 'absent', 'latest'] $package_ensure = 'present'

This comment has been minimized.

@alexjfisher

alexjfisher May 12, 2017
Contributor

Installed?

This comment has been minimized.

@bastelfreak

bastelfreak May 15, 2017
Author Contributor

fixed

$package_name = $postgresql::params::contrib_package_name,
$package_ensure = 'present'
String $package_name = $postgresql::params::contrib_package_name,
Enum['present', 'absent', 'latest'] $package_ensure = 'present'

This comment has been minimized.

@alexjfisher

alexjfisher May 12, 2017
Contributor

Installed?

This comment has been minimized.

@bastelfreak

bastelfreak May 15, 2017
Author Contributor

fixed

String $object_type = 'database',
$object_name = undef,
String $psql_db = $postgresql::server::default_database,
String$psql_user = $postgresql::server::user,

This comment has been minimized.

@alexjfisher

alexjfisher May 12, 2017
Contributor

Missing space.

This comment has been minimized.

@bastelfreak

bastelfreak May 15, 2017
Author Contributor

fixed

$package_name = $postgresql::params::postgis_package_name,
$package_ensure = 'present'
String $package_name = $postgresql::params::postgis_package_name,
Enum['present', 'absent', 'latest'] $package_ensure = 'present'

This comment has been minimized.

@alexjfisher

alexjfisher May 12, 2017
Contributor

Installed?

This comment has been minimized.

@bastelfreak

bastelfreak May 15, 2017
Author Contributor

fixed

{"name":"puppetlabs/concat","version_requirement":">= 1.1.0 <3.0.0"}
{"name":"puppetlabs/stdlib","version_requirement":">= 4.13.1 < 5.0.0"},
{"name":"puppetlabs/apt","version_requirement":">= 2.0.0 < 3.0.0"},
{"name":"puppetlabs/concat","version_requirement":">= 1.1.0 < 3.0.0"}

This comment has been minimized.

@alexjfisher

alexjfisher May 12, 2017
Contributor

Is this still correct? (New versions of concat have been released since this PR was opened)

This comment has been minimized.

@bastelfreak

bastelfreak May 15, 2017
Author Contributor

I'm happily using it with concat4. but bumping this shouldn't be part of this PR.

@@ -190,7 +143,7 @@ class { 'postgresql::server': }
it do
is_expected.to contain_concat__fragment('pg_hba_rule_test').with({
:content => /local\s+all\s+all\s+0\.0\.0\.0\/0\s+peer/
})
})

This comment has been minimized.

@alexjfisher

alexjfisher May 12, 2017
Contributor

Alignment doesn't look right here

This comment has been minimized.

@bastelfreak

bastelfreak May 15, 2017
Author Contributor

fixed

$package_ensure = 'present'
Enum['file', 'absent'] $file_ensure = 'file',
Stdlib::Absolutepath $validcon_script_path = $postgresql::params::validcon_script_path,
String $package_name = $postgresql::params::client_package_name,

This comment has been minimized.

@hasegeli

hasegeli May 13, 2017
Contributor

It cannot be an empty string. String[1] would be more appropriate.

This comment has been minimized.

@bastelfreak

bastelfreak May 21, 2017
Author Contributor

fixed

Enum['file', 'absent'] $file_ensure = 'file',
Stdlib::Absolutepath $validcon_script_path = $postgresql::params::validcon_script_path,
String $package_name = $postgresql::params::client_package_name,
Enum['present', 'absent', 'latest'] $package_ensure = 'present'

This comment has been minimized.

@hasegeli

hasegeli May 13, 2017
Contributor

We can define it as Postgresql::Package_ensure not to repeat ourself.

@@ -49,40 +49,40 @@
user => $user,
auth_method => 'ident',
auth_option => $local_auth_option,
order => '001',
order => 1,

This comment has been minimized.

@hasegeli

hasegeli May 13, 2017
Contributor

This is actually working as a string on the Concat module. 2 would be sorted after after 10.

This comment has been minimized.

@hunner

hunner May 25, 2017
Member

Can we change the concat resource to order => 'numeric' to help? I think that makes more sense anyway.

This comment has been minimized.

@bastelfreak

bastelfreak May 25, 2017
Author Contributor

done

String $db,
Optional[String] $privilege = undef,
String $object_type = 'database',
$object_name = undef,

This comment has been minimized.

@hasegeli

hasegeli May 13, 2017
Contributor

Can't we define this one as String[1]?

This comment has been minimized.

@bastelfreak

bastelfreak May 21, 2017
Author Contributor

Which atribute do you mean, object_name? This one is optional.

This comment has been minimized.

@hasegeli

hasegeli May 21, 2017
Contributor

Optional[String[1]] maybe?

This comment has been minimized.

@bastelfreak

bastelfreak May 21, 2017
Author Contributor

sounds good, will do.

String$psql_user = $postgresql::server::user,
Integer $port = $postgresql::server::port,
Boolean $onlyif_exists = false,
Hash $connect_settings = $postgresql::server::default_connect_settings,

This comment has been minimized.

@hasegeli

hasegeli May 13, 2017
Contributor

Can't we specify what is in the Hash?

$object_name = undef,
String $psql_db = $postgresql::server::default_database,
String$psql_user = $postgresql::server::user,
Integer $port = $postgresql::server::port,

This comment has been minimized.

@hasegeli

hasegeli May 13, 2017
Contributor

It should be greater than 0. I am not sure if there is a portable max limit.

@bastelfreak bastelfreak force-pushed the bastelfreak:puppet4 branch 3 times, most recently from 81a02a4 to bd7cfba May 15, 2017
@ekohl
ekohl approved these changes May 21, 2017
Enum['file', 'absent'] $file_ensure = 'file',
Stdlib::Absolutepath $validcon_script_path = $postgresql::params::validcon_script_path,
String[1] $package_name = $postgresql::params::client_package_name,
Enum['present', 'absent', 'latest'] $package_ensure = 'present'

This comment has been minimized.

@ekohl

ekohl May 21, 2017
Contributor

Users can ensure a specific version. This enum limits it so I'm afraid this must just be String. This is true for all the $package_ensure occurrences.

$package_name = $postgresql::params::devel_package_name,
$package_ensure = 'present',
$link_pg_config = $postgresql::params::link_pg_config
String $package_name = $postgresql::params::devel_package_name,

This comment has been minimized.

@ekohl

ekohl May 21, 2017
Contributor

Can be String[1]. Applies to more $package_name occurrences.

$group,
$role = $name,
$ensure = 'present',
String $group,

This comment has been minimized.

@ekohl

ekohl May 21, 2017
Contributor

I believe this accepts empty strings. Maybe String[1]? Same for role below.

This comment has been minimized.

@bastelfreak

bastelfreak May 25, 2017
Author Contributor

fixed

Enum['local', 'host', 'hostssl', 'hostnossl'] $type,
String $database,
String $user,
String $auth_method,

This comment has been minimized.

@ekohl

ekohl May 21, 2017
Contributor

In the validation code there's some regex validation with validate_re. I believe you can simplify that to assert_type(Enum[$allowed_auth_methods], $auth_method) but I'm not 100% sure.

This comment has been minimized.

@hunner

hunner May 25, 2017
Member

I think that's valid and useful.

This comment has been minimized.

@bastelfreak

bastelfreak May 25, 2017
Author Contributor

fixed

Copy link
Contributor

@ekohl ekohl left a comment

I meant this and need to leave a comment. Are you happy now Github?

@bastelfreak bastelfreak force-pushed the bastelfreak:puppet4 branch 2 times, most recently from f9cd11c to 43adc82 May 23, 2017
$package_ensure = 'present',
$link_pg_config = $postgresql::params::link_pg_config
String $package_name = $postgresql::params::devel_package_name,
Enum['present', 'absent', 'latest', 'installed'] $package_ensure = 'present',

This comment has been minimized.

@hunner

hunner May 25, 2017
Member

Here as well.

$package_name = $postgresql::params::docs_package_name,
$package_ensure = 'present',
String $package_name = $postgresql::params::docs_package_name,
Enum['present', 'absent', 'latest', 'installed'] $package_ensure = 'present',

This comment has been minimized.

@hunner

hunner May 25, 2017
Member

Here as well.

$package_name = $postgresql::params::java_package_name,
$package_ensure = 'present'
String $package_name = $postgresql::params::java_package_name,
Enum['present', 'absent', 'latest', 'installed'] $package_ensure = 'present'

This comment has been minimized.

@hunner

hunner May 25, 2017
Member

Here as well.

@@ -49,40 +49,40 @@
user => $user,
auth_method => 'ident',
auth_option => $local_auth_option,
order => '001',
order => 1,

This comment has been minimized.

@hunner

hunner May 25, 2017
Member

Can we change the concat resource to order => 'numeric' to help? I think that makes more sense anyway.

$package_name = $postgresql::params::contrib_package_name,
$package_ensure = 'present'
String $package_name = $postgresql::params::contrib_package_name,
Enum['present', 'absent', 'latest', 'installed'] $package_ensure = 'present'

This comment has been minimized.

@hunner

hunner May 25, 2017
Member

Here as well.

Enum['local', 'host', 'hostssl', 'hostnossl'] $type,
String $database,
String $user,
String $auth_method,

This comment has been minimized.

@hunner

hunner May 25, 2017
Member

I think that's valid and useful.

$package_name = $postgresql::params::postgis_package_name,
$package_ensure = 'present'
String $package_name = $postgresql::params::postgis_package_name,
Enum['present', 'absent', 'latest', 'installed'] $package_ensure = 'present'

This comment has been minimized.

@hunner

hunner May 25, 2017
Member

Here as well.

bastelfreak added 18 commits Mar 17, 2017
this version is needed to get the datatypes
bastelfreak added 9 commits Mar 17, 2017
we only allowed present/absent/latest on the first attempt. But people
can also pass their own version strings so we've to accept any not empty
string.
@bastelfreak bastelfreak force-pushed the bastelfreak:puppet4 branch from fd47d59 to b0cafe6 May 25, 2017
@hunner hunner merged commit b0cafe6 into puppetlabs:master Jun 5, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hunner added a commit that referenced this pull request Jun 5, 2017
The goal of this is to get rid of most of the validate_* calls, not to introduce datatypes to _all_ params.
@bastelfreak bastelfreak deleted the bastelfreak:puppet4 branch Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants