-
-
Notifications
You must be signed in to change notification settings - Fork 451
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
Add new classes for installing Ops Manager on a target machine. #500
Conversation
@claycogg Thanks! I haven't got time right this minute for a proper review, but at first glance, looks really good. |
Hi @claycogg, can you take a look at the used email address in the commi? It isn't associated with your github account. |
@@ -0,0 +1,42 @@ | |||
# PRIVATE CLASS: do not call directly | |||
class mongodb::opsmanager::install { | |||
$package_ensure = $mongodb::opsmanager::package_ensure |
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.
can you add assert_private()
from stdlib as first statement after the class header?
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.
If I assert_private() do I then need to remove the ops_manager_install_spec.rb file? I am likely mistaken, but won't it prevent the tests from being able to run?
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.
The tests need to be moved to a file that tests the mongodb::opsmanager
class.
manifests/opsmanager/service.pp
Outdated
@@ -0,0 +1,27 @@ | |||
# PRIVATE CLASS: do not call directly | |||
class mongodb::opsmanager::service { | |||
$ensure = $mongodb::opsmanager::service_ensure |
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.
same here, please add assert_private()
Hi @claycogg, thanks for this PR! The code looks pretty good. I made a few inline comments for really little things. |
mms.mail.hostname=<%=@smtp_server_hostname%> | ||
mms.mail.port=<%=@smtp_server_port%> | ||
|
||
<%unless @user_svc_class.nil?%> |
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.
or just <%- if @user_svc_class -%>?
The only things in puppet that are false, are false
and undef
, so unless @foo.nil?
style is only needed for Optional[Booleans]
s IIRC
# running with replication or authentication, please refer to the | ||
# documentation at http://mms.mongodb.com/help-hosted. | ||
# #################################### | ||
mongo.mongoUri=<%=@mongo_uri%> |
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.
Stylewise, I think some extra spaces inside the erb tags would be nice.
manifests/opsmanager.pp
Outdated
|
||
group { $group: | ||
ensure => 'present', | ||
name => $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.
Is name
required here? It's the namevar of a group resource isn't it?
manifests/opsmanager.pp
Outdated
name => $group, | ||
} | ||
|
||
-> user { $user: |
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.
Stylewise, I'm not too keen on declaring relationships this way (especially with the extra blank line between resources). Are these users not created when the packages are installed anyway? Is the user supposed to be a member of the group? Maybe a comment would help here.
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.
By default the package creates a mongodb-mms user I believe. I have no issue with using that user and not creating a new one.
manifests/opsmanager.pp
Outdated
} | ||
|
||
if ($mongo_uri == 'mongodb://127.0.0.1:27017'){ | ||
contain mongodb::server |
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 looks a bit funky. Do you really need/want to contain
the server class here? Possibly just include
it instead? I can envisage circular dependency issues otherwise. This is the sort of pattern I might conceivably use...
class role mongodb_server {
include profile::mongodb # a profile that contains mongodb::server
include profile::mongodb_opsmanager # a profile that contains mongodb::opsmanager
Class['profile::mongodb'] -> Class['profile::mongodb_opsmanager']
}
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.
Include seems correct ya. I am slightly unfamiliar with contain and used it incorrectly here. I can probably remove this section entirely as it is only there to make sure Ops Manager actually runs with the default settings the module provides.
manifests/opsmanager.pp
Outdated
String $email_dao_class = 'com.xgen.svc.core.dao.email.JavaEmailDao', #AWS SES: com.xgen.svc.core.dao.email.AwsEmailDao or SMTP: com.xgen.svc.core.dao.email.JavaEmailDao | ||
String $mail_transport = 'smtp', #smtp or smtps | ||
String $smtp_server_hostname = 'your-email-relay.email.com', # if email_dao_class is SMTP: Email hostname your email provider specifies. | ||
String $smtp_server_port = '25', #if email_dao_class is SMTP: Email hostname your email provider specifies. |
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.
Does this need to be a String
?
manifests/opsmanager.pp
Outdated
String $download_url = "https://downloads.mongodb.com/on-prem-mms/rpm/mongodb-mms-${version}.x86_64.rpm", | ||
String $mongo_uri = $mongodb::params::opsmanager_mongo_uri, | ||
String $url = $mongodb::params::opsmanager_url, | ||
Integer $port = $mongodb::params::opsmanager_port, |
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.
Stdlib::Port
?
manifests/opsmanager.pp
Outdated
String $service_name = $mongodb::params::opsmanager_service_name, | ||
|
||
String $version = $mongodb::params::opsmanager_version, | ||
String $download_url = "https://downloads.mongodb.com/on-prem-mms/rpm/mongodb-mms-${version}.x86_64.rpm", |
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 'optional' or in some way. As a user I'd want to be able to host the package in a local yum/pulp repo.
Maybe just a Optional[String[1]] $package_source = undef
? The README can explain that the package either has to be already available to your package manager but also give an example with the mongodb.com URL being used.
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.
The only issue is that Ops Manager isn't currently available in a bonafide repo. The rpms are just hosted in an S3 bucket so as far as I know there is no metadata file to allow yum/pulp access to all available rpms.
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.
In my environment, I’d download the rpm and manually upload in to a pulp repo (instead of configuring an upstream to sync from).
I think you’re right that it would be nice if the module did default to something that worked for most people. But for people stuck behind proxies etc. we need to offer a solution.
Making it Optional doesn’t help if you don’t default to undef
, as you can’t declare a class with a parameter set to undef
to override the default String.
Maybe an extra parameter is needed. manage_package_source
or something.
@ekohl you’re normally spot on with suggesting elegant solutions for this sort of thing. Any thoughts? ;)
manifests/opsmanager/install.pp
Outdated
|
||
package { $package_name: | ||
ensure => $my_package_ensure, | ||
name => $package_name, |
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.
name
parameter is redundant.
README.md
Outdated
@@ -625,6 +657,24 @@ Plain-text user password (will be hashed) | |||
##### `roles` | |||
Array with user roles. Default: ['dbAdmin'] | |||
|
|||
##### `opsmanager_url` | |||
URL where Ops Manager can be accessed. Default: $facts['networking']['fqdn'] |
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.
To me, URL implies more than just hostname.
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.
Looking pretty good. I've left some inline comments with my observations.
Thank you for all the feedback. I will be updating this morning. Should I squash my changes and force push or just have the PR include 2 commits? |
Personally I'm always a fan of merging squashed commits but in Vox Pupuli this isn't common practice and we just merge whatever was submitted. During review it can also be a benefit to add additional commits because it makes viewing changes easier for the reviewer. So whatever you prefer. |
@ekohl It depends. If there are a few well organised commits, we often merge as is. If there are tens then we'd normally asked for them to be squashed. @claycogg |
588b914
to
ef6a0fc
Compare
manifests/opsmanager/install.pp
Outdated
|
||
case $facts['os']['family'] { | ||
'RedHat': { | ||
$my_provider = 'rpm' |
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.
Maybe wrap this with if versioncmp(fact('puppetversion'),'5.4.0') < 0
?
This would make it more obvious to rip out later when eventually we drop support for earlier puppet versions.
P.S. Check my syntax is actually correct!
manifests/opsmanager/install.pp
Outdated
'RedHat': { | ||
$my_provider = 'rpm' | ||
} | ||
default: { |
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.
Can we not put a 'Debian'
case in here? I've just double checked, and package provider
definitely defaults to apt
on osfamily == 'Debian'
. Unlike yum
(the default for RedHat), the apt
provider doesn't have support for source
and you need to use dpkg
instead.
I'm working on a PR to move away from a shared |
@ekohl In that case, should I remove any sort of params.pp and move all of the opsmanager params into globals.pp? You had a recommendation to do a separate params.pp for opsmanager, should it also have its own globals.pp? |
Hi people. I'm fine with merging this as is. This PR already has quite a few lines and many comments. I think everything else should be handled in further PRs. @alexjfisher @ekohl what do you think? |
@alexjfisher @ekohl Sorry to bother you both. Any chance you'll be able to provide another round of feedback this week? I'm more than happy to do more PRs going forward as @bastelfreak recommended to polish the Ops Manager part of the module. |
manifests/opsmanager/config.pp
Outdated
$user = $mongodb::opsmanager::user | ||
$group = $mongodb::opsmanager::group | ||
|
||
file { '/opt/mongodb/mms/conf/conf-mms.properties': |
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.
Could you correct the alignment?
manifests/globals.pp
Outdated
@@ -30,6 +30,21 @@ | |||
$pidfilepath = undef, | |||
$pidfilemode = undef, | |||
$manage_pidfile = undef, | |||
|
|||
$opsmanager_user = 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.
I'm leaning to leaving all these out. I want to do the same for other classes
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.
Do you mean the user and group undef declarations or all of the undef declarations?
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 meant all $opsmanager_*
variables from globals.pp
.
manifests/opsmanager/install.pp
Outdated
} | ||
} | ||
|
||
else { |
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.
Could you add this to line with the matching }
spec/classes/ops_manager_spec.rb
Outdated
context "on #{os}" do | ||
let(:facts) { facts } | ||
|
||
let :pre_condition do |
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 can be replaced with:
let(:params) do
{
opsmanager_url: 'http://localhost:8080'
}
end
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.
Just this, and a squash/rebase, and we're good to merge I think?
Messed up the squash somehow, fixing it now. |
Actually, not sure how to fix this. |
c44f03d
to
c7c43e2
Compare
manifests/opsmanager.pp
Outdated
String[1] $mongo_uri = $mongodb::opsmanager::params::mongo_uri, | ||
Stdlib::Httpurl $opsmanager_url = $mongodb::opsmanager::params::opsmanager_url, | ||
String[1] $client_certificate_mode = 'None', | ||
String[1] $from_email_addr = 'from@yourdomain.com', |
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.
Could you change these defaults to at least @example.com
? If users are expected to set these, we can also leave them empty.
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/opsmanager.pp
Outdated
String[1] $admin_email_addr = 'admin@yourdomain.com', | ||
String[1] $email_dao_class = 'com.xgen.svc.core.dao.email.JavaEmailDao', #AWS SES: com.xgen.svc.core.dao.email.AwsEmailDao or SMTP: com.xgen.svc.core.dao.email.JavaEmailDao | ||
Enum['smtp','smtps'] $mail_transport = 'smtp', #smtp or smtps | ||
String[1] $smtp_server_hostname = 'your-email-relay.email.com', # if email_dao_class is SMTP: Email hostname your email provider specifies. |
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.
smtp.example.com
perhaps? Also wondering if this should be a Stdlib::Host
type.
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. Also stdlib dependency updated.
manifests/opsmanager.pp
Outdated
String[1] $email_dao_class = 'com.xgen.svc.core.dao.email.JavaEmailDao', #AWS SES: com.xgen.svc.core.dao.email.AwsEmailDao or SMTP: com.xgen.svc.core.dao.email.JavaEmailDao | ||
Enum['smtp','smtps'] $mail_transport = 'smtp', #smtp or smtps | ||
String[1] $smtp_server_hostname = 'your-email-relay.email.com', # if email_dao_class is SMTP: Email hostname your email provider specifies. | ||
String[1] $smtp_server_port = '25', #if email_dao_class is SMTP: Email hostname your email provider specifies. |
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.
Perhaps this should be a Stdlib::Port
type?
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/opsmanager.pp
Outdated
String[1] $reply_to_email_addr = 'replyto@yourdomain.com', | ||
String[1] $admin_email_addr = 'admin@yourdomain.com', | ||
String[1] $email_dao_class = 'com.xgen.svc.core.dao.email.JavaEmailDao', #AWS SES: com.xgen.svc.core.dao.email.AwsEmailDao or SMTP: com.xgen.svc.core.dao.email.JavaEmailDao | ||
Enum['smtp','smtps'] $mail_transport = 'smtp', #smtp or smtps |
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.
The comment is redundant because the type already tells us enough.
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
c7c43e2
to
858ddd4
Compare
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.
Let's merge this and then iterate on it.
Add new classes for installing Ops Manager on a target machine.
Pull Request (PR) description
Adds classes for installing Ops Manager as well as a way to complete the initial configuration without the UI. Add 2 tests for testing that it is installed and the service starts.
This Pull Request (PR) fixes the following issues
It does not fix any existing issues but adds a feature.