Move params to classes and hiera; Align defaults with supported versions #712
Conversation
h-haaks
force-pushed
the
move-params-to-hiera
branch
from
fd3cc23
to
625aa83
Compare
| @@ -82,8 +82,6 @@ | |||
| group => $group, | |||
| } | |||
|
|
|||
| if ($logpath and $syslog) { fail('You cannot use syslog with logpath') } | |||
|
|
|||
There was a problem hiding this comment.
Reordered checks in template so logpath is just ignored if syslog is true
| let(:pre_condition) do | ||
| 'class { mongodb::globals: | ||
| manage_package => true | ||
| manage_package_repo => false |
There was a problem hiding this comment.
manage_package is removed.
Breaking change?
| # @param bind_ip | ||
| # This setting can be used to configure MonogDB process to bind to and listen for connections from applications on this address. | ||
| # If not specified, the module will use the default for your OS distro. | ||
| # Note: This value should be passed as an array. |
There was a problem hiding this comment.
All of the above is either client or server params. There is no need for them in globals.
There was a problem hiding this comment.
This is a breaking change.
| # | ||
| # @param manage_package | ||
| # wgether this module willm manage the mongoDB server package |
There was a problem hiding this comment.
I removed this because I couldn't make any sense out of it.
It only controlled package naming.
manifests/globals.pp
Outdated
| @@ -159,6 +84,9 @@ | |||
| use_enterprise_repo => $use_enterprise_repo, | |||
| repo_location => $repo_location, | |||
| proxy => $repo_proxy, | |||
| proxy_username => $proxy_username, | |||
| proxy_password => $proxy_password, | |||
| aptkey_options => $aptkey_options, | |||
There was a problem hiding this comment.
Added some missing params to the repo class.
|
@stevenpost do you have any feedback on this one? |
There was a problem hiding this comment.
Perhaps managing the repository can also be completely outside of the globals class?
While most users will need to add it explicitly, it seems the cleaner option to be, as it would allow us to get rid of this globals class. It will also make it more explicit to those users that they are using an additional repository, instead of having that magically enabled and they reading the docs as they run into issues with things like a proxy.
| # @param bind_ip | ||
| # This setting can be used to configure MonogDB process to bind to and listen for connections from applications on this address. | ||
| # If not specified, the module will use the default for your OS distro. | ||
| # Note: This value should be passed as an array. |
There was a problem hiding this comment.
This is a breaking change.
I do not think leaving the repo management loosely coupled like that is a good idea. As I mentioned in the roadmap issue the only option to globals here is to add a mongodb class on top that manages repo, server and client. |
Fair enough, what you propose is kind of how the elasticsearch module does it AFAIK, see https://forge.puppet.com/modules/puppet/elasticsearch/readme for details. By default the repo is managed, but this can be overridden. |
Yes, something like that with params to manage the different components. |
|
@stevenpost and @witjoh is this ok to merge? I'll fix #714 when it's merged, and then start on a draft for the mogodb class. |
h-haaks
force-pushed
the
move-params-to-hiera
branch
from
625aa83
to
3790180
Compare
h-haaks
force-pushed
the
move-params-to-hiera
branch
from
3790180
to
7ce43cf
Compare
Pull Request (PR) description
Removing params classes and setting os specific param values in hiera data.
Aligning defaults with the supported MongoDB version.
Breaking:
Removed lots of unnecessary global params
Debian family; mongod logfile name changed from mongodb.log to mongod.log