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

Fixing Computer System relationship on Physical Storage #17945

Merged
merged 1 commit into from
Sep 5, 2018
Merged

Fixing Computer System relationship on Physical Storage #17945

merged 1 commit into from
Sep 5, 2018

Conversation

EsdrasVP
Copy link
Member

@EsdrasVP EsdrasVP commented Sep 4, 2018 •
edited
Loading

This PR is able to:

  • Keep Computer System on PhysicalStorage for any provider to use and allow it to have Computer Systems through Canisters.

@EsdrasVP
Copy link
Member Author

EsdrasVP commented Sep 4, 2018

@miq-bot assign @agrare

@EsdrasVP EsdrasVP mentioned this pull request Sep 4, 2018
Merged
@EsdrasVP EsdrasVP changed the title Fixing Computer System association on Physical Storage Fixing Computer System pluralization on Physical Storage Sep 4, 2018
@EsdrasVP EsdrasVP changed the title Fixing Computer System pluralization on Physical Storage [WIP] Fixing Computer System pluralization on Physical Storage Sep 4, 2018
@miq-bot miq-bot added the wip label Sep 4, 2018
@EsdrasVP EsdrasVP changed the title [WIP] Fixing Computer System pluralization on Physical Storage Fixing Computer System pluralization on Physical Storage Sep 4, 2018
@miq-bot miq-bot removed the wip label Sep 4, 2018
agrare
agrare reviewed Sep 4, 2018
View reviewed changes
app/models/physical_storage.rb
@@ -9,10 +9,6 @@ class PhysicalStorage < ApplicationRecord
has_one :asset_detail, :as => :resource, :dependent => :destroy, :inverse_of => false

has_many :canisters, :dependent => :destroy, :inverse_of => false
has_many :computer_system, :through => :canisters
has_many :hardware, :through => :computer_system
has_many :guest_devices, :through => :canisters
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 delete these? Thought you just had to change to `:computer_systems

Copy link
Member Author

Choose a reason for hiding this comment

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

@agrare Initially I thought that just fix the pluralization would do the job, but talking with the team we noted that it didn't make sense to let these relationships on PhysicalStorage since they're already in Canister. So I updated this PR title and description to fit the changes.

Copy link
Contributor

@himdel himdel Sep 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

from the UI side, no problem with this, the failing test caused by the other PR was really just the rename, fixed in ManageIQ/manageiq-ui-classic#4602 (so we're no longer touching PhysicalStorage#computer_system at all)

Copy link
Member

Choose a reason for hiding this comment

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

👍 sorry for the disruption @himdel

agrare
agrare reviewed Sep 4, 2018
View reviewed changes
app/models/ems_refresh/save_inventory_physical_infra.rb Outdated
@@ -101,7 +101,7 @@ def save_physical_storages_inventory(ems, hashes, target = nil)
h[:physical_chassis_id] = h.delete(:physical_chassis).try(:[], :id)
end

child_keys = %i(computer_system asset_detail physical_disk canister)
Copy link
Member

Choose a reason for hiding this comment

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

So should there be a computer_system child_key in save_canisters_inventory?

@EsdrasVP EsdrasVP changed the title Fixing Computer System pluralization on Physical Storage Fixing Computer System relationship on Physical Storage Sep 5, 2018
@EsdrasVP EsdrasVP changed the title Fixing Computer System relationship on Physical Storage [WIP] Fixing Computer System relationship on Physical Storage Sep 5, 2018
@EsdrasVP EsdrasVP changed the title [WIP] Fixing Computer System relationship on Physical Storage Fixing Computer System relationship on Physical Storage Sep 5, 2018
@agrare
Copy link
Member

agrare commented Sep 5, 2018

@EsdrasVP thinking about this a bit more, I think we should leave the has_one :computer_system on a physical_storage. If we want we can add a computer_system to canisters and add a has_many :canister_computer_systems, :through => :canisters but since not all storages will have canisters we should not remove computer_system from physical_storage.

@EsdrasVP
Copy link
Member Author

EsdrasVP commented Sep 5, 2018

@EsdrasVP thinking about this a bit more, I think we should leave the has_one :computer_system on a physical_storage. If we want we can add a computer_system to canisters and add a has_many :canister_computer_systems, :through => :canisters but since not all storages will have canisters we should not remove computer_system from physical_storage.

@agrare Makes sense, I'm changing to this and on the provider PR I moved the computer system parse from PhysicalStorage to Canister.

@EsdrasVP EsdrasVP force-pushed the fix_physical_storage_model branch 2 times, most recently from 51f6518 to 79f4547 Compare September 5, 2018 12:51
agrare
agrare reviewed Sep 5, 2018
View reviewed changes
app/models/physical_storage.rb
has_many :physical_disks, :dependent => :destroy, :inverse_of => :physical_storage

has_one :computer_system, :as => :managed_entity, :dependent => :destroy
Copy link
Member

Choose a reason for hiding this comment

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

You're still missing the previous relations (:hardware, :guest_devices)

IMO this should go back to before #17706 and just add has_many :canisters and has_many :canister_computer_systems

Copy link
Member Author

Choose a reason for hiding this comment

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

@agrare Fixed it, and put :guest_devices through :hardware in order to be more general for other providers.

@miq-bot
Copy link
Member

miq-bot commented Sep 5, 2018

Checked commit EsdrasVP@a774358 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

app/models/physical_storage.rb

@EsdrasVP EsdrasVP closed this Sep 5, 2018
@EsdrasVP EsdrasVP reopened this Sep 5, 2018
@agrare agrare merged commit 1d98cbe into ManageIQ:master Sep 5, 2018
@agrare agrare added this to the Sprint 94 Ending Sept 10, 2018 milestone Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himdel himdel himdel left review comments

@agrare agrare agrare approved these changes

Assignees

@agrare agrare

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

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