Add PhysicalDisks into Canisters #18053
Conversation
There was a problem hiding this comment.
Hold pending specs in ManageIQ/manageiq-providers-lenovo#235 (review)
EsdrasVP
force-pushed
the
add_physical_disks_to_canisters
branch
2 times, most recently
from
6f013cc to
6e0f220
Compare
EsdrasVP
changed the title
EsdrasVP
force-pushed
the
add_physical_disks_to_canisters
branch
from
6e0f220 to
a573ecf
Compare
EsdrasVP
changed the title
EsdrasVP
changed the title
| return if hashes.nil? | ||
|
|
||
| # Update the associated ids | ||
| hashes.each do |h| | ||
| h[:physical_storage_id] = h.delete(:physical_storage).try(:[], :id) |
There was a problem hiding this comment.
Is the intention to be able to save physical disks from both physical_storages and canisters? I think this is a good idea and it looks like you're going that direction by changing save_physical_disks_inventory(physical_storage, hashes) to save_physical_disks_inventory(parent, hashes) but then you need to keep this foreign_key lookup not replace it with the canisters one.
There was a problem hiding this comment.
You also need to add :physical_disks to the child_keys in save_canisters_inventory
There was a problem hiding this comment.
Is the intention to be able to save physical disks from both physical_storages and canisters? I think this is a good idea and it looks like you're going that direction by changing
save_physical_disks_inventory(physical_storage, hashes)tosave_physical_disks_inventory(parent, hashes)but then you need to keep this foreign_key lookup not replace it with the canisters one.
@agrare I only left a canister foreign_key because, even though we are letting it abstract enough for physical_disks to have any parent, currently they'll always be inside of a physical_storage, what will change is that sometimes it is inside a canister that is inside a physical_storage. It's a little bit confusing at first glance, but this allow us to show all disks inside a storage even if it is inside it through another component like a canister.
There was a problem hiding this comment.
currently they'll always be inside of a
physical_storage
That's true of the lenovo provider, but might not be true of all physical infra providers right? Not all physical storages have canisters, in fact I think the vast majority do not no?
There was a problem hiding this comment.
That's true of the lenovo provider, but might not be true of all physical infra providers right? Not all physical storages have canisters, in fact I think the vast majority do not no?
Yeah, that's what bothers me. Is there another way for us to access a physical_disk that is inside a canister from a storage? Because the way foreign_key is used on the other components doesn't allow us to access physical_disks as an attribute for both storage and canister at the same time.
There was a problem hiding this comment.
I think that's okay, from a refresh POV we don't need to be able to access both because a disk can only belong to one or the other.
There was a problem hiding this comment.
but then you need to keep this foreign_key lookup not replace it with the canisters one.
A physical_disk should always belong to a physical_storage and saved through association, so I guess we can keep only the lookup for a canister here, so when the canister is present save this disk for both storage and canister.
You also need to add
:physical_disksto thechild_keysinsave_canisters_inventory
We don't plan to save a physical_disk to a canister through association as explained above
Not all physical storages have canisters, in fact I think the vast majority do not no?
Correct, but then we only save a canister to physical_storage when it really exists, and the physical_disk will always be saved to the physical_storage regardless.
There was a problem hiding this comment.
@felipedf I'm confused if a physical disk should belong to a physical_storage instead of a canister then why are we doing this?
We could revert this and just add a simple canister id lookup to the old method.
There was a problem hiding this comment.
The physical_disk will be saved to the storage through the association and saved to the canister with a simple id lookup as you said. I reverted to the old method to use physical_storage instead parent
There was a problem hiding this comment.
Okay let me know when you have the specs in the lenovo PR ready and passing with this applied locally
EsdrasVP
force-pushed
the
add_physical_disks_to_canisters
branch
2 times, most recently
from
ff331ed to
2da8fa1
Compare
felipedf
force-pushed
the
add_physical_disks_to_canisters
branch
from
2da8fa1 to
5ab40e4
Compare
felipedf
force-pushed
the
add_physical_disks_to_canisters
branch
from
9ec48c8 to
1b364dc
Compare
|
Checked commits EsdrasVP/manageiq@5ab40e4~...1b364dc with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
| @@ -1,5 +1,6 @@ | |||
| class PhysicalDisk < ApplicationRecord | |||
| belongs_to :physical_storage, :foreign_key => :physical_storage_id, :inverse_of => :physical_disks | |||
| belongs_to :canister, :foreign_key => :canister_id, :inverse_of => :physical_disks | |||
There was a problem hiding this comment.
You don't need the foreign_key if the key matches the belongs_to
|
@felipedf can you add the canisters and physical_disks inventory collections to the base physical_infra_manager persister definition and fix the foreign_key for canisters in a follow-up PR? |
This PR is able to:
save_inventory_infrafor PhysicalDisksphysical_storagedeletion from physical disks inventory saving (bug fix)Depends on: