Add new classes for installing Ops Manager on a target machine. #500

claycogg · 2018-10-09T19:28:46Z

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.

alexjfisher · 2018-10-09T19:38:58Z

@claycogg Thanks! I haven't got time right this minute for a proper review, but at first glance, looks really good.

bastelfreak · 2018-10-09T21:18:19Z

Hi @claycogg, can you take a look at the used email address in the commi? It isn't associated with your github account.

manifests/opsmanager.pp

bastelfreak · 2018-10-09T21:20:29Z

manifests/opsmanager/install.pp

@@ -0,0 +1,42 @@
+# PRIVATE CLASS: do not call directly
+class mongodb::opsmanager::install {
+  $package_ensure = $mongodb::opsmanager::package_ensure


can you add assert_private() from stdlib as first statement after the class header?

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?

The tests need to be moved to a file that tests the mongodb::opsmanager class.

bastelfreak · 2018-10-09T21:20:46Z

manifests/opsmanager/service.pp

@@ -0,0 +1,27 @@
+# PRIVATE CLASS: do not call directly
+class mongodb::opsmanager::service {
+  $ensure           = $mongodb::opsmanager::service_ensure


same here, please add assert_private()

spec/classes/ops_manager_install_spec.rb

templates/opsmanager/conf-mms.properties.erb

bastelfreak · 2018-10-09T21:24:55Z

Hi @claycogg, thanks for this PR! The code looks pretty good. I made a few inline comments for really little things.

alexjfisher · 2018-10-09T21:40:55Z

templates/opsmanager/conf-mms.properties.erb

+mms.mail.hostname=<%=@smtp_server_hostname%>
+mms.mail.port=<%=@smtp_server_port%>
+
+<%unless @user_svc_class.nil?%>


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

alexjfisher · 2018-10-09T21:43:52Z

templates/opsmanager/conf-mms.properties.erb

+# running with replication or authentication, please refer to the
+# documentation at http://mms.mongodb.com/help-hosted.
+# ####################################
+mongo.mongoUri=<%=@mongo_uri%>


Stylewise, I think some extra spaces inside the erb tags would be nice.

alexjfisher · 2018-10-09T21:53:45Z

manifests/opsmanager.pp

+
+    group { $group:
+      ensure => 'present',
+      name   => $group,


Is name required here? It's the namevar of a group resource isn't it?

alexjfisher · 2018-10-09T22:00:28Z

manifests/opsmanager.pp

+      name   => $group,
+    }
+
+    -> user { $user:


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.

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.

alexjfisher · 2018-10-09T22:07:16Z

manifests/opsmanager.pp

+    }
+
+    if ($mongo_uri == 'mongodb://127.0.0.1:27017'){
+      contain mongodb::server


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'] }

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.

alexjfisher · 2018-10-09T22:08:26Z

manifests/opsmanager.pp

+  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.


Does this need to be a String?

alexjfisher · 2018-10-09T22:08:38Z

manifests/opsmanager.pp

+  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,


Stdlib::Port?

alexjfisher · 2018-10-09T22:18:22Z

manifests/opsmanager.pp

+  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",


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.

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.

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? ;)

alexjfisher · 2018-10-09T22:19:00Z

manifests/opsmanager/install.pp

+
+  package { $package_name:
+    ensure => $my_package_ensure,
+    name   => $package_name,


name parameter is redundant.

alexjfisher · 2018-10-09T22:22:46Z

README.md

@@ -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']


To me, URL implies more than just hostname.

alexjfisher

Looking pretty good. I've left some inline comments with my observations.

claycogg · 2018-10-10T12:48:07Z

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?

ekohl · 2018-10-10T13:05:37Z

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.

alexjfisher · 2018-10-10T13:18:30Z

@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
For now, just pushing extra commits would be fine and then maybe squash just before the PR is merged? I'll add WIP to the PR title so that nobody merges it prematurely. Feel free to remove it when you're done.

alexjfisher · 2018-10-20T16:15:54Z

manifests/opsmanager/install.pp

+
+  case $facts['os']['family'] {
+    'RedHat': {
+      $my_provider = 'rpm'


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!

alexjfisher · 2018-10-20T16:19:18Z

manifests/opsmanager/install.pp

+    'RedHat': {
+      $my_provider = 'rpm'
+    }
+    default: {


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.

ekohl · 2018-10-22T16:31:21Z

I'm working on a PR to move away from a shared params.pp and exposing all variables in globals.pp. I'd prefer not to introduce that if we're going to remove it very soon after.

claycogg · 2018-10-23T17:54:30Z

I'm working on a PR to move away from a shared params.pp and exposing all variables in globals.pp. I'd prefer not to introduce that if we're going to remove it very soon after.

@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?

bastelfreak · 2018-10-24T18:50:54Z

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?

claycogg · 2018-10-29T13:17:10Z

@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.

ekohl · 2018-10-29T13:37:31Z

manifests/opsmanager/config.pp

+  $user  = $mongodb::opsmanager::user
+  $group = $mongodb::opsmanager::group
+
+file { '/opt/mongodb/mms/conf/conf-mms.properties':


Could you correct the alignment?

ekohl · 2018-10-29T13:38:19Z

manifests/globals.pp

@@ -30,6 +30,21 @@
  $pidfilepath           = undef,
  $pidfilemode           = undef,
  $manage_pidfile        = undef,
+
+  $opsmanager_user           = undef,


I'm leaning to leaving all these out. I want to do the same for other classes

Do you mean the user and group undef declarations or all of the undef declarations?

I meant all $opsmanager_* variables from globals.pp.

ekohl · 2018-10-29T13:39:37Z

manifests/opsmanager/install.pp

+    }
+  }
+
+  else {


Could you add this to line with the matching }

spec/classes/ops_manager_install_spec.rb

ekohl · 2018-11-02T12:13:50Z

spec/classes/ops_manager_spec.rb

+    context "on #{os}" do
+      let(:facts) { facts }
+
+      let :pre_condition do


This can be replaced with:

let(:params) do { opsmanager_url: 'http://localhost:8080' } end

Just this, and a squash/rebase, and we're good to merge I think?

claycogg · 2018-11-07T14:03:22Z

Messed up the squash somehow, fixing it now.

claycogg · 2018-11-07T14:37:20Z

Messed up the squash somehow, fixing it now.

Actually, not sure how to fix this.

ekohl · 2018-11-08T11:40:44Z

manifests/opsmanager.pp

+  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',


Could you change these defaults to at least @example.com? If users are expected to set these, we can also leave them empty.

ekohl · 2018-11-08T11:42:02Z

manifests/opsmanager.pp

+  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.


smtp.example.com perhaps? Also wondering if this should be a Stdlib::Host type.

Fixed. Also stdlib dependency updated.

ekohl · 2018-11-08T11:42:17Z

manifests/opsmanager.pp

+  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.


Perhaps this should be a Stdlib::Port type?

ekohl · 2018-11-08T11:43:52Z

manifests/opsmanager.pp

+  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


The comment is redundant because the type already tells us enough.

alexjfisher · 2018-11-08T11:54:40Z

@claycogg I fixed up the rebase for you. I'm going to make the changes @ekohl would like, so we can get this merged.

ekohl

Let's merge this and then iterate on it.

Add new classes for installing Ops Manager on a target machine.

claycogg force-pushed the opsmanager branch from eef70c3 to 294800a Compare October 9, 2018 19:55

bastelfreak reviewed Oct 9, 2018

View reviewed changes

manifests/opsmanager.pp Outdated Show resolved Hide resolved

bastelfreak reviewed Oct 9, 2018

View reviewed changes

manifests/opsmanager.pp Outdated Show resolved Hide resolved

bastelfreak reviewed Oct 9, 2018

View reviewed changes

spec/classes/ops_manager_install_spec.rb Outdated Show resolved Hide resolved

bastelfreak reviewed Oct 9, 2018

View reviewed changes

spec/classes/ops_manager_install_spec.rb Outdated Show resolved Hide resolved

bastelfreak reviewed Oct 9, 2018

View reviewed changes

templates/opsmanager/conf-mms.properties.erb Outdated Show resolved Hide resolved

alexjfisher reviewed Oct 9, 2018

View reviewed changes

alexjfisher changed the title ~~Add new classes for installing Ops Manager on a target machine.~~ WIP: Add new classes for installing Ops Manager on a target machine. Oct 10, 2018

claycogg force-pushed the opsmanager branch 4 times, most recently from 588b914 to ef6a0fc Compare October 10, 2018 20:11

alexjfisher reviewed Oct 20, 2018

View reviewed changes

claycogg changed the title ~~Add new classes for installing Ops Manager on a target machine.~~ WIP: Add new classes for installing Ops Manager on a target machine. Oct 22, 2018

claycogg force-pushed the opsmanager branch from 8e43b47 to ec36da6 Compare October 23, 2018 19:04

bastelfreak approved these changes Oct 24, 2018

View reviewed changes

claycogg changed the title ~~WIP: Add new classes for installing Ops Manager on a target machine.~~ Add new classes for installing Ops Manager on a target machine. Oct 25, 2018

ekohl reviewed Oct 29, 2018

View reviewed changes

claycogg force-pushed the opsmanager branch from 455b590 to 7bfee71 Compare November 1, 2018 18:21

bastelfreak approved these changes Nov 2, 2018

View reviewed changes

ekohl approved these changes Nov 2, 2018

View reviewed changes

alexjfisher added the needs-rebase label Nov 6, 2018

claycogg force-pushed the opsmanager branch from 7bfee71 to c44f03d Compare November 7, 2018 13:56

claycogg closed this Nov 7, 2018

claycogg reopened this Nov 7, 2018

alexjfisher force-pushed the opsmanager branch from c44f03d to c7c43e2 Compare November 8, 2018 11:30

ekohl requested changes Nov 8, 2018

View reviewed changes

ekohl removed the needs-rebase label Nov 8, 2018

Add new classes for installing Ops Manager

858ddd4

alexjfisher force-pushed the opsmanager branch from c7c43e2 to 858ddd4 Compare November 8, 2018 12:02

ekohl approved these changes Nov 8, 2018

View reviewed changes

alexjfisher merged commit a71b841 into voxpupuli:master Nov 8, 2018

alexjfisher removed the needs-work not ready to merge just yet label Nov 8, 2018

buchstabensalat pushed a commit to pecharmin/puppet-mongodb that referenced this pull request Nov 29, 2018

Merge pull request voxpupuli#500 from claycogg/opsmanager

ee7158c

Add new classes for installing Ops Manager on a target machine.

+                  }
+                }
+                else {

Add new classes for installing Ops Manager on a target machine. #500

Add new classes for installing Ops Manager on a target machine. #500

Conversation

claycogg commented Oct 9, 2018

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

alexjfisher commented Oct 9, 2018

bastelfreak commented Oct 9, 2018

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

bastelfreak commented Oct 9, 2018

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

alexjfisher left a comment

Choose a reason for hiding this comment

claycogg commented Oct 10, 2018

ekohl commented Oct 10, 2018

alexjfisher commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Oct 22, 2018

claycogg commented Oct 23, 2018

bastelfreak commented Oct 24, 2018

claycogg commented Oct 29, 2018 • 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

claycogg commented Nov 7, 2018

claycogg commented Nov 7, 2018

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

alexjfisher commented Nov 8, 2018

ekohl left a comment

Choose a reason for hiding this comment

claycogg commented Oct 29, 2018 •

edited

Loading