Merge pull request #311 from mattkirby/pooler_137

(POOLER-137) Support integer environment variables

CHANGELOG.md

@@ -12,6 +12,8 @@ If you're looking for changes from before this, refer to the project's
 git logs & PR history.
 # [Unreleased](https://github.com/puppetlabs/vmpooler/compare/0.3.0...master)
 
+### Fixed
+ - Improve support for configuration via environment variables (POOLER-137)
 
 # [0.3.0](https://github.com/puppetlabs/vmpooler/compare/0.2.2...0.3.0)
 

Gemfile

@@ -26,6 +26,7 @@ group :test do
   gem 'rspec', '>= 3.2'
   gem 'simplecov', '>= 0.11.2'
   gem 'yarjuf', '>= 2.0'
+  gem 'climate_control', '>= 0.2.0'
   # Rubocop would be ok jruby but for now we only use it on
   # MRI or Windows platforms
   gem "rubocop", :platforms => [:ruby, :x64_mingw]

docs/configuration.md

@@ -19,7 +19,7 @@ Provide the entire configuration as a blob of yaml. Individual parameters passed
 
 Path to a the file to use when loading the vmpooler configuration. This is only evaluated if `VMPOOLER_CONFIG` has not been specified.
 
-### DOMAIN\_NAME
+### DOMAIN
 
 If set, returns a top-level 'domain' JSON key in POST requests
 (optional)
@@ -103,7 +103,7 @@ Same as `vm_lifetime`, but applied if a valid authentication token is
 included during the request.
 (required)
 
-### VM\_PREFIX
+### PREFIX
 
 If set, prefixes all created VMs with this string. This should include a separator.
 (optional; default: '')

lib/vmpooler.rb

@@ -47,21 +47,21 @@ module Vmpooler
     end
 
     # Set some configuration defaults
-    parsed_config[:config]['task_limit'] = ENV['TASK_LIMIT'] || parsed_config[:config]['task_limit'] || 10
+    parsed_config[:config]['task_limit'] = string_to_int(ENV['TASK_LIMIT']) || parsed_config[:config]['task_limit'] || 10
-    parsed_config[:config]['migration_limit'] = ENV['MIGRATION_LIMIT'] if ENV['MIGRATION_LIMIT']
+    parsed_config[:config]['migration_limit'] = string_to_int(ENV['MIGRATION_LIMIT']) if ENV['MIGRATION_LIMIT']
-    parsed_config[:config]['vm_checktime'] = ENV['VM_CHECKTIME'] || parsed_config[:config]['vm_checktime'] || 1
+    parsed_config[:config]['vm_checktime'] = string_to_int(ENV['VM_CHECKTIME']) || parsed_config[:config]['vm_checktime'] || 1
-    parsed_config[:config]['vm_lifetime'] = ENV['VM_LIFETIME'] || parsed_config[:config]['vm_lifetime']  || 24
+    parsed_config[:config]['vm_lifetime'] = string_to_int(ENV['VM_LIFETIME']) || parsed_config[:config]['vm_lifetime']  || 24
-    parsed_config[:config]['prefix'] = ENV['VM_PREFIX'] || parsed_config[:config]['prefix'] || ''
+    parsed_config[:config]['prefix'] = ENV['PREFIX'] || parsed_config[:config]['prefix'] || ''
 
     parsed_config[:config]['logfile'] = ENV['LOGFILE'] if ENV['LOGFILE']
 
     parsed_config[:config]['site_name'] = ENV['SITE_NAME'] if ENV['SITE_NAME']
-    parsed_config[:config]['domain'] = ENV['DOMAIN_NAME'] if ENV['DOMAIN_NAME']
+    parsed_config[:config]['domain'] = ENV['DOMAIN'] if ENV['DOMAIN']
     parsed_config[:config]['clone_target'] = ENV['CLONE_TARGET'] if ENV['CLONE_TARGET']
-    parsed_config[:config]['timeout'] = ENV['TIMEOUT'] if ENV['TIMEOUT']
+    parsed_config[:config]['timeout'] = string_to_int(ENV['TIMEOUT']) if ENV['TIMEOUT']
-    parsed_config[:config]['vm_lifetime_auth'] = ENV['VM_LIFETIME_AUTH'] if ENV['VM_LIFETIME_AUTH']
+    parsed_config[:config]['vm_lifetime_auth'] = string_to_int(ENV['VM_LIFETIME_AUTH']) if ENV['VM_LIFETIME_AUTH']
-    parsed_config[:config]['max_tries'] = ENV['MAX_TRIES'] if ENV['MAX_TRIES']
+    parsed_config[:config]['max_tries'] = string_to_int(ENV['MAX_TRIES']) if ENV['MAX_TRIES']
-    parsed_config[:config]['retry_factor'] = ENV['RETRY_FACTOR'] if ENV['RETRY_FACTOR']
+    parsed_config[:config]['retry_factor'] = string_to_int(ENV['RETRY_FACTOR']) if ENV['RETRY_FACTOR']
     parsed_config[:config]['create_folders'] = ENV['CREATE_FOLDERS'] if ENV['CREATE_FOLDERS']
     parsed_config[:config]['create_template_delta_disks'] = ENV['CREATE_TEMPLATE_DELTA_DISKS'] if ENV['CREATE_TEMPLATE_DELTA_DISKS']
     parsed_config[:config]['experimental_features'] = ENV['EXPERIMENTAL_FEATURES'] if ENV['EXPERIMENTAL_FEATURES']
@@ -70,26 +70,26 @@ module Vmpooler
 
     parsed_config[:redis] = parsed_config[:redis] || {}
     parsed_config[:redis]['server'] = ENV['REDIS_SERVER'] || parsed_config[:redis]['server'] || 'localhost'
-    parsed_config[:redis]['port'] = ENV['REDIS_PORT'] if ENV['REDIS_PORT']
+    parsed_config[:redis]['port'] = string_to_int(ENV['REDIS_PORT']) if ENV['REDIS_PORT']
     parsed_config[:redis]['password'] = ENV['REDIS_PASSWORD'] if ENV['REDIS_PASSWORD']
-    parsed_config[:redis]['data_ttl'] = ENV['REDIS_DATA_TTL'] || parsed_config[:redis]['data_ttl'] || 168
+    parsed_config[:redis]['data_ttl'] = string_to_int(ENV['REDIS_DATA_TTL']) || parsed_config[:redis]['data_ttl'] || 168
 
     parsed_config[:statsd] = parsed_config[:statsd] || {} if ENV['STATSD_SERVER']
     parsed_config[:statsd]['server'] = ENV['STATSD_SERVER'] if ENV['STATSD_SERVER']
     parsed_config[:statsd]['prefix'] = ENV['STATSD_PREFIX'] if ENV['STATSD_PREFIX']
-    parsed_config[:statsd]['port'] = ENV['STATSD_PORT'] if ENV['STATSD_PORT']
+    parsed_config[:statsd]['port'] = string_to_int(ENV['STATSD_PORT']) if ENV['STATSD_PORT']
 
     parsed_config[:graphite] = parsed_config[:graphite] || {} if ENV['GRAPHITE_SERVER']
     parsed_config[:graphite]['server'] = ENV['GRAPHITE_SERVER'] if ENV['GRAPHITE_SERVER']
     parsed_config[:graphite]['prefix'] = ENV['GRAPHITE_PREFIX'] if ENV['GRAPHITE_PREFIX']
-    parsed_config[:graphite]['port'] = ENV['GRAPHITE_PORT'] if ENV['GRAPHITE_PORT']
+    parsed_config[:graphite]['port'] = string_to_int(ENV['GRAPHITE_PORT']) if ENV['GRAPHITE_PORT']
 
     parsed_config[:auth] = parsed_config[:auth] || {} if ENV['AUTH_PROVIDER']
     if parsed_config.has_key? :auth
       parsed_config[:auth]['provider'] = ENV['AUTH_PROVIDER'] if ENV['AUTH_PROVIDER']
       parsed_config[:auth][:ldap] = parsed_config[:auth][:ldap] || {} if parsed_config[:auth]['provider'] == 'ldap'
       parsed_config[:auth][:ldap]['host'] = ENV['LDAP_HOST'] if ENV['LDAP_HOST']
-      parsed_config[:auth][:ldap]['port'] = ENV['LDAP_PORT'] if ENV['LDAP_PORT']
+      parsed_config[:auth][:ldap]['port'] = string_to_int(ENV['LDAP_PORT']) if ENV['LDAP_PORT']
       parsed_config[:auth][:ldap]['base'] = ENV['LDAP_BASE'] if ENV['LDAP_BASE']
       parsed_config[:auth][:ldap]['user_object'] = ENV['LDAP_USER_OBJECT'] if ENV['LDAP_USER_OBJECT']
     end
@@ -175,4 +175,11 @@ module Vmpooler
     end
     pools_hash
   end
+
+  def self.string_to_int(s)
+    # Returns a integer if input is a string
+    return if s.nil?
+    return unless s =~ /\d/
+    return Integer(s)
+  end
 end

spec/unit/env_config.rb

@@ -0,0 +1,222 @@
+require 'vmpooler'
+require 'climate_control'
+require 'mock_redis'
+
+describe 'Vmpooler' do
+  describe 'config' do
+
+    describe 'environment variables' do
+
+      test_integer = '5'
+      test_string = 'test_string'
+      test_bool = 'true'
+      test_cases = [
+        ['migration_limit', test_integer, nil],
+        ['task_limit', test_integer, 10],
+        ['vm_checktime', test_integer, 1],
+        ['vm_lifetime', test_integer, 24],
+        ['timeout', test_integer, nil],
+        ['vm_lifetime_auth', test_integer, nil],
+        ['max_tries', test_integer, nil],
+        ['retry_factor', test_integer, nil],
+        ['prefix', test_string, ""],
+        ['logfile', test_string, nil],
+        ['site_name', test_string, nil],
+        ['domain', test_string, nil],
+        ['clone_target', test_string, nil],
+        ['create_folders', test_bool, nil],
+        ['create_template_delta_disks', test_bool, nil],
+        ['experimental_features', test_bool, nil],
+        ['purge_unconfigured_folders', test_bool, nil],
+        ['usage_stats', test_bool, nil],
+      ]
+
+      test_cases.each do |key, value, default|
+        it "should set a value for #{key}" do
+          with_modified_env "#{key.upcase}": value do
+            test_value = value
+            test_value = Integer(value) if value =~ /\d/
+            config = Vmpooler.config
+
+            expect(config[:config][key]).to eq(test_value)
+          end
+        end
+
+        it "should set a default value for each #{key}" do
+          config = Vmpooler.config
+
+          expect(config[:config][key]).to eq(default)
+        end
+
+        if value =~ /\d/
+          it "should not set bad_data as a value for #{key}" do
+            with_modified_env "#{key.upcase}": 'bad_data' do
+              config = Vmpooler.config
+
+              expect(config[:config][key]).to eq(default)
+            end
+          end
+        end
+      end
+    end
+
+    describe 'redis environment variables' do
+      let(:redis) { MockRedis.new }
+
+      test_cases = [
+        ['server', 'redis', 'localhost'],
+        ['port', '4567', nil],
+        ['password', 'testpass', nil],
+        ['data_ttl', '500', 168],
+      ]
+
+      test_cases.each do |key, value, default|
+        it "should set a value for #{key}" do
+          allow(Vmpooler).to receive(:new_redis).and_return(redis)
+          with_modified_env "REDIS_#{key.upcase}": value do
+            test_value = value
+            test_value = Integer(value) if value =~ /\d/
+            config = Vmpooler.config
+
+            expect(config[:redis][key]).to eq(test_value)
+          end
+        end
+
+        it "should set a default value for each #{key}" do
+          config = Vmpooler.config
+
+          expect(config[:redis][key]).to eq(default)
+        end
+
+        if value =~ /\d/
+          it "should not set bad_data as a value for #{key}" do
+            with_modified_env "#{key.upcase}": 'bad_data' do
+              config = Vmpooler.config
+
+              expect(config[:redis][key]).to eq(default)
+            end
+          end
+        end
+      end
+    end
+
+    describe 'statsd environment variables' do
+
+      test_cases = [
+        ['prefix', 'vmpooler', nil],
+        ['port', '4567', nil],
+      ]
+
+      test_cases.each do |key, value, default|
+        it "should set a value for #{key}" do
+          with_modified_env STATSD_SERVER: 'test', "STATSD_#{key.upcase}": value do
+            test_value = value
+            test_value = Integer(value) if value =~ /\d/
+            config = Vmpooler.config
+
+            expect(config[:statsd][key]).to eq(test_value) end
+        end
+
+        it "should set a default value for each #{key}" do
+          with_modified_env STATSD_SERVER: 'test' do
+            config = Vmpooler.config
+
+            expect(config[:statsd][key]).to eq(default)
+          end
+        end
+
+        if value =~ /\d/
+          it "should not set bad_data as a value for #{key}" do
+            with_modified_env STATSD_SERVER: 'test', "STATSD_#{key.upcase}": 'bad_data' do
+              config = Vmpooler.config
+
+              expect(config[:statsd][key]).to eq(default)
+            end
+          end
+        end
+      end
+    end
+
+    describe 'graphite environment variables' do
+
+      test_cases = [
+        ['prefix', 'vmpooler', nil],
+        ['port', '4567', nil],
+      ]
+
+      test_cases.each do |key, value, default|
+        it "should set a value for #{key}" do
+          with_modified_env GRAPHITE_SERVER: 'test', "GRAPHITE_#{key.upcase}": value do
+            test_value = value
+            test_value = Integer(value) if value =~ /\d/
+            config = Vmpooler.config
+
+            expect(config[:graphite][key]).to eq(test_value)
+          end
+        end
+
+        it "should set a default value for each #{key}" do
+          with_modified_env GRAPHITE_SERVER: 'test' do
+            config = Vmpooler.config
+
+            expect(config[:graphite][key]).to eq(default)
+          end
+        end
+
+        if value =~ /\d/
+          it "should not set bad_data as a value for #{key}" do
+            with_modified_env GRAPHITE_SERVER: 'test', "GRAPHITE_#{key.upcase}": 'bad_data' do
+              config = Vmpooler.config
+
+              expect(config[:graphite][key]).to eq(default)
+            end
+          end
+        end
+      end
+    end
+
+    describe 'ldap environment variables' do
+
+      test_cases = [
+        ['host', 'test', nil],
+        ['port', '4567', nil],
+        ['base', 'dc=example,dc=com', nil],
+        ['user_object', 'uid', nil],
+      ]
+
+      test_cases.each do |key, value, default|
+        it "should set a value for #{key}" do
+          with_modified_env AUTH_PROVIDER: 'ldap', "LDAP_#{key.upcase}": value do
+            test_value = value
+            test_value = Integer(value) if value =~ /\d/
+            config = Vmpooler.config
+
+            expect(config[:auth][:ldap][key]).to eq(test_value)
+          end
+        end
+
+        it "should set a default value for each #{key}" do
+          with_modified_env AUTH_PROVIDER: 'ldap' do
+            config = Vmpooler.config
+
+            expect(config[:auth][:ldap][key]).to eq(default)
+          end
+        end
+
+        if value =~ /\d/
+          it "should not set bad_data as a value for #{key}" do
+            with_modified_env AUTH_PROVIDER: 'ldap', "LDAP_#{key.upcase}": 'bad_data' do
+              config = Vmpooler.config
+
+              expect(config[:auth][:ldap][key]).to eq(default)
+            end
+          end
+        end
+      end
+    end
+  end
+
+  def with_modified_env(options, &block)
+    ClimateControl.modify(options, &block)
+  end
+end