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.

Sign up for GitHub

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

Jump to bottom

Adding PhysicalStorage into PhysicalChassis #17616

Merged
merged 1 commit into from
Jul 17, 2018
Merged

Adding PhysicalStorage into PhysicalChassis #17616

merged 1 commit into from
Jul 17, 2018

Conversation

EsdrasVP
Copy link
Member

@EsdrasVP EsdrasVP commented Jun 20, 2018 •
edited
Loading

This PR is able to:

  • Add the possibility of PhysicalStorage being inside a PhysicalChassis

Depends on:

@EsdrasVP EsdrasVP closed this Jun 20, 2018
@EsdrasVP EsdrasVP reopened this Jun 20, 2018
This was referenced Jun 21, 2018
Merged
Merged
@EsdrasVP EsdrasVP closed this Jun 21, 2018
@EsdrasVP EsdrasVP reopened this Jun 21, 2018
@EsdrasVP EsdrasVP force-pushed the add_storage_to_chassis branch 4 times, most recently from bede9b8 to a1413c0 Compare June 25, 2018 17:38
@EsdrasVP
Copy link
Member Author

@miq-bot assign @agrare

@EsdrasVP EsdrasVP force-pushed the add_storage_to_chassis branch 2 times, most recently from d8a97f4 to 4f12220 Compare June 29, 2018 19:40
@EsdrasVP EsdrasVP closed this Jun 29, 2018
@EsdrasVP EsdrasVP reopened this Jun 29, 2018
martinpovolny
martinpovolny approved these changes Jul 2, 2018
View reviewed changes
Copy link
Member

@martinpovolny martinpovolny left a comment

Choose a reason for hiding this comment

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

Looks good!

martinpovolny
martinpovolny approved these changes Jul 2, 2018
View reviewed changes
Copy link
Member

@martinpovolny martinpovolny left a comment

Choose a reason for hiding this comment

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

Looks good!

@martinpovolny
Copy link
Member

Sorry for the double review. Internet connection is making fun of me.

@agrare
Copy link
Member

agrare commented Jul 2, 2018

This looks fine to me but depends on ManageIQ/manageiq-schema#224
@EsdrasVP please put any dependent PRs in the description, this would have failed if merged before the schema change.

@martinpovolny
Copy link
Member

@agrare : can you merge both?

@agrare
Copy link
Member

agrare commented Jul 2, 2018

@martinpovolny no we need @Fryguy to merge the schema one

@EsdrasVP
Copy link
Member Author

EsdrasVP commented Jul 2, 2018

This looks fine to me but depends on ManageIQ/manageiq-schema#224
@EsdrasVP please put any dependent PRs in the description, this would have failed if merged before the schema change.

@agrare Sorry for that, I fixed it now.

@EsdrasVP EsdrasVP force-pushed the add_storage_to_chassis branch 2 times, most recently from 57c4bab to 2d112ad Compare July 5, 2018 20:07
@EsdrasVP EsdrasVP mentioned this pull request Jul 17, 2018
Closed
@EsdrasVP
Copy link
Member Author

EsdrasVP commented Jul 17, 2018 •
edited
Loading

@agrare Since the schema PR was merged, I think this one is ok too. Could you check it out?

agrare
agrare reviewed Jul 17, 2018
View reviewed changes
app/models/physical_storage.rb Outdated
belongs_to :physical_rack, :foreign_key => :physical_rack_id, :inverse_of => :physical_storages
belongs_to :physical_chassis, :foreign_key => :physical_chassis_id, :inverse_of => :physical_storages
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to specify the foreign_key here? It should "just work"

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially I thought that it was necessary, but it doesn't, so I removed it now.

agrare
agrare reviewed Jul 17, 2018
View reviewed changes
app/models/physical_rack.rb
has_many :physical_chassis, :dependent => :nullify, :inverse_of => :physical_rack
has_many :physical_servers, :dependent => :nullify, :inverse_of => :physical_rack
has_many :physical_storages, :dependent => :nullify, :inverse_of => :physical_rack
Copy link
Member

Choose a reason for hiding this comment

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

This is just adding an association that already existing on the physical_storage model right? Nothing was changed in schema for this to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it doesn't change nothing in schema. This was done because PhysicalRack requires an association with PhysicalStorage in order to get them when needed. If this is not added, then we can get a PhysicalRack from a PhysicalStorage, but the inverse does not work.

@miq-bot
Copy link
Member

miq-bot commented Jul 17, 2018

Checked commit EsdrasVP@b01d027 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

agrare added a commit to agrare/manageiq that referenced this pull request Jul 17, 2018
@agrare
Merge pull request ManageIQ#17616 from EsdrasVP/add_storage_to_chassis
ed7477c 
Adding PhysicalStorage into PhysicalChassis
@agrare agrare merged commit 70f26ec into ManageIQ:master Jul 17, 2018
@agrare agrare added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinpovolny martinpovolny martinpovolny approved these changes

@agrare agrare agrare approved these changes

Assignees

@agrare agrare

Labels
enhancement providers/physical
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@EsdrasVP @martinpovolny @agrare @miq-bot