issue209: take into account all remarks from the PR.

source/lib/vagrant-openstack-provider/action/create_server.rb

@@ -31,7 +31,7 @@ def execute(env)
 
           options = {
             flavor: @resolver.resolve_flavor(env),
-            image: @resolver.resolve_image(env, config.image),
+            image: @resolver.resolve_image(env),
             volume_boot: @resolver.resolve_volume_boot(env),
             networks: @resolver.resolve_networks(env),
             volumes: @resolver.resolve_volumes(env),

source/lib/vagrant-openstack-provider/client/nova.rb

@@ -52,15 +52,15 @@ def create_server(env, options)
           s['name'] = options[:name]
           if options[:image_ref].nil?
             s['block_device_mapping'] = [{ volume_id: options[:volume_boot][:id],
-                                           device_name: options[:volume_boot][:device] }] unless options[:volume_boot][:id].nil?
+                                           device_name: options[:volume_boot][:device] }] if options[:volume_boot].key?(:id)
             s['block_device_mapping_v2'] = [{ boot_index: '0',
                                               volume_size: options[:volume_boot][:size],
                                               uuid: options[:volume_boot][:image],
                                               device_name: options[:volume_boot][:device],
                                               source_type: 'image',
                                               destination_type: 'volume',
                                               delete_on_termination: options[:volume_boot][:delete_on_destroy] }]\
-                                              unless options[:volume_boot][:image].nil?
+                                              if options[:volume_boot].key?(:image)
           else
             s['imageRef'] = options[:image_ref]
           end

source/lib/vagrant-openstack-provider/config_resolver.rb

@@ -23,16 +23,14 @@ def resolve_flavor(env)
         flavor
       end
 
-      def resolve_image(env, image_name)
+      def resolve_image(env)
         @logger.info 'Resolving image'
-        return nil if image_name.nil?
+        resolve_image_internal(env, env[:machine].provider_config.image)
+      end
 
-        nova = env[:openstack_client].nova
-        env[:ui].info(I18n.t('vagrant_openstack.finding_image'))
-        images = nova.get_all_images(env)
-        image = find_matching(images, image_name)
-        fail Errors::NoMatchingImage unless image
-        image
+      def resolve_volume_boot_image(env)
+        @logger.info 'Resolving image to create a volume from'
+        resolve_image_internal(env, env[:machine].provider_config.volume_boot[:image])
       end
 
       def resolve_floating_ip(env)
@@ -90,9 +88,13 @@ def resolve_volume_boot(env)
         size = (volume[:size].nil?) ? nil : volume[:size]
         delete_on_destroy = (volume[:delete_on_destroy].nil?) ? nil : volume[:delete_on_destroy]
 
-        image = resolve_image(env, volume[:image]) unless volume[:image].nil?
+        image = resolve_volume_boot_image(env) unless volume[:image].nil?
         image_id = (image.nil?) ? nil : image.id
-        { id: volume[:id], image: image_id, device: device, size: size, delete_on_destroy: delete_on_destroy }
+        if image.nil?
+          return { id: volume[:id], device: device }
+        else
+          { image: image_id, device: device, size: size, delete_on_destroy: delete_on_destroy }
+        end
       end
 
       def resolve_volumes(env)
@@ -138,6 +140,17 @@ def resolve_security_groups(env)
 
       private
 
+      def resolve_image_internal(env, image_name)
+        return nil if image_name.nil?
+
+        nova = env[:openstack_client].nova
+        env[:ui].info(I18n.t('vagrant_openstack.finding_image'))
+        images = nova.get_all_images(env)
+        image = find_matching(images, image_name)
+        fail Errors::NoMatchingImage unless image
+        image
+      end
+
       def search_free_ip(config, nova, env)
         @logger.debug 'Retrieving all allocated floating ips on tenant'
         all_floating_ips = nova.get_all_floating_ips(env)
@@ -261,31 +274,37 @@ def resolve_volume_from_string(volume, volume_list)
       def resolve_volume_from_hash(volume, volume_list, volume_ids)
         device = nil
         device = volume[:device] if volume.key?(:device)
+        delete_on_destroy = (volume[:delete_on_destroy].nil?) ? 'true' : volume[:delete_on_destroy]
 
         volume_id = nil
         if volume.key?(:id)
           fail Errors::ConflictVolumeNameId, volume: volume if volume.key?(:name)
-          fail Errors::ConflictVolumeNameId, volume: volume if volume.key?(:image)
+          check_boot_volume_conflict(volume)
           volume_id = volume[:id]
           fail Errors::UnresolvedVolumeId, id: volume_id unless volume_ids.include? volume_id
         elsif volume.key?(:name)
           volume_list.each do |v|
             next unless v.name.eql? volume[:name]
             fail Errors::MultipleVolumeName, name: volume[:name] unless volume_id.nil?
-            fail Errors::ConflictVolumeNameId, volume: volume if volume.key?(:image)
+            check_boot_volume_conflict(volume)
             volume_id = v.id
           end
           fail Errors::UnresolvedVolumeName, name: volume[:name] unless volume_ids.include? volume_id
         elsif volume.key?(:image)
           fail Errors::UnresolvedVolume, volume: volume unless volume.key?(:size)
-          delete_on_destroy = (volume[:delete_on_destroy].nil?) ? 'true' : volume[:delete_on_destroy]
-          return { id: volume_id, image: volume[:image], device: device, size: volume[:size], delete_on_destroy: delete_on_destroy }
+          fail Errors::ConflictBootVolume, volume: volume if volume.key?(:id)
+          fail Errors::ConflictBootVolume, volume: volume if volume.key?(:name)
+          return { image: volume[:image], device: device, size: volume[:size], delete_on_destroy: delete_on_destroy }
         else
-          fail Errors::ConflictVolumeNameId, volume: volume
+          fail Errors::ConflictBootVolume, volume: volume
         end
         { id: volume_id, device: device }
       end
 
+      def check_boot_volume_conflict(volume)
+        fail Errors::ConflictBootVolume, volume: volume if volume.key?(:image) || volume.key?(:size) || volume.key?(:delete_on_destroy)
+      end
+
       # This method finds any matching _thing_ from a list of names
       # in a collection of _things_. The first to match is the returned
       # one. Names in list can be a regexp, a partial match is chosen

source/lib/vagrant-openstack-provider/errors.rb

@@ -51,8 +51,8 @@ class NoMatchingImage < VagrantOpenstackError
         error_key(:no_matching_image)
       end
 
-      class ConflictImageName < VagrantOpenstackError
-        error_key(:conflict_image_name)
+      class ConflictBootVolume < VagrantOpenstackError
+        error_key(:conflict_boot_volume)
       end
 
       class SyncMethodError < VagrantOpenstackError

source/locales/en.yml

@@ -167,9 +167,9 @@ en:
       no_matching_image: |-
         No matching image was found! Please check your image setting to
         make sure you have a valid image chosen.
-      conflict_image_name: |-
-        One (and only one) image must be specified in the configuration.
-        Please check that an image name is not defined also in the 'volume_boot' definition.
+      conflict_boot_volume: |-
+        When booting from an existing volume it is not authorized to specify in your Vagrantfile either 'image' or 'size' or 'delete_on_destroy'.
+        When booting from a newly creating volume it is not authorized to specify in your Vagrantfile either 'id' or 'name'.
       sync_method_error: |-
         Value '%{sync_method_value}' is not allowed for 'sync_method' configuration parameter. Valid values are 'rsync' and 'none'
       rsync_error: |-

source/spec/vagrant-openstack-provider/action/create_server_spec.rb

@@ -58,7 +58,7 @@
       r.stub(:resolve_flavor).with(anything) do
         Flavor.new('flavor-01', 'small', nil, nil, nil)
       end
-      r.stub(:resolve_image).with(anything, anything) do
+      r.stub(:resolve_image).with(anything) do
         Item.new('image-01', 'ubuntu')
       end
       r.stub(:resolve_volume_boot).with(anything) { 'ubuntu-drive' }

source/spec/vagrant-openstack-provider/client/nova_spec.rb

@@ -222,10 +222,7 @@
               headers:
               {
                 'Accept' => 'application/json',
-                'Accept-Encoding' => 'gzip, deflate',
-                'Content-Length' => '248',
                 'Content-Type' => 'application/json',
-                'User-Agent' => 'Ruby',
                 'X-Auth-Token' => '123456'
               })
             .to_return(status: 202, body: '{ "server": { "id": "o1o2o3" } }')
@@ -250,10 +247,7 @@
               headers:
               {
                 'Accept' => 'application/json',
-                'Accept-Encoding' => 'gzip, deflate',
-                'Content-Length' => '127',
                 'Content-Type' => 'application/json',
-                'User-Agent' => 'Ruby',
                 'X-Auth-Token' => '123456'
               })
             .to_return(status: 202, body: '{ "server": { "id": "o1o2o3" } }')

source/spec/vagrant-openstack-provider/config_resolver_spec.rb

@@ -190,7 +190,7 @@
           [Item.new('img-001', 'image-01'),
            Item.new('img-002', 'image-02')]
         end
-        @action.resolve_image(env, config.image).should eq(Item.new('img-001', 'image-01'))
+        @action.resolve_image(env).should eq(Item.new('img-001', 'image-01'))
       end
     end
     context 'with name' do
@@ -201,7 +201,7 @@
           [Item.new('img-001', 'image-01'),
            Item.new('img-002', 'image-02')]
         end
-        @action.resolve_image(env, config.image).should eq(Item.new('img-002', 'image-02'))
+        @action.resolve_image(env).should eq(Item.new('img-002', 'image-02'))
       end
     end
     context 'with invalid identifier' do
@@ -212,7 +212,7 @@
           [Item.new('img-001', 'image-01'),
            Item.new('img-002', 'image-02')]
         end
-        expect { @action.resolve_image(env, config.image) }.to raise_error(Errors::NoMatchingImage)
+        expect { @action.resolve_image(env) }.to raise_error(Errors::NoMatchingImage)
       end
     end
     context 'with no images in config' do
@@ -223,7 +223,7 @@
           [Item.new('img-001', 'image-01'),
            Item.new('img-002', 'image-02')]
         end
-        @action.resolve_image(env, config.image).should eq(nil)
+        @action.resolve_image(env).should eq(nil)
       end
     end
   end
@@ -520,11 +520,7 @@
       context 'with string volume id' do
         it 'returns normalized volume' do
           config.stub(:volume_boot) { '001' }
-          nova.stub(:get_all_images).with(anything) do
-            [Item.new('img-001', 'image-01'),
-             Item.new('img-002', 'image-02')]
-          end
-          expect(@action.resolve_volume_boot(env)).to eq id: '001', image: nil, device: 'vda', size: nil, delete_on_destroy: nil
+          expect(@action.resolve_volume_boot(env)).to eq id: '001', device: 'vda'
         end
       end
 
@@ -535,18 +531,14 @@
             [Item.new('img-001', 'image-01'),
              Item.new('img-002', 'image-02')]
           end
-          expect(@action.resolve_volume_boot(env)).to eq id: '001', image: nil, device: 'vda', size: nil, delete_on_destroy: nil
+          expect(@action.resolve_volume_boot(env)).to eq id: '001', device: 'vda'
         end
       end
 
       context 'with hash volume id' do
         it 'returns normalized volume' do
           config.stub(:volume_boot) { { id: '001' } }
-          nova.stub(:get_all_images).with(anything) do
-            [Item.new('img-001', 'image-01'),
-             Item.new('img-002', 'image-02')]
-          end
-          expect(@action.resolve_volume_boot(env)).to eq id: '001', image: nil, device: 'vda', size: nil, delete_on_destroy: nil
+          expect(@action.resolve_volume_boot(env)).to eq id: '001', device: 'vda'
         end
       end
 
@@ -557,18 +549,14 @@
             [Item.new('img-001', 'image-01'),
              Item.new('img-002', 'image-02')]
           end
-          expect(@action.resolve_volume_boot(env)).to eq id: '001', image: nil, device: 'vda', size: nil, delete_on_destroy: nil
+          expect(@action.resolve_volume_boot(env)).to eq id: '001', device: 'vda'
         end
       end
 
       context 'with hash volume id and device' do
         it 'returns normalized volume' do
           config.stub(:volume_boot) { { id: '001', device: 'vdb' } }
-          nova.stub(:get_all_images).with(anything) do
-            [Item.new('img-001', 'image-01'),
-             Item.new('img-002', 'image-02')]
-          end
-          expect(@action.resolve_volume_boot(env)).to eq id: '001', image: nil, device: 'vdb', size: nil, delete_on_destroy: nil
+          expect(@action.resolve_volume_boot(env)).to eq id: '001', device: 'vdb'
         end
       end
 
@@ -579,14 +567,14 @@
             [Item.new('img-001', 'image-01'),
              Item.new('img-002', 'image-02')]
           end
-          expect(@action.resolve_volume_boot(env)).to eq id: '001', image: nil, device: 'vdb', size: nil, delete_on_destroy: nil
+          expect(@action.resolve_volume_boot(env)).to eq id: '001', device: 'vdb'
         end
       end
 
       context 'with empty hash' do
         it 'raises an error' do
           config.stub(:volume_boot) { {} }
-          expect { @action.resolve_volume_boot(env) }.to raise_error(Errors::ConflictVolumeNameId)
+          expect { @action.resolve_volume_boot(env) }.to raise_error(Errors::ConflictBootVolume)
         end
       end
 
@@ -628,14 +616,42 @@
       context 'with hash containing a name and an image_name' do
         it 'raises an error' do
           config.stub(:volume_boot) { { name: 'vol-01', image: 'img_001' } }
-          expect { @action.resolve_volume_boot(env) }.to raise_error(Errors::ConflictVolumeNameId)
+          expect { @action.resolve_volume_boot(env) }.to raise_error(Errors::ConflictBootVolume)
+        end
+      end
+
+      context 'with hash containing a name and a size' do
+        it 'raises an error' do
+          config.stub(:volume_boot) { { name: 'vol-01', size: '10' } }
+          expect { @action.resolve_volume_boot(env) }.to raise_error(Errors::ConflictBootVolume)
+        end
+      end
+
+      context 'with hash containing a name and a delete_on_destroy indication' do
+        it 'raises an error' do
+          config.stub(:volume_boot) { { name: 'vol-01', delete_on_destroy: 'true' } }
+          expect { @action.resolve_volume_boot(env) }.to raise_error(Errors::ConflictBootVolume)
         end
       end
 
       context 'with hash containing a volume_id and an image_name' do
         it 'raises an error' do
           config.stub(:volume_boot) { { id: 'id', image: 'img_001' } }
-          expect { @action.resolve_volume_boot(env) }.to raise_error(Errors::ConflictVolumeNameId)
+          expect { @action.resolve_volume_boot(env) }.to raise_error(Errors::ConflictBootVolume)
+        end
+      end
+
+      context 'with hash containing a volume_id and a size' do
+        it 'raises an error' do
+          config.stub(:volume_boot) { { id: 'id', size: '10' } }
+          expect { @action.resolve_volume_boot(env) }.to raise_error(Errors::ConflictBootVolume)
+        end
+      end
+
+      context 'with hash containing a volume_id and a delete_on_destroy indication' do
+        it 'raises an error' do
+          config.stub(:volume_boot) { { id: 'id', delete_on_destroy: 'true' } }
+          expect { @action.resolve_volume_boot(env) }.to raise_error(Errors::ConflictBootVolume)
         end
       end
 
@@ -653,7 +669,7 @@
             [Item.new('img-001', 'image-01'),
              Item.new('img-002', 'image-02')]
           end
-          expect(@action.resolve_volume_boot(env)).to eq id: nil, image: 'img-001', device: 'vda', size: '10', delete_on_destroy: 'true'
+          expect(@action.resolve_volume_boot(env)).to eq image: 'img-001', device: 'vda', size: '10', delete_on_destroy: 'true'
         end
       end
 
@@ -664,7 +680,7 @@
             [Item.new('img-001', 'image-01'),
              Item.new('img-002', 'image-02')]
           end
-          expect(@action.resolve_volume_boot(env)).to eq id: nil, image: 'img-001', device: 'vdb', size: '10', delete_on_destroy: 'false'
+          expect(@action.resolve_volume_boot(env)).to eq image: 'img-001', device: 'vdb', size: '10', delete_on_destroy: 'false'
         end
       end
     end
@@ -781,7 +797,7 @@
       context 'with empty hash' do
         it 'raises an error' do
           config.stub(:volumes) { [{}] }
-          expect { @action.resolve_volumes(env) }.to raise_error(Errors::ConflictVolumeNameId)
+          expect { @action.resolve_volumes(env) }.to raise_error(Errors::ConflictBootVolume)
         end
       end