Merge pull request #4 from puppetlabs/fix-purge

Move vsphere specific methods out of vmpooler

Gemfile.lock

@@ -136,7 +136,7 @@ GEM
       rubocop-ast (>= 1.0.1)
       ruby-progressbar (~> 1.7)
       unicode-display_width (>= 1.4.0, < 2.0)
-    rubocop-ast (1.14.0)
+    rubocop-ast (1.15.0)
       parser (>= 3.0.1.1)
     ruby-progressbar (1.11.0)
     ruby2_keywords (0.0.5)
@@ -160,7 +160,7 @@ GEM
     thrift (0.15.0)
     tilt (2.0.10)
     unicode-display_width (1.8.0)
-    vmpooler (2.0.0)
+    vmpooler (2.1.0)
       concurrent-ruby (~> 1.1)
       connection_pool (~> 2.2)
       net-ldap (~> 0.16)
@@ -197,7 +197,7 @@ DEPENDENCIES
   rubocop (~> 1.1.0)
   simplecov (>= 0.11.2)
   thor (~> 1.0, >= 1.0.1)
-  vmpooler (~> 2.0)
+  vmpooler (~> 2.1)
   vmpooler-provider-vsphere!
   yarjuf (>= 2.0)
 

lib/vmpooler/providers/vsphere.rb

@@ -51,8 +51,8 @@ module Vmpooler
           'vsphere'
         end
 
-        def folder_configured?(folder_title, base_folder, configured_folders, whitelist)
+        def folder_configured?(folder_title, base_folder, configured_folders, allowlist)
-          return true if whitelist&.include?(folder_title)
+          return true if allowlist&.include?(folder_title)
           return false unless configured_folders.keys.include?(folder_title)
           return false unless configured_folders[folder_title] == base_folder
 
@@ -119,7 +119,30 @@ module Vmpooler
           try >= max_tries ? raise : retry
         end
 
-        def purge_unconfigured_folders(base_folders, configured_folders, whitelist)
+        # Return a list of pool folders
+        def pool_folders(provider_name)
+          folders = {}
+          $config[:pools].each do |pool|
+            next unless pool['provider'] == provider_name.to_s
+
+            folder_parts = pool['folder'].split('/')
+            datacenter = get_target_datacenter_from_config(pool['name'])
+            folders[folder_parts.pop] = "#{datacenter}/vm/#{folder_parts.join('/')}"
+          end
+          folders
+        end
+
+        def get_base_folders(folders)
+          base = []
+          folders.each do |_key, value|
+            base << value
+          end
+          base.uniq
+        end
+
+        def purge_unconfigured_resources(allowlist)
+          configured_folders = pool_folders(name)
+          base_folders = get_base_folders(configured_folders)
           @connection_pool.with_metrics do |pool_object|
             connection = ensured_vsphere_connection(pool_object)
 
@@ -129,7 +152,7 @@ module Vmpooler
 
               folder_children.each do |folder_hash|
                 folder_hash.each do |folder_title, folder_object|
-                  destroy_folder_and_children(folder_object) unless folder_configured?(folder_title, base_folder, configured_folders, whitelist)
+                  destroy_folder_and_children(folder_object) unless folder_configured?(folder_title, base_folder, configured_folders, allowlist)
                 end
               end
             end

spec/unit/pool_manager_spec.rb

@@ -61,7 +61,7 @@ EOT
 ---
 :config:
 :providers:
-  :mock:
+  :vsphere:
 :pools:
   - name: '#{pool}'
     size: 1
@@ -1060,7 +1060,7 @@ EOT
     end
   end
 
-  describe '#purge_unused_vms_and_folders' do
+  describe '#purge_unused_vms_and_resources' do
     let(:config) { YAML.load(<<-EOT
 ---
 :config: {}
@@ -1074,104 +1074,46 @@ EOT
     }
 
     it 'should return when purging is not enabled' do
-      expect(subject.purge_unused_vms_and_folders).to be_nil
+      expect(subject.purge_unused_vms_and_resources).to be_nil
     end
 
     context 'with purging enabled globally' do
       before(:each) do
-        config[:config]['purge_unconfigured_folders'] = true
+        config[:config]['purge_unconfigured_resources'] = true
         expect(Thread).to receive(:new).and_yield
       end
 
       it 'should run a purge for each provider' do
-        expect(subject).to receive(:purge_vms_and_folders)
+        expect(subject).to receive(:purge_vms_and_resources)
 
-        subject.purge_unused_vms_and_folders
+        subject.purge_unused_vms_and_resources
       end
 
       it 'should log when purging fails' do
-        expect(subject).to receive(:purge_vms_and_folders).and_raise(RuntimeError,'MockError')
+        expect(subject).to receive(:purge_vms_and_resources).and_raise(RuntimeError,'MockError')
         expect(logger).to receive(:log).with('s', '[!] failed while purging provider mock VMs and folders with an error: MockError')
 
-        subject.purge_unused_vms_and_folders
+        subject.purge_unused_vms_and_resources
       end
     end
 
     context 'with purging enabled on the provider' do
       before(:each) do
-        config[:providers][:mock]['purge_unconfigured_folders'] = true
+        config[:providers][:mock]['purge_unconfigured_resources'] = true
         expect(Thread).to receive(:new).and_yield
       end
 
       it 'should run a purge for the provider' do
-        expect(subject).to receive(:purge_vms_and_folders)
+        expect(subject).to receive(:purge_vms_and_resources)
 
-        subject.purge_unused_vms_and_folders
+        subject.purge_unused_vms_and_resources
       end
     end
   end
 
-  describe '#pool_folders' do
+  describe '#purge_vms_and_resources' do
-    let(:folder_name) { 'myinstance' }
-    let(:folder_base) { 'vmpooler' }
-    let(:folder) { [folder_base,folder_name].join('/') }
-    let(:datacenter) { 'dc1' }
     let(:provider_name) { 'mock_provider' }
-    let(:expected_response) {
+    let(:allowlist) { nil }
-      {
-        folder_name => "#{datacenter}/vm/#{folder_base}"
-      }
-    }
-    let(:config) { YAML.load(<<-EOT
----
-:providers:
-  :mock:
-:pools:
-  - name: '#{pool}'
-    folder: '#{folder}'
-    size: 1
-    datacenter: '#{datacenter}'
-    provider: '#{provider_name}'
-  - name: '#{pool}2'
-    folder: '#{folder}'
-    size: 1
-    datacenter: '#{datacenter}'
-    provider: '#{provider_name}2'
-EOT
-      )
-    }
-
-    context 'when evaluating pool folders' do
-      before do
-        expect(subject).not_to be_nil
-        # Inject mock provider into global variable - Note this is a code smell
-        $providers = { provider_name => provider }
-      end
-
-      it 'should return a list of pool folders' do
-        expect(provider).to receive(:get_target_datacenter_from_config).with(pool).and_return(datacenter)
-
-        expect(subject.pool_folders(provider_name)).to eq(expected_response)
-      end
-
-      it 'should raise an error when the provider fails to get the datacenter' do
-        expect(provider).to receive(:get_target_datacenter_from_config).with(pool).and_raise('mockerror')
-
-        expect{ subject.pool_folders(provider_name) }.to raise_error(RuntimeError, 'mockerror')
-      end
-    end
-  end
-
-  describe '#purge_vms_and_folders' do
-    let(:folder_name) { 'myinstance' }
-    let(:folder_base) { 'vmpooler' }
-    let(:datacenter) { 'dc1' }
-    let(:full_folder_path) { "#{datacenter}/vm/folder_base" }
-    let(:configured_folders) { { folder_name => full_folder_path } }
-    let(:base_folders) { [ full_folder_path ] }
-    let(:folder) { [folder_base,folder_name].join('/') }
-    let(:provider_name) { 'mock_provider' }
-    let(:whitelist) { nil }
     let(:config) { YAML.load(<<-EOT
 ---
 :config: {}
@@ -1179,9 +1121,7 @@ EOT
   :mock_provider: {}
 :pools:
   - name: '#{pool}'
-    folder: '#{folder}'
     size: 1
-    datacenter: '#{datacenter}'
     provider: '#{provider_name}'
 EOT
       )
@@ -1194,20 +1134,18 @@ EOT
         $providers = { provider_name => provider }
       end
 
-      it 'should run purge_unconfigured_folders' do
+      it 'should run purge_unconfigured_resources' do
-        expect(subject).to receive(:pool_folders).and_return(configured_folders)
+        expect(provider).to receive(:purge_unconfigured_resources).with(allowlist)
-        expect(provider).to receive(:purge_unconfigured_folders).with(base_folders, configured_folders, whitelist)
+        allow(provider).to receive(:provider_config).and_return({})
-        expect(provider).to receive(:provider_config).and_return({})
 
-        subject.purge_vms_and_folders(provider_name)
+        subject.purge_vms_and_resources(provider_name)
       end
 
       it 'should raise any errors' do
-        expect(subject).to receive(:pool_folders).and_return(configured_folders)
+        expect(provider).to receive(:purge_unconfigured_resources).with(allowlist).and_raise('mockerror')
-        expect(provider).to receive(:purge_unconfigured_folders).with(base_folders, configured_folders, whitelist).and_raise('mockerror')
+        allow(provider).to receive(:provider_config).and_return({})
-        expect(provider).to receive(:provider_config).and_return({})
 
-        expect{ subject.purge_vms_and_folders(provider_name) }.to raise_error(RuntimeError, 'mockerror')
+        expect{ subject.purge_vms_and_resources(provider_name) }.to raise_error(RuntimeError, 'mockerror')
       end
     end
   end

spec/unit/providers/vsphere_spec.rb

@@ -104,36 +104,36 @@ EOT
     let(:other_folder) { 'folder2' }
     let(:base_folder) { 'dc1/vm/base' }
     let(:configured_folders) { { folder_title => base_folder } }
-    let(:whitelist) { nil }
+    let(:allowlist) { nil }
     it 'should return true when configured_folders includes the folder_title' do
-      expect(subject.folder_configured?(folder_title, base_folder, configured_folders, whitelist)).to be true
+      expect(subject.folder_configured?(folder_title, base_folder, configured_folders, allowlist)).to be true
     end
 
     it 'should return false when title is not in configured_folders' do
-      expect(subject.folder_configured?(other_folder, base_folder, configured_folders, whitelist)).to be false
+      expect(subject.folder_configured?(other_folder, base_folder, configured_folders, allowlist)).to be false
     end
 
     context 'with another base folder' do
       let(:base_folder) { 'dc2/vm/base' }
       let(:configured_folders) { { folder_title => 'dc1/vm/base' } }
       it 'should return false' do
-        expect(subject.folder_configured?(folder_title, base_folder, configured_folders, whitelist)).to be false
+        expect(subject.folder_configured?(folder_title, base_folder, configured_folders, allowlist)).to be false
       end
     end
 
-    context 'with a whitelist set' do
+    context 'with a allowlist set' do
-      let(:whitelist) { [ other_folder ] }
+      let(:allowlist) { [ other_folder ] }
       it 'should return true' do
-        expect(subject.folder_configured?(other_folder, base_folder, configured_folders, whitelist)).to be true
+        expect(subject.folder_configured?(other_folder, base_folder, configured_folders, allowlist)).to be true
       end
     end
 
-    context 'with string whitelist value' do
+    context 'with string allowlist value' do
-      let(:whitelist) { 'whitelist' }
+      let(:allowlist) { 'allowlist' }
       it 'should raise an error' do
-        expect(whitelist).to receive(:include?).and_raise('mockerror')
+        expect(allowlist).to receive(:include?).and_raise('mockerror')
 
-        expect{ subject.folder_configured?(other_folder, base_folder, configured_folders, whitelist) }.to raise_error(RuntimeError, 'mockerror')
+        expect{ subject.folder_configured?(other_folder, base_folder, configured_folders, allowlist) }.to raise_error(RuntimeError, 'mockerror')
       end
     end
   end
@@ -297,12 +297,61 @@ EOT
     end
   end
 
-  describe '#purge_unconfigured_folders' do
+  describe '#pool_folders' do
+    let(:pool) { 'pool1' }
+    let(:folder_name) { 'myinstance' }
+    let(:folder_base) { 'vmpooler' }
+    let(:folder) { [folder_base,folder_name].join('/') }
+    let(:datacenter) { 'dc1' }
+    let(:provider_name) { 'mock_provider' }
+    let(:expected_response) {
+      {
+        folder_name => "#{datacenter}/vm/#{folder_base}"
+      }
+    }
+    context 'when evaluating pool folders' do
+      before do
+        expect(subject).not_to be_nil
+        #replace top-level global config
+        $config = YAML.load(<<-EOT
+---
+:providers:
+  :mock:
+:pools:
+  - name: '#{pool}'
+    folder: '#{folder}'
+    size: 1
+    datacenter: '#{datacenter}'
+    provider: '#{provider_name}'
+  - name: '#{pool}2'
+    folder: '#{folder}'
+    size: 1
+    datacenter: '#{datacenter}'
+    provider: '#{provider_name}2'
+        EOT
+        )
+      end
+
+      it 'should return a list of pool folders' do
+        expect(subject).to receive(:get_target_datacenter_from_config).with(pool).and_return(datacenter)
+
+        expect(subject.pool_folders(provider_name)).to eq(expected_response)
+      end
+
+      it 'should raise an error when the provider fails to get the datacenter' do
+        expect(subject).to receive(:get_target_datacenter_from_config).with(pool).and_raise('mockerror')
+
+        expect{ subject.pool_folders(provider_name) }.to raise_error(RuntimeError, 'mockerror')
+      end
+    end
+  end
+
+  describe '#purge_unconfigured_resources' do
     let(:folder_title) { 'folder1' }
     let(:base_folder) { 'dc1/vm/base' }
     let(:folder_object) { mock_RbVmomi_VIM_Folder({ :name => base_folder }) }
     let(:child_folder) { mock_RbVmomi_VIM_Folder({ :name => folder_title }) }
-    let(:whitelist) { nil }
+    let(:allowlist) { nil }
     let(:base_folders) { [ base_folder ] }
     let(:configured_folders) { { folder_title => base_folder } }
     let(:folder_children) { [ folder_title => child_folder ] }
@@ -310,6 +359,7 @@ EOT
 
     before(:each) do
       allow(subject).to receive(:connect_to_vsphere).and_return(connection)
+      allow(subject).to receive(:pool_folders).and_return(configured_folders)
     end
 
     context 'with an empty folder' do
@@ -317,7 +367,7 @@ EOT
         expect(subject).to receive(:get_folder_children).with(base_folder, connection).and_return(empty_list)
         expect(subject).to_not receive(:destroy_folder_and_children)
 
-        subject.purge_unconfigured_folders(base_folders, configured_folders, whitelist)
+        subject.purge_unconfigured_resources(allowlist)
       end
     end
 
@@ -325,7 +375,7 @@ EOT
       expect(subject).to receive(:get_folder_children).with(base_folder, connection).and_return(folder_children)
       allow(subject).to receive(:folder_configured?).and_return(true)
 
-      subject.purge_unconfigured_folders(base_folders, configured_folders, whitelist)
+      subject.purge_unconfigured_resources(allowlist)
     end
 
     context 'with a folder that is not configured' do
@@ -337,14 +387,14 @@ EOT
       it 'should destroy the folder and children' do
         expect(subject).to receive(:destroy_folder_and_children).with(child_folder).and_return(nil)
 
-        subject.purge_unconfigured_folders(base_folders, configured_folders, whitelist)
+        subject.purge_unconfigured_resources(allowlist)
       end
     end
 
     it 'should raise any errors' do
       expect(subject).to receive(:get_folder_children).and_throw('mockerror')
 
-      expect{ subject.purge_unconfigured_folders(base_folders, configured_folders, whitelist) }.to raise_error(/mockerror/)
+      expect{ subject.purge_unconfigured_resources(allowlist) }.to raise_error(/mockerror/)
     end
   end
 

vmpooler-provider-vsphere.gemspec

@@ -17,7 +17,7 @@ Gem::Specification.new do |s|
   s.require_paths = ["lib"]
   s.add_dependency 'rbvmomi', '>= 2.1', '< 4.0'
   
-  s.add_development_dependency 'vmpooler', '~> 2.0'
+  s.add_development_dependency 'vmpooler', '~> 2.1' # renaming done in version 2.1
 
   # Testing dependencies
   s.add_development_dependency 'climate_control', '>= 0.2.0'