Add Data Types and remove deprecated parameters #406

wyardley · 2017-10-20T18:47:25Z

First pass, may need to make some adjustments still.

Gets rid of all validate_* calls
Adds data types to most publicly accessible class params
Adds some basic class inclusion spec tests where they didn't exist before at @bastelfreak's suggestion.

For now, storage_engine is a string rather than enum, not sure if there's a small enough set of names to make this worth forcing.

juniorsysadmin · 2017-10-21T12:03:02Z

manifests/client.pp

@@ -9,10 +9,10 @@
 #   is repository dependent.
 #
 class mongodb::client (
-  $ensure       = $mongodb::params::package_ensure_client,
-  $package_name = $mongodb::params::client_package_name,
+  Variant[Boolean, String] $ensure = $mongodb::params::package_ensure_client,


Some use false. Do we have any convention in voxpupuli?

I based this on params - package_ensure_client has both $version and true. This module uses some weird patterns. I think we may want to try and fix some of the weird design decisions (though some of them would require major structural work), but for this pass, I'm just trying to keep things working.

Boolean looks really wrong here. But as a first step we should keep the current behavior to make a migration to datatypes easier.

Here's where it's used https://github.com/voxpupuli/puppet-mongodb/blob/master/manifests/params.pp#L45-L61
I think this is also causing some quirks when installing versions, because of vendor release tags etc.

ekohl · 2017-10-21T15:09:59Z

manifests/client.pp

@@ -9,10 +9,10 @@
 #   is repository dependent.
 #
 class mongodb::client (
-  $ensure       = $mongodb::params::package_ensure_client,
-  $package_name = $mongodb::params::client_package_name,
+  Variant[Boolean, String] $ensure = $mongodb::params::package_ensure_client,


Some use false. Do we have any convention in voxpupuli?

ekohl · 2017-10-21T15:10:48Z

manifests/client.pp

-  anchor { '::mongodb::client::start': }
-  -> class { '::mongodb::client::install': }
-  -> anchor { '::mongodb::client::end': }
+  anchor { 'mongodb::client::start': }


Maybe replace the anchor as well with a contain?

Yeah, I want to do the structural changes in another PR (if you want to start working on one, that would be great). But might be good to get the acceptance tests in a better place first? Right now they're a total disaster, maybe worse than when we started.

ekohl · 2017-10-21T15:14:16Z

manifests/server/config.pp

@@ -89,7 +85,6 @@
      $noauth = true
    }
    if $keyfile and $key {
-      validate_string($key)
      validate_re($key,'.{6}')


Does this mean it should be String[6] or String[6, 6]?

I think the key is longer than 6 characters, so maybe 6,? I didn't know you could specify the length of the string that way, but that makes sense, can work on replacing this one too.

Yeah, I think String[6] will do it. Updated.

bastelfreak · 2017-10-21T21:38:22Z

README.md

-class {'::mongodb::client': } ->
-class {'::mongodb::server': }
+class {'mongodb::client': } ->
+class {'mongodb::server': }


can you move the arrow to the next line?

bastelfreak · 2017-10-21T21:40:24Z

README.md

-class {'::mongodb::server': }->
-class {'::mongodb::client': }
+class {'mongodb::server': }->
+class {'mongodb::client': }


can you move the arrow to the next line?

bastelfreak · 2017-10-21T21:40:33Z

README.md

-class {'::mongodb::server': }->
-class {'::mongodb::client': }
+class {'mongodb::server': }->
+class {'mongodb::client': }


please move the arrow

bastelfreak · 2017-10-21T21:44:23Z

manifests/mongos.pp

+  String $package_name                                      = $mongodb::params::mongos_package_name,
+  Optional[Stdlib::Absolutepath] $unixsocketprefix          = $mongodb::params::mongos_unixsocketprefix,
+  Optional[Stdlib::Absolutepath] $pidfilepath               = $mongodb::params::mongos_pidfilepath,
+  Optional[Variant[Boolean, Stdlib::Absolutepath]] $logpath = $mongodb::params::mongos_logpath,


Having a boolean here looks interesting

I think it's needed because there are cases where there are defaults in certain cases (from params), but users need to be able to suppress them? So in that case, passing undef will get the default, but false will work to suppress it. This is a quirk of Puppet that I don't particularly like, but other than adding an additional boolean parameter, I don't know if there's an easy fix.

One option is Variant[Enum[''], Stdlib::Absolutepath] (or 'unmanaged'). These are times I miss the explicit null rather than undef.

bastelfreak · 2017-10-21T21:46:00Z

manifests/server.pp

+  Optional[String] $admin_password                    = undef,
+  Boolean $handle_creds                               = $mongodb::params::handle_creds,
+  Boolean $store_creds                                = $mongodb::params::store_creds,
+  Array $admin_roles                                  = ['userAdmin', 'readWrite', 'dbAdmin',


this is really long. should we move the data to params.pp?

voxpupuli/puppet-mongodb#406 has implemented some backwards incompatible changes in data types. As we are in the process of removing mongodb in RDO during queens cycle, i think there is no point in fixing tripleo related manifest and we can pin it to the last commit before the breaking change. Change-Id: I115ce0daffbe99adb639f27e3a0eb93a43e4ab18

wyardley added this to the 2.0.0 milestone Oct 20, 2017

wyardley mentioned this pull request Oct 20, 2017

Tasks for release 2.0.0 #405

Closed

8 tasks

wyardley changed the title ~~Add Data Types to most class parameters~~ Add Data Types and remove deprecated parameters Oct 20, 2017

wyardley added the backwards-incompatible label Oct 20, 2017

wyardley requested a review from bastelfreak October 21, 2017 00:34

juniorsysadmin reviewed Oct 21, 2017

View reviewed changes

ekohl reviewed Oct 21, 2017

View reviewed changes

bastelfreak reviewed Oct 21, 2017

View reviewed changes

bastelfreak approved these changes Oct 21, 2017

View reviewed changes

William Yardley added 4 commits October 21, 2017 15:40

Add data types to most params

9d8fcc5

Remove leading colons on class name and improve syntax in docs

6c9b463

Add basic class inclusion tests, and stub in necessary facts

daeeead

Add more data types based on param docs in README

c10e72d

bastelfreak merged commit 23068c1 into voxpupuli:master Oct 21, 2017

wyardley deleted the data_types branch November 8, 2017 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Data Types and remove deprecated parameters #406

Add Data Types and remove deprecated parameters #406

wyardley commented Oct 20, 2017 •

edited

Loading

juniorsysadmin Oct 21, 2017

ekohl Oct 21, 2017

wyardley Oct 21, 2017

bastelfreak Oct 21, 2017

wyardley Oct 21, 2017

ekohl Oct 21, 2017

ekohl Oct 21, 2017

wyardley Oct 21, 2017

ekohl Oct 21, 2017

wyardley Oct 21, 2017

wyardley Oct 21, 2017

bastelfreak Oct 21, 2017

bastelfreak Oct 21, 2017

bastelfreak Oct 21, 2017

bastelfreak Oct 21, 2017

wyardley Oct 21, 2017

ekohl Oct 22, 2017

bastelfreak Oct 21, 2017

Add Data Types and remove deprecated parameters #406

Add Data Types and remove deprecated parameters #406

Conversation

wyardley commented Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyardley commented Oct 20, 2017 •

edited

Loading