From e0ed1ab435b5204f8c7b3ce464ea6f2f5a37ec06 Mon Sep 17 00:00:00 2001
From: Alexander Fisher <alex@linfratech.co.uk>
Date: Mon, 24 Oct 2016 16:35:29 +0100
Subject: [PATCH] Add `timeout` parameter to migration defined types

Expose the `Exec` resource's `timeout` parameter in the flyway and
liquibase migration defined types.  This is better than setting a
`Resource default statement` for Execs where the `Area of effect` can be
problematic. See
https://docs.puppet.com/puppet/latest/reference/lang_defaults.html#area-of-effect

Tests, (and a few minor lint fixups) included.

Signed-off-by: Alexander Fisher <alex@linfratech.co.uk>
---
 Gemfile                                  |  2 --
 manifests/flyway.pp                      |  6 +++---
 manifests/flyway_migration.pp            | 26 +++++++++++++++---------
 manifests/liquibase.pp                   | 12 +++++------
 manifests/liquibase_migration.pp         | 22 ++++++++++++--------
 spec/classes/flyway_spec.rb              |  4 ++--
 spec/classes/liquibase_spec.rb           |  4 ++--
 spec/defines/flyway_migration_spec.rb    | 19 +++++++++++++++++
 spec/defines/liquibase_migration_spec.rb | 21 ++++++++++++++++++-
 9 files changed, 82 insertions(+), 34 deletions(-)

diff --git a/Gemfile b/Gemfile
index e960f7c..b7bdc1a 100644
--- a/Gemfile
+++ b/Gemfile
@@ -6,8 +6,6 @@ group :development, :test do
   gem 'puppetlabs_spec_helper',  :require => false
   gem 'serverspec',              :require => false
   gem 'puppet-lint',             :require => false
-  gem 'beaker',                  :require => false
-  gem 'beaker-rspec',            :require => false
   gem 'pry',                     :require => false
   gem 'simplecov',               :require => false
 end
diff --git a/manifests/flyway.pp b/manifests/flyway.pp
index b28ae45..a81d289 100644
--- a/manifests/flyway.pp
+++ b/manifests/flyway.pp
@@ -34,7 +34,7 @@
     include ::java
     Class['::java'] -> Database_schema::Flyway_migration<||>
   }
-  
+
   archive { "flyway-commandline-${version}":
     ensure   => $ensure,
     url      => $real_source,
@@ -42,6 +42,6 @@
     root_dir => "flyway-${version}",
     checksum => false
   }
-  
+
   Class['database_schema::flyway'] -> Database_schema::Flyway_migration<||>
-}
\ No newline at end of file
+}
diff --git a/manifests/flyway_migration.pp b/manifests/flyway_migration.pp
index a9d35e0..227822a 100644
--- a/manifests/flyway_migration.pp
+++ b/manifests/flyway_migration.pp
@@ -21,17 +21,23 @@
 #  Version number to migrate up to (see the migrate option "target" in the flyway docs). Defaults to "latest"
 # [*placeholders*]
 #  A hash containing placeholders you want flyway to use. Each key expands to a "-placeholder.KEY='VALUE'" argument to flyway.
+# [*timeout*]
+#  The maximum time the migration should take in seconds.  This gets passed directly to the migration Exec resource. Defaults to 300.
 #
 define database_schema::flyway_migration (
   $schema_source,
   $db_username,
   $db_password,
   $jdbc_url,
-  $flyway_path         = '/opt/flyway-3.1',
-  $target_schemas      = undef,
-  $ensure              = latest,
-  $placeholders        = {},
+  $flyway_path    = '/opt/flyway-3.1',
+  $target_schemas = undef,
+  $ensure         = latest,
+  $placeholders   = {},
+  $timeout        = 300,
 ){
+  validate_integer($timeout)
+  validate_hash($placeholders)
+
   $title_hash   = sha1($title)
   $staging_path = "/tmp/flyway-migration-${title_hash}"
   file { $staging_path:
@@ -39,13 +45,12 @@
     recurse => true,
     source  => $schema_source
   }
-  
-  validate_hash($placeholders)
+
   $placeholders_str = flyway_cmd_placeholders($placeholders)
 
   $target_version = $ensure ? {latest => '', default => " -target=${ensure}"}
-  $flyway_base_command = "flyway -user='${db_username}' -password='${db_password}' -url='${jdbc_url}' ${placeholders_str} -locations='filesystem:${staging_path}'$target_version"
-  
+  $flyway_base_command = "flyway -user='${db_username}' -password='${db_password}' -url='${jdbc_url}' ${placeholders_str} -locations='filesystem:${staging_path}'${target_version}"
+
   if $target_schemas == undef {
     $flyway_command = $flyway_base_command
   }
@@ -53,12 +58,13 @@
     $joined_schemas = join($target_schemas, ',')
     $flyway_command = "${flyway_base_command} -schemas='${joined_schemas}'"
   }
-  
+
   exec { "Migration for ${title}":
     cwd     => $flyway_path,
     path    => "${flyway_path}:/usr/local/bin:/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/sbin",
     unless  => "${flyway_command} validate",
     command => "${flyway_command} migrate",
-    require => File[$staging_path]
+    timeout => $timeout,
+    require => File[$staging_path],
   }
 }
diff --git a/manifests/liquibase.pp b/manifests/liquibase.pp
index 6efe51b..088f432 100644
--- a/manifests/liquibase.pp
+++ b/manifests/liquibase.pp
@@ -30,22 +30,22 @@
     undef   => "http://repo1.maven.org/maven2/org/liquibase/liquibase-core/${version}/liquibase-core-${version}-bin.tar.gz",
     default => $source
   }
-  
+
   $dir_ensure = $ensure ? {
     absent  => absent,
     default => directory
   }
-  
+
   if $ensure == present and $manage_java {
     include ::java
     Class['::java'] -> Database_schema::Liquibase_migration<||>
   }
-  
+
   file { "${target_dir}/liquibase":
     ensure => $dir_ensure,
     force  => true
   }
-  
+
   archive { "liquibase-core-${version}-bin":
     ensure   => $ensure,
     url      => $real_source,
@@ -53,6 +53,6 @@
     root_dir => 'liquibase',
     checksum => false
   }
-  
+
   Class['database_schema::liquibase'] -> Database_schema::Liquibase_migration<||>
-}
\ No newline at end of file
+}
diff --git a/manifests/liquibase_migration.pp b/manifests/liquibase_migration.pp
index 4d6805f..3f71fb8 100644
--- a/manifests/liquibase_migration.pp
+++ b/manifests/liquibase_migration.pp
@@ -19,16 +19,21 @@
 #  Default schema to apply migrations to.
 # [*ensure*]
 #  Only supported value is "latest".
+# [*timeout*]
+#  The maximum time the migration should take in seconds.  This gets passed directly to the migration Exec resource. Defaults to 300.
 #
 define database_schema::liquibase_migration (
   $changelog_source,
   $db_username,
   $db_password,
   $jdbc_url,
-  $liquibase_path      = '/opt/liquibase',
-  $default_schema      = undef,
-  $ensure              = latest
+  $liquibase_path = '/opt/liquibase',
+  $default_schema = undef,
+  $ensure         = latest,
+  $timeout        = 300,
 ){
+  validate_integer($timeout)
+
   $title_hash   = sha1(title)
   $changelog_basename = inline_template('<%= File.basename(@changelog_source) %>')
   $staging_path = "/tmp/liquibase-migration-${title_hash}"
@@ -40,21 +45,22 @@
     ensure => present,
     source => $changelog_source
   }
-  
+
   $liquibase_base_command = "liquibase --username='${db_username}' --password='${db_password}' --url='${jdbc_url}' --changeLogFile='${changelog_path}'"
-  
+
   if $default_schema == undef {
     $flyway_command = $liquibase_base_command
   }
   else {
     $flyway_command = "${liquibase_base_command} --defaultSchemaNAme='${default_schema}'"
   }
-  
+
   exec { "Migration for ${title}":
     cwd     => $liquibase_path,
     path    => "${liquibase_path}:/usr/local/bin:/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/sbin",
     onlyif  => "${flyway_command} status | grep 'change sets have not been applied'",
     command => "${flyway_command} update",
-    require => File[$changelog_path]
+    timeout => $timeout,
+    require => File[$changelog_path],
   }
-}
\ No newline at end of file
+}
diff --git a/spec/classes/flyway_spec.rb b/spec/classes/flyway_spec.rb
index e4c7cfb..c071d40 100644
--- a/spec/classes/flyway_spec.rb
+++ b/spec/classes/flyway_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
 describe 'database_schema::flyway' do
-  let(:facts){{:operatingsystem => 'RedHat', :osfamily => 'RedHat'}}
+  let(:facts){{:operatingsystem => 'RedHat', :osfamily => 'RedHat', :operatingsystemrelease => '7'}}
   include_examples :compile
-end
\ No newline at end of file
+end
diff --git a/spec/classes/liquibase_spec.rb b/spec/classes/liquibase_spec.rb
index f942fe9..280c2a2 100644
--- a/spec/classes/liquibase_spec.rb
+++ b/spec/classes/liquibase_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
 describe 'database_schema::liquibase' do
-  let(:facts){{:operatingsystem => 'RedHat', :osfamily => 'RedHat'}}
+  let(:facts){{:operatingsystem => 'RedHat', :osfamily => 'RedHat', :operatingsystemrelease => '7'}}
   include_examples :compile
-end
\ No newline at end of file
+end
diff --git a/spec/defines/flyway_migration_spec.rb b/spec/defines/flyway_migration_spec.rb
index 68937e9..80ecb34 100644
--- a/spec/defines/flyway_migration_spec.rb
+++ b/spec/defines/flyway_migration_spec.rb
@@ -30,4 +30,23 @@
       end
     end
   end
+  describe 'timeout' do
+    context 'when not specified' do
+      it 'exec has timeout set to 300 seconds' do
+        is_expected.to contain_exec('Migration for example db').with_timeout(300)
+      end
+    end
+    context 'when specified' do
+      let(:params){{
+        :schema_source => '/some/path',
+        :db_username   => 'user',
+        :db_password   => 'supersecret',
+        :jdbc_url      => 'jdbc:h2:test',
+        :timeout       => 3600
+      }}
+      it 'passes timeout to exec' do
+        is_expected.to contain_exec('Migration for example db').with_timeout(3600)
+      end
+    end
+  end
 end
diff --git a/spec/defines/liquibase_migration_spec.rb b/spec/defines/liquibase_migration_spec.rb
index 86ac991..51672c9 100644
--- a/spec/defines/liquibase_migration_spec.rb
+++ b/spec/defines/liquibase_migration_spec.rb
@@ -9,4 +9,23 @@
     :jdbc_url         => 'jdbc:h2:test'
   }}
   include_examples :compile
-end
\ No newline at end of file
+  describe 'timeout' do
+    context 'when not specified' do
+      it 'exec has timeout set to 300 seconds' do
+        is_expected.to contain_exec('Migration for example db').with_timeout(300)
+      end
+    end
+    context 'when specified' do
+      let(:params){{
+        :changelog_source => '/some/path',
+        :db_username      => 'user',
+        :db_password      => 'supersecret',
+        :jdbc_url         => 'jdbc:h2:test',
+        :timeout          => 3600
+      }}
+      it 'passes timeout to exec' do
+        is_expected.to contain_exec('Migration for example db').with_timeout(3600)
+      end
+    end
+  end
+end