-
Notifications
You must be signed in to change notification settings - Fork 897
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 Storage Model #17380
Save Physical Storage Model #17380
Conversation
127620a
to
642fa3a
Compare
This pull request is not mergeable. Please rebase and repush. |
642fa3a
to
9ede302
Compare
This pull request is not mergeable. Please rebase and repush. |
6bc0e38
to
f96e152
Compare
LGTM @agrare Please review. Note that there are no specs going through this yet, otherwise this would fail since I just merged the database change. I expect that the provider refresh will ultimately test this. |
It looks like the specs are here, https://github.com/ManageIQ/manageiq-providers-lenovo/pull/170/files#diff-98cc26e5e52ffafdabcfc6bd8f1ea1d3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you're missing an association
I also get errors when trying to run with your lenovo PR and this applied:
NameError:
uninitialized constant XClarityClient::Storage
Is there a gem change also needed?
app/models/physical_storage.rb
Outdated
@@ -0,0 +1,9 @@ | |||
class PhysicalStorage < ApplicationRecord | |||
belongs_to :ext_management_system, :foreign_key => :ems_id, :inverse_of => :physical_storages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add the physical_storages association to ext_management_system for this to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fixed it to following what was done in ManageIQ/manageiq#17423. Now Physical Infra Manager have many physical_storages
.
end | ||
|
||
child_keys = %i(computer_system asset_detail) | ||
save_inventory_multi(ems.physical_storages, hashes, deletes, [:ems_ref], child_keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here there is no ems.physical_storages yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it, now it should work.
f96e152
to
73dc83e
Compare
73dc83e
to
dd555a9
Compare
Checked commit EsdrasVP@dd555a9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still having issues with ManageIQ/manageiq-providers-lenovo#170 but they look like they are specific to that PR. The save_inventory part of this looks good and I tested the associations and they work as expected.
This PR is able to save physical storage model into ManageIQ. Likewise Switches, Servers, Racks and Chassis, the Storage is a physical component, which is why I labeled it PhysicalStorage in model.
Depends on: