From bf96c5af4ed1f9636d86411050476cd0ec6f9f93 Mon Sep 17 00:00:00 2001
From: Moti Asayag <masayag@redhat.com>
Date: Sun, 14 May 2017 14:13:54 +0300
Subject: [PATCH] Allow to receive datastore by name or by id

Datastores on the same setup may have the same name. In order to add a
disk on a specific datastore, it is better to use the datastore id.

https://bugzilla.redhat.com/show_bug.cgi?id=1437359
---
 .../redhat/infra_manager/provision/disk.rb    | 32 +++++++---
 .../infra_manager/provision/disk_spec.rb      | 59 +++++++++++++++++++
 2 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb b/app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb
index b82efb590..743a3cc61 100644
--- a/app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb
+++ b/app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb
@@ -36,14 +36,7 @@ def prepare_disks_for_add(disks_spec)
   end
 
   def prepare_disk_for_add(disk_spec)
-    storage_name = disk_spec[:datastore]
-    raise MiqException::MiqProvisionError, "Storage is required for disk: <#{disk_spec.inspect}>" if storage_name.blank?
-
-    storage = Storage.find_by(:name => storage_name)
-    if storage.nil?
-      raise MiqException::MiqProvisionError, "Unable to find storage: <#{storage_name}> for disk: <#{disk_spec.inspect}>"
-    end
-
+    storage = find_storage!(disk_spec)
     da_options = {
       :size_in_mb       => disk_spec[:sizeInMB],
       :storage          => storage,
@@ -56,4 +49,27 @@ def prepare_disk_for_add(disk_spec)
     disk_attachment_builder = ManageIQ::Providers::Redhat::InfraManager::DiskAttachmentBuilder.new(da_options)
     disk_attachment_builder.disk_attachment
   end
+
+  def find_storage!(disk_spec)
+    storage_id = disk_spec[:datastore_id]
+    storage_name = disk_spec[:datastore]
+    if storage_name.blank? && storage_id.blank?
+      raise MiqException::MiqProvisionError, "Storage is required for disk: <#{disk_spec.inspect}>"
+    end
+
+    if storage_id
+      storage = Storage.find(storage_id)
+      raise_storage_not_found!(disk_spec, storage_id) if storage.nil?
+    else
+      storage = Storage.find_by(:name => storage_name)
+      raise_storage_not_found!(disk_spec, storage_name) if storage.nil?
+    end
+
+    storage
+  end
+
+  def raise_storage_not_found!(disk_spec, storage_desc)
+    error = "Unable to find storage: <#{storage_desc}> for disk: <#{disk_spec.inspect}>"
+    raise MiqException::MiqProvisionError
+  end
 end
diff --git a/spec/models/manageiq/providers/redhat/infra_manager/provision/disk_spec.rb b/spec/models/manageiq/providers/redhat/infra_manager/provision/disk_spec.rb
index fec808b18..049905288 100644
--- a/spec/models/manageiq/providers/redhat/infra_manager/provision/disk_spec.rb
+++ b/spec/models/manageiq/providers/redhat/infra_manager/provision/disk_spec.rb
@@ -164,4 +164,63 @@
       @task.poll_add_disks_complete
     end
   end
+
+  context "#find_storage!" do
+    let(:datastore_name) { "test_storage" }
+    let(:datastore_id) { 123 }
+
+    context "when storage exists" do
+      let(:storage) { FactoryGirl.create(:storage, :name => "test_storage", :id => 123 ) }
+
+      it "searches for a storage by its ID" do
+        disk_spec = { :datastore_id => 123 }
+        allow(Storage).to receive(:find).with(datastore_id).and_return(storage)
+
+        expect(@task.send(:find_storage!, disk_spec)).to eq(storage)
+      end
+
+      it "searches for a storage by its name" do
+        disk_spec = { :datastore => datastore_name }
+        allow(Storage).to receive(:find_by).with(:name => datastore_name).and_return(storage)
+
+        expect(@task.send(:find_storage!, disk_spec)).to eq(storage)
+      end
+
+      it "searches for a storage both by its id and name" do
+        disk_spec = { :datastore_id => 123, :datastore => datastore_name }
+        allow(Storage).to receive(:find).with(datastore_id).and_return(storage)
+
+        expect(Storage).not_to receive(:find_by)
+        expect(@task.send(:find_storage!, disk_spec)).to eq(storage)
+      end
+    end
+
+    context "when storage does not exist" do
+      it "searches for a storage by its ID" do
+        disk_spec = { :datastore_id => 123 }
+        allow(Storage).to receive(:find).with(datastore_id).and_return(nil)
+
+        expect{ @task.send(:find_storage!, disk_spec) }.to raise_error(MiqException::MiqProvisionError)
+      end
+
+      it "searches for a storage by its name" do
+        disk_spec = { :datastore => datastore_name }
+        allow(Storage).to receive(:find_by).with(:name => datastore_name).and_return(nil)
+
+        expect{ @task.send(:find_storage!, disk_spec) }.to raise_error(MiqException::MiqProvisionError)
+      end
+
+      it "searches for a storage both by its id and name" do
+        disk_spec = { :datastore => datastore_name, :datastore_id => 123 }
+        allow(Storage).to receive(:find).with(datastore_id).and_return(nil)
+        expect(Storage).not_to receive(:find_by).with(:name => datastore_name)
+
+        expect{ @task.send(:find_storage!, disk_spec) }.to raise_error(MiqException::MiqProvisionError)
+      end
+
+      it "searches for a storage without specifying id or name" do
+        expect{ @task.send(:find_storage!, {}) }.to raise_error(MiqException::MiqProvisionError)
+      end
+    end
+  end
 end