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

Move ResourceGroup relationship into VmOrTemplate model #14948

Merged
merged 1 commit into from
Jun 2, 2017
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
3 changes: 1 addition & 2 deletions app/models/manageiq/providers/cloud_manager/vm.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ class ManageIQ::Providers::CloudManager::Vm < ::Vm
belongs_to :flavor
belongs_to :orchestration_stack
belongs_to :cloud_tenant
belongs_to :resource_group

has_many :network_ports, :as => :device
has_many :cloud_subnets, -> { distinct }, :through => :network_ports
Expand Down Expand Up @@ -127,7 +126,7 @@ def resize_queue(userid, new_flavor)
def resize(new_flavor_id)
raise ArgumentError, _("new_flavor_id cannot be nil") if new_flavor_id.nil?
new_flavor = Flavor.find(new_flavor_id)
raise ArgumentError, _("flavor cannot be found") if new_flavor.nil?
raise ArgumentError, _("flavor cannot be found") if new_flavor.nil?
raw_resize(new_flavor)
end

Expand Down
8 changes: 7 additions & 1 deletion app/models/resource_group.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
class ResourceGroup < ApplicationRecord
has_many :vms
alias_attribute :images, :templates
Copy link
Member

@kbrock kbrock May 24, 2017

Choose a reason for hiding this comment

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

Do we need images alias?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just thought it would be a handy alias. :)


has_many :vm_or_templates

# Rely on default scopes to get expected information
has_many :vms, :class_name => 'Vm'
has_many :templates, :class_name => 'MiqTemplate'
end
2 changes: 2 additions & 0 deletions app/models/vm_or_template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ class VmOrTemplate < ApplicationRecord
has_many :guest_applications, :dependent => :destroy
has_many :patches, :dependent => :destroy

belongs_to :resource_group
Copy link
Member

Choose a reason for hiding this comment

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

👍


# Accounts - Users and Groups
has_many :accounts, :dependent => :destroy
has_many :users, -> { where(:accttype => 'user') }, :class_name => "Account"
Expand Down
20 changes: 18 additions & 2 deletions spec/models/resource_group_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,24 @@
end

context "relationships" do
it "has many vms" do
expect(resource_group).to respond_to(:vms)
before do
@vm = FactoryGirl.create(:vm_google, :template => false, :resource_group => resource_group)
@template = FactoryGirl.create(:template_google, :template => true, :resource_group => resource_group)
end

it "returns the expected results for vms" do
expect(resource_group.vms).to include(@vm)
expect(resource_group.vms).to_not include(@template)
end

it "returns the expected results for templates" do
expect(resource_group.templates).to include(@template)
expect(resource_group.templates).to_not include(@vm)
end

it "returns the expected results for vm_or_templates" do
expect(resource_group.vm_or_templates).to include(@template)
expect(resource_group.vm_or_templates).to include(@vm)
end
end
end
13 changes: 13 additions & 0 deletions spec/models/vm_or_template_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,19 @@
end
end

context "#resource_group" do
before do
Copy link
Member

Choose a reason for hiding this comment

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

Why the context, @variables and a before block if there is only one test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case I add more tests later.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought... Do we even need this test? It feels like we're testing rails here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, sanity testing in case someone adds scopes/filters/whatever that conflict with what I'm expecting.

Copy link
Member

Choose a reason for hiding this comment

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

If this is a regular activerecord has_one, I don't understand why we'd need to test that. They're usually helpful when you're developing and want to make sure you did both sides of the association correctly but aren't really needed after that.

@resource_group = FactoryGirl.create(:resource_group)
@vm_with_rg = FactoryGirl.create(:vm_amazon, :resource_group => @resource_group)
@vm_without_rg = FactoryGirl.create(:vm_amazon)
end

it "has a has_one association with resource groups" do
expect(@vm_with_rg.resource_group).to eql(@resource_group)
expect(@vm_without_rg.resource_group).to be_nil
end
end

context "#scan_profile_categories" do
before do
@vm = FactoryGirl.create(:vm_vmware)
Expand Down