Adds host to physical server relationship #14026

rodneyhbrown7 · 2017-02-22T20:07:20Z

-modifies the host and physical server controller
-updates the physical server and host model

rodneyhbrown7 · 2017-02-22T20:09:49Z

@miq-bot add_label wip, providers/physical-infrastructure

juliancheal · 2017-02-22T20:14:24Z

This PR relies on #13973 being merged.

juliancheal · 2017-02-22T20:15:18Z

app/models/host.rb

-
+
+  # Physical infra reference
+  has_one :physical_server, :foreign_key => "serialNumber", :primary_key => "service_tag", :class_name => "PhysicalServer"


:foreign_key should be serial_number not serialNumber.

rodneyhbrown7 · 2017-02-23T18:24:22Z

Closing to re-run travis tests

rodneyhbrown7 · 2017-02-23T18:24:42Z

Re-open to run travis tests based on upstream resolutions

blomquisg · 2017-02-28T15:41:55Z

app/models/host.rb

-
+
+  # Physical infra reference
+  has_one :physical_server, :foreign_key => "serial_number", :primary_key => "service_tag", :class_name => "PhysicalServer"


This relationship is correct, in that a Host has_one :physical_server. However, the foreign key of "serial_number" is incorrect.

This should just be:

has_one :physical_server, :inverse_of => :host

blomquisg · 2017-02-28T15:42:12Z

app/models/physical_server.rb

@@ -6,6 +6,7 @@ class PhysicalServer < ApplicationRecord
  belongs_to :ext_management_system, :foreign_key => :ems_id, :class_name => "ManageIQ::Providers::PhysicalInfraManager"

  default_value_for :enabled, true
+  has_one :host, :foreign_key => "service_tag", :primary_key => "serial_number"


This relationship should be belongs_to, so it would look like:

belongs_to :host

Greg, does a physical server belong to a host or does the host belong to the physical server? I was thinking that a physical server has a host, which has 1 - many VMs. It is possible to have a physical server with no host and a host with no VMs. Is my thinking backwards on this?

I have the same thought that @rodneyhbrown7 has. We need to sync this.

@blomquisg I agree with @rodneyhbrown7, because in the real life a "host" is installed into a "physical server", not the other way around. By the way, in real life (also) there is a possibility of exists a "blank" physical server, with no hypervisor installed(which means that we'll not have a host context to this physical server).

The names has_one and belongs_to are merely reflecting where the foreign key lives. If we want to move the foreign key over to the hosts table, then the model verbiage would reverse.

See also:

http://guides.rubyonrails.org/association_basics.html#the-belongs-to-association

http://guides.rubyonrails.org/association_basics.html#the-has-one-association

Here's the trick. Right now, it's the Physical Infrastructure Inventory saving code that's going to create this relationship. If the foreign key is moved from PhysicalServer to Host, it means that the Physical Infrastructure Inventory saving code would be changing an object in another manager's inventory.

This might not cause any problems. But, it's definitely a first for MIQ.

There are lots of long term ideas to make this relationship building less "brute force". But, unfortunately, no one has had time to dig into any of them to make it a reality. #soon!

juliancheal · 2017-03-02T00:46:35Z

This PR LGTM, but needs PR #13973 being merged first.

rodneyhbrown7 · 2017-03-15T19:06:06Z

Closing to re-run travis tests

rodneyhbrown7 · 2017-03-15T19:06:17Z

Re-opening to re-run travis tests

miq-bot · 2017-03-16T19:01:34Z

This pull request is not mergeable. Please rebase and repush.

-modifies the host and physical server controller -updates the physical server and host model

-use /api/hosts/<id>?attribute=physical_servers instead of /api/hosts/<id> to retrieve the list of associated physical_servers

rodneyhbrown7 · 2017-03-21T14:56:14Z

@miq-bot remove-label wip

rodneyhbrown7 · 2017-03-21T14:56:43Z

@miq-bot remove_label wip

chessbyte · 2017-03-21T15:00:05Z

app/controllers/api/physical_servers_controller.rb

+        response_payload['host'] = case physical_server.host
+                                   when nil then nil
+                                   else physical_server.host.id
+                                   end


I think this case statement can be simplified to:

response_payload['host'] = physical_server.host.try(:id)

@chessbyte is better response_payload['host'] or response_payload['host_id']? Once we will need the ID of the host to perform other request in host resource.

abellotti · 2017-03-21T17:28:58Z

I'm a bit confused on the API changes here as they are partial, no config/api.yml changes so the collection itself is not exposed (i.e. GET /api/physical_servers/1 gives a 404). was the intent to expose this in the API ?

Other than above, the code itself in the API could be a lot simpler leveraging @additional_attributes and defining host_id as a virtual attribute (delegate) in the model.

rodneyhbrown7 · 2017-03-21T18:31:36Z

This is due to the fact that we chunked up a larger commit into smaller commits and didn't do a good job of correlating all of the PRs. The actual api.yml file changes are in #14028 This PR updates the controller to make the host relationship in the model and adds the relationship to a controller.

As to the comment about the host_id. I think we determined that because of the association between hosts and servers we con't need an actual delegate? host_id is a property of physical_server because of the association.

blomquisg · 2017-03-23T16:40:00Z

One last thing: There was some discussion around the semantics of the belongs_to and has_one. @juliancheal changed it so that the hosts table has the physical_server_id. Whereas previously, the physical_servers table had the host_id.

That means that the direction of the relationship now reads correctly, but needs to be reversed from what you have here.

It now should be:

class Host

  belongs_to :physical_server, inverse_of => :host
end

class PhysicalServer

  has_one :host, inverse_of => :physical_server
end

rodneyhbrown7 · 2017-03-23T17:34:51Z

Reversing the polarity of the relationship between the servers and the host.

walteraa · 2017-03-23T18:27:54Z

@rodneyhbrown7 @blomquisg I have a doubt about migration: Considering this change that Rodney did, should we change the migration about this relationship?

blomquisg · 2017-03-23T18:41:20Z

@walteraa, nope. The foreign key is already on the correct table.

blomquisg · 2017-03-23T18:45:06Z

app/controllers/api/physical_servers_controller.rb

+        response_payload['host_id'] = case physical_server.host
+                                      when nil then nil
+                                      else physical_server.host.id
+                                      end


Oh, @chessbyte had a comment a while ago about this case statement.

The case statement could be:

response_payload['host_id'] = physical_server.host.try(:id)

Refactor relationship statement to a elegant way.

miq-bot · 2017-03-23T20:07:27Z

Checked commits https://github.com/lenovo/manageiq/compare/79fb96226d51d819330abedf67c4393fa66ebbb6~...3c72c7e9e16c6bef33cb4de29c4ca34f3aba49e3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🍪

rodneyhbrown7 changed the title ~~Adds host to physical server relationship~~ [WIP] Adds host to physical server relationship Feb 22, 2017

miq-bot added providers/physical wip labels Feb 22, 2017

juliancheal reviewed Feb 22, 2017

View reviewed changes

This was referenced Feb 22, 2017

Add physical server REST api #14028

Merged

Update firmware refs #14034

Closed

[WIP] Updates to support the lenovo physical provider #13911

Closed

rodneyhbrown7 closed this Feb 23, 2017

rodneyhbrown7 reopened this Feb 23, 2017

blomquisg suggested changes Feb 28, 2017

View reviewed changes

rodneyhbrown7 closed this Mar 15, 2017

rodneyhbrown7 reopened this Mar 15, 2017

miq-bot added the unmergeable label Mar 16, 2017

Rodney H. Brown added 3 commits March 17, 2017 11:15

Adds host to physical server relationship

79fb962

-modifies the host and physical server controller -updates the physical server and host model

Adjusts foreign key relationship between phys and host

18dd7d6

Removes show method from hosts controller

05adb35

-use /api/hosts/<id>?attribute=physical_servers instead of /api/hosts/<id> to retrieve the list of associated physical_servers

miq-bot removed the unmergeable label Mar 17, 2017

Addresses rubocop warnings from PR

1b116a6

miq-bot changed the title ~~[WIP] Adds host to physical server relationship~~ Adds host to physical server relationship Mar 21, 2017

miq-bot removed the wip label Mar 21, 2017

chessbyte added api enhancement labels Mar 21, 2017

chessbyte assigned blomquisg and abellotti Mar 21, 2017

chessbyte reviewed Mar 21, 2017

View reviewed changes

Rodney Brown added 2 commits March 21, 2017 12:30

Remove referenced to undefined :enabled attribute

7da578f

Modifies the REST response change host to host_id

f0207c2

Reverses the relationship of physical servers to hosts

838c057

blomquisg approved these changes Mar 23, 2017

View reviewed changes

blomquisg reviewed Mar 23, 2017

View reviewed changes

walteraa and others added 2 commits March 23, 2017 16:28

Refactor statement which show relationship in API

842fc85

Refactor relationship statement to a elegant way.

Removes custom show method from physical_servers_controller

3c72c7e

odravison mentioned this pull request Mar 24, 2017

[WIP] Structure for Physical Server Pages ManageIQ/manageiq-ui-classic#755

Closed

blomquisg merged commit 684f835 into ManageIQ:master Mar 24, 2017

blomquisg added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 24, 2017

bdunne mentioned this pull request Mar 24, 2017

inverse_of should be a symbol #14505

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds host to physical server relationship #14026

Adds host to physical server relationship #14026

rodneyhbrown7 commented Feb 22, 2017 •

edited

Loading

rodneyhbrown7 commented Feb 22, 2017

juliancheal commented Feb 22, 2017

juliancheal Feb 22, 2017

rodneyhbrown7 commented Feb 23, 2017

rodneyhbrown7 commented Feb 23, 2017

blomquisg Feb 28, 2017

blomquisg Feb 28, 2017

rodneyhbrown7 Mar 2, 2017 •

edited

Loading

odravison Mar 2, 2017

walteraa Mar 2, 2017 •

edited

Loading

blomquisg Mar 3, 2017 •

edited

Loading

juliancheal commented Mar 2, 2017

rodneyhbrown7 commented Mar 15, 2017

rodneyhbrown7 commented Mar 15, 2017

miq-bot commented Mar 16, 2017

rodneyhbrown7 commented Mar 21, 2017

rodneyhbrown7 commented Mar 21, 2017

chessbyte Mar 21, 2017 •

edited

Loading

walteraa Mar 21, 2017

abellotti commented Mar 21, 2017

rodneyhbrown7 commented Mar 21, 2017 •

edited

Loading

blomquisg commented Mar 23, 2017

rodneyhbrown7 commented Mar 23, 2017

walteraa commented Mar 23, 2017 •

edited

Loading

blomquisg commented Mar 23, 2017

blomquisg Mar 23, 2017

miq-bot commented Mar 23, 2017



		# Physical infra reference
		has_one :physical_server, :foreign_key => "serialNumber", :primary_key => "service_tag", :class_name => "PhysicalServer"

Adds host to physical server relationship #14026

Adds host to physical server relationship #14026

Conversation

rodneyhbrown7 commented Feb 22, 2017 • edited Loading

rodneyhbrown7 commented Feb 22, 2017

juliancheal commented Feb 22, 2017

juliancheal Feb 22, 2017

Choose a reason for hiding this comment

rodneyhbrown7 commented Feb 23, 2017

rodneyhbrown7 commented Feb 23, 2017

blomquisg Feb 28, 2017

Choose a reason for hiding this comment

blomquisg Feb 28, 2017

Choose a reason for hiding this comment

rodneyhbrown7 Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

odravison Mar 2, 2017

Choose a reason for hiding this comment

walteraa Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

blomquisg Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

juliancheal commented Mar 2, 2017

rodneyhbrown7 commented Mar 15, 2017

rodneyhbrown7 commented Mar 15, 2017

miq-bot commented Mar 16, 2017

rodneyhbrown7 commented Mar 21, 2017

rodneyhbrown7 commented Mar 21, 2017

chessbyte Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

walteraa Mar 21, 2017

Choose a reason for hiding this comment

abellotti commented Mar 21, 2017

rodneyhbrown7 commented Mar 21, 2017 • edited Loading

blomquisg commented Mar 23, 2017

rodneyhbrown7 commented Mar 23, 2017

walteraa commented Mar 23, 2017 • edited Loading

blomquisg commented Mar 23, 2017

blomquisg Mar 23, 2017

Choose a reason for hiding this comment

miq-bot commented Mar 23, 2017

rodneyhbrown7 commented Feb 22, 2017 •

edited

Loading

rodneyhbrown7 Mar 2, 2017 •

edited

Loading

walteraa Mar 2, 2017 •

edited

Loading

blomquisg Mar 3, 2017 •

edited

Loading

chessbyte Mar 21, 2017 •

edited

Loading

rodneyhbrown7 commented Mar 21, 2017 •

edited

Loading

walteraa commented Mar 23, 2017 •

edited

Loading