From 19a613c25a7187324a442d0003c2960e4754ad7b Mon Sep 17 00:00:00 2001
From: Adrian <sensei.loafage@gmail.com>
Date: Thu, 30 Sep 2021 12:56:12 -0700
Subject: [PATCH 1/2] (SUP-2557) Ensure backup class is not included

Prior to this commit, by default the pe_databases::backup class would be
included but not do anything.  That was because the logic for the class
parameter was correct

disable_maintenance => ! $manage_database_backups

But the logic to include the class was not

if defined('$manage_database_backups')

This commit fixes this logic and adds spec tests to cover it.  I also
moved the test for checking the cron resources from an acceptance test
to spec.
---
 manifests/init.pp              |  4 +++-
 spec/acceptance/backup_spec.rb |  6 ------
 spec/classes/backup_spec.rb    | 12 ++++++++++++
 spec/classes/init_spec.rb      | 17 +++++++++--------
 4 files changed, 24 insertions(+), 15 deletions(-)
 create mode 100644 spec/classes/backup_spec.rb

diff --git a/manifests/init.pp b/manifests/init.pp
index 8b84305..d7d7d32 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -43,7 +43,9 @@
           include pe_databases::postgresql_settings::table_settings
         }
       }
-      if defined('$manage_database_backups') {
+      # Because this parameter is a value of undef with a data type of Undef,
+      # We can the NotUndef type to determine if the value has been set
+      if $manage_database_backups =~ NotUndef {
         class { 'pe_databases::backup':
           disable_maintenance => ! $manage_database_backups,
         }
diff --git a/spec/acceptance/backup_spec.rb b/spec/acceptance/backup_spec.rb
index 276bc90..b164a73 100644
--- a/spec/acceptance/backup_spec.rb
+++ b/spec/acceptance/backup_spec.rb
@@ -11,10 +11,4 @@ class { 'pe_databases':
     # Run it twice and test for idempotency
     idempotent_apply(pp)
   end
-  it 'checks if backup cron jobs are up' do
-    run_shell('crontab -l -u pe-postgres') do |r|
-      expect(r.stdout).to match(%r{pe-activity, pe-classifier, pe-inventory, pe-orchestrator, pe-postgres, pe-rbac})
-      expect(r.stdout).to match(%r{pe-puppetdb})
-    end
-  end
 end
diff --git a/spec/classes/backup_spec.rb b/spec/classes/backup_spec.rb
new file mode 100644
index 0000000..3239aa3
--- /dev/null
+++ b/spec/classes/backup_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+
+describe 'pe_databases::backup' do
+  context 'when backing up tables' do
+    let (:pre_condition) {'include pe_databases'}
+    it {
+      # I have no idea how this works, but these are the resources we should end up with
+      is_expected.to contain_cron('puppet_enterprise_database_backup_[pe-activity, pe-classifier, pe-inventory, pe-orchestrator, pe-postgres, pe-rbac]')
+      is_expected.to contain_cron('puppet_enterprise_database_backup_[pe-puppetdb]')
+    }
+  end
+end
diff --git a/spec/classes/init_spec.rb b/spec/classes/init_spec.rb
index f00799f..2e00a65 100644
--- a/spec/classes/init_spec.rb
+++ b/spec/classes/init_spec.rb
@@ -1,14 +1,6 @@
 require 'spec_helper'
 
 describe 'pe_databases' do
-  let(:params) do
-    {
-      manage_database_backups:  false,
-      manage_postgresql_settings: false,
-      manage_table_settings:  false,
-    }
-  end
-
   on_supported_os.each do |os, os_facts|
     context "on #{os}" do
       let(:facts) { os_facts }
@@ -32,4 +24,13 @@
 
     it { is_expected.to contain_notify('pe_databases_systemd_warn') }
   end
+
+  context 'backups are not included by default' do
+    it { is_expected.to_not contain_class('pe_databases::backup') }
+  end
+
+  context 'backups are included if configured' do
+    let(:params) { {manage_database_backups: true} }
+    it { is_expected.to contain_class('pe_databases::backup') }
+  end
 end

From c0959d92ef2f13e582a036da1dd1f4cba64edc98 Mon Sep 17 00:00:00 2001
From: Adrian <sensei.loafage@gmail.com>
Date: Thu, 30 Sep 2021 14:29:57 -0700
Subject: [PATCH 2/2] linting

---
 spec/classes/backup_spec.rb | 3 ++-
 spec/classes/init_spec.rb   | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/spec/classes/backup_spec.rb b/spec/classes/backup_spec.rb
index 3239aa3..c43c969 100644
--- a/spec/classes/backup_spec.rb
+++ b/spec/classes/backup_spec.rb
@@ -2,7 +2,8 @@
 
 describe 'pe_databases::backup' do
   context 'when backing up tables' do
-    let (:pre_condition) {'include pe_databases'}
+    let(:pre_condition) { 'include pe_databases' }
+
     it {
       # I have no idea how this works, but these are the resources we should end up with
       is_expected.to contain_cron('puppet_enterprise_database_backup_[pe-activity, pe-classifier, pe-inventory, pe-orchestrator, pe-postgres, pe-rbac]')
diff --git a/spec/classes/init_spec.rb b/spec/classes/init_spec.rb
index 2e00a65..a8308cf 100644
--- a/spec/classes/init_spec.rb
+++ b/spec/classes/init_spec.rb
@@ -26,11 +26,12 @@
   end
 
   context 'backups are not included by default' do
-    it { is_expected.to_not contain_class('pe_databases::backup') }
+    it { is_expected.not_to contain_class('pe_databases::backup') }
   end
 
   context 'backups are included if configured' do
-    let(:params) { {manage_database_backups: true} }
+    let(:params) { { manage_database_backups: true } }
+
     it { is_expected.to contain_class('pe_databases::backup') }
   end
 end