Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Vm#supports_terminate? checking EMS#vm_destroy #20222

Merged
merged 1 commit into from
May 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions app/models/vm_or_template/operations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,6 @@ def validate_vm_control_shelve_offload_action
end
unsupported_reason_add(:control, msg) if msg
end

supports :terminate do
msg = unsupported_reason(:control) unless supports_control?
msg ||= _("Provider doesn't support vm_destroy") unless ext_management_system.respond_to?(:vm_destroy)
unsupported_reason_add(:terminate, msg) if msg
end
end

def validate_vm_control_powered_on
Expand Down
54 changes: 34 additions & 20 deletions spec/models/vm_or_template_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -658,35 +658,49 @@
end
end

context "#supports_terminate?" do
let(:ems_does_vm_destroy) { FactoryBot.create(:ems_vmware) }
let(:ems_doesnot_vm_destroy) { FactoryBot.create(:ems_storage) }
context "#supports_control?" do
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these were actually testing supports_control?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@agrare agrare May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still an internal helper method used all over the place, e.g.: https://github.com/ManageIQ/manageiq-providers-vmware/blob/master/app/models/manageiq/providers/vmware/infra_manager/vm/operations/guest.rb#L6

The purpose of this PR isn't to refactor supports_control

let(:retired_vm) { FactoryBot.create(:vm, :retired => true, :ext_management_system => ems, :host => host) }
let(:template) { FactoryBot.create(:miq_template) }
let(:terminated_vm) { FactoryBot.create(:vm_amazon, :raw_power_state => "terminated") }
let(:vm_no_host) { FactoryBot.create(:vm, :ext_management_system => ems) }
let(:disconnected_vm) { FactoryBot.create(:vm, :host => host, :ext_management_system => ems, :connection_state => "disconnected") }
let(:archived_vm) { FactoryBot.create(:vm, :host => host) }
let(:vm) { FactoryBot.create(:vm, :ext_management_system => ems, :host => host) }
let(:ems) { FactoryBot.create(:ems_infra) }
let(:host) { FactoryBot.create(:host) }

it "returns true for a VM not terminated" do
vm = FactoryBot.create(:vm, :host => host, :ext_management_system => ems_does_vm_destroy)
allow(vm).to receive_messages(:terminated? => false)
expect(vm.supports_terminate?).to eq(true)
it "returns false for a retired vm" do
expect(retired_vm.supports_control?).to be_falsey
expect(retired_vm.unsupported_reason(:control)).to eq("The VM is retired")
end

it "returns false for a template" do
expect(template.supports_control?).to be_falsey
expect(template.unsupported_reason(:control)).to eq("The VM is a template")
end

it "returns false for a terminated VM" do
vm = FactoryBot.create(:vm, :host => host, :ext_management_system => ems_does_vm_destroy)
allow(vm).to receive_messages(:terminated? => true)
expect(vm.supports_terminate?).to eq(false)
expect(vm.unsupported_reason(:terminate)).to eq("The VM is terminated")
expect(terminated_vm.supports_control?).to eq(false)
expect(terminated_vm.unsupported_reason(:control)).to eq("The VM is terminated")
end

it "returns false for a vm without a host" do
expect(vm_no_host.supports_control?).to be_falsey
expect(vm_no_host.unsupported_reason(:control)).to eq("The VM is not connected to a Host")
end

it "returns false for a provider doesn't support vm_destroy" do
vm = FactoryBot.create(:vm, :host => host, :ext_management_system => ems_doesnot_vm_destroy)
allow(vm).to receive_messages(:terminated? => false)
expect(vm.supports_terminate?).to eq(false)
expect(vm.unsupported_reason(:terminate)).to eq("Provider doesn't support vm_destroy")
it "returns false for a disconnected vm" do
expect(disconnected_vm.supports_control?).to be_falsey
expect(disconnected_vm.unsupported_reason(:control)).to eq("The VM does not have a valid connection state")
end

it "returns false for an archived vm" do
expect(archived_vm.supports_control?).to be_falsey
expect(archived_vm.unsupported_reason(:control)).to eq("The VM is not connected to an active Provider")
end

it "returns false for a WMware VM" do
vm = FactoryBot.create(:vm, :host => host, :ext_management_system => ems_does_vm_destroy)
allow(vm).to receive_messages(:terminated? => false)
expect(vm.supports_terminate?).to eq(true)
it "returns true for a valid vm" do
expect(vm.supports_control?).to be_truthy
end
end

Expand Down