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

Save Physical Chassis #17236

Merged
merged 1 commit into from
Apr 18, 2018
Merged

Save Physical Chassis #17236

merged 1 commit into from
Apr 18, 2018

Conversation

caiocmpaes
Copy link
Contributor

@caiocmpaes caiocmpaes commented Mar 29, 2018

This PR is able to save Physical Chassis model into ManageIQ. The choice for the name PhysicalChassis is in order to have consistency with PhysicalRack and PhysicalServer. I used the approach sugested in ManageIQ/manageiq#16853 to save the physical_rack and the physical_servers related to It.

Depends on:

@caiocmpaes caiocmpaes changed the title Save Physical Chassis [WIP] Save Physical Chassis Apr 2, 2018
@miq-bot miq-bot added the wip label Apr 2, 2018
@miq-bot
Copy link
Member

miq-bot commented Apr 4, 2018

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

@caiocmpaes caiocmpaes changed the title [WIP] Save Physical Chassis Save Physical Chassis Apr 11, 2018
@miq-bot miq-bot removed the wip label Apr 11, 2018
@@ -66,6 +66,7 @@ def self.api_allowed_attributes
has_many :storage_profiles, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :physical_racks, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :physical_switches, :foreign_key => "ems_id", :dependent => :destroy, :inverse_of => :ext_management_system
has_many :physical_chassis, :foreign_key => "ems_id", :class_name => "PhysicalChassis", :dependent => :destroy, :inverse_of => :ext_management_system
Copy link
Member

Choose a reason for hiding this comment

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

Did you need the class_name just for the inflection? If so you can add it to https://github.com/ManageIQ/manageiq/tree/master/lib/vmdb/inflections.rb

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 already added the rule to the inflections.rb. This prevents rails from making physical_chassis become physical_chasses when It is mouting database queries. However, even with the inflections file, I still had to set this class_name. Seems like the inflections file is not considered in the instantiation of the ExtManagementSystem
error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error on the screenshot happens when I remove the ":class_name => "PhysicalChassis"

Copy link
Member

Choose a reason for hiding this comment

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

That is because you need the capital inflection for the class names, see Queue/queue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I was using the inflections only to the table names. Now It is for Class Names too.

@miq-bot
Copy link
Member

miq-bot commented Apr 17, 2018

Checked commit https://github.com/caiocmpaes/manageiq/commit/35523efb7e7e1184955752580a7726c85418dbfc with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM

@agrare agrare merged commit 4176adb into ManageIQ:master Apr 18, 2018
@agrare agrare added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 18, 2018
@caiocmpaes caiocmpaes deleted the save_physical_chassi branch April 18, 2018 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants