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

Save Canister Model #17706

Merged
merged 1 commit into from
Sep 4, 2018
Merged

Save Canister Model #17706

merged 1 commit into from
Sep 4, 2018

Conversation

EsdrasVP
Copy link
Member

@EsdrasVP EsdrasVP commented Jul 13, 2018 •
edited
Loading

This PR is able to:

  • Add Canister model
  • Create relationship between Canisters and PhysicalStorages
  • Save Canister inventory

Depends on:

@EsdrasVP EsdrasVP mentioned this pull request Jul 13, 2018
Merged
@miq-bot miq-bot added the wip label Jul 13, 2018
@EsdrasVP EsdrasVP force-pushed the save_canister_model branch 2 times, most recently from 251802e to 077a112 Compare July 20, 2018 14:03
@miq-bot
Copy link
Member

miq-bot commented Aug 13, 2018

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

@EsdrasVP EsdrasVP changed the title [WIP] Save Canister Model Save Canister Model Aug 30, 2018
@EsdrasVP
Copy link
Member Author

@miq-bot assign @agrare

@miq-bot miq-bot removed the wip label Aug 30, 2018
@agrare
Copy link
Member

agrare commented Aug 30, 2018

@EsdrasVP can you add an entry in the Persister for PhysicalInfraManager for this? https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/inventory/persister/builder/physical_infra_manager.rb
We need to keep these in sync because we're moving to graph refresh for all providers and lenovo will be moved soon.

@EsdrasVP
Copy link
Member Author

@EsdrasVP can you add an entry in the Persister for PhysicalInfraManager for this? https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/inventory/persister/builder/physical_infra_manager.rb
We need to keep these in sync because we're moving to graph refresh for all providers and lenovo will be moved soon.

@agrare I updated it with Canisters and PhysicalStorages, since both of them are related.

agrare
agrare reviewed Aug 30, 2018
View reviewed changes
app/models/ems_refresh/save_inventory_physical_infra.rb Outdated
#
# Saves the canister information of a storage
#
def save_canister_inventory(parent, hash)
Copy link
Member

Choose a reason for hiding this comment

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

If a canister can only belong to a physical storage it'd be better to call this physical_storage instead of parent. We use parent if something can belong to more than one type of parent.

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 Fear enough, I put it just in case this change in the future, but I'm changing it to physical_storage.

@EsdrasVP EsdrasVP closed this Aug 30, 2018
@EsdrasVP EsdrasVP reopened this Aug 30, 2018
@agrare
Copy link
Member

agrare commented Aug 30, 2018

@EsdrasVP looks like it is failing for an unrelated reason, I pinged the maintainer of those tests about it and when he fixes it i'll restart these tests and merge.

@EsdrasVP
Copy link
Member Author

@EsdrasVP looks like it is failing for an unrelated reason, I pinged the maintainer of those tests about it and when he fixes it i'll restart these tests and merge.

@agrare That's ok, I'll wait for a feedback.

@miq-bot
Copy link
Member

miq-bot commented Sep 3, 2018

Checked commit EsdrasVP@4fc696d 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. 🍪

@EsdrasVP
Copy link
Member Author

EsdrasVP commented Sep 3, 2018

@agrare Now I think everything is fine.

@agrare agrare merged commit 41b294f into ManageIQ:master Sep 4, 2018
@agrare agrare added this to the Sprint 94 Ending Sept 10, 2018 milestone Sep 4, 2018
himdel
himdel reviewed Sep 4, 2018
View reviewed changes
app/models/physical_storage.rb
has_many :guest_devices, :through => :hardware

has_many :canisters, :dependent => :destroy, :inverse_of => false
has_many :computer_system, :through => :canisters
Copy link
Contributor

@himdel himdel Sep 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

this looks surprising.. shouldn't this be computer_systems? (plural because has_many..)

Copy link
Member

Choose a reason for hiding this comment

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

Ugh you're right I missed that, @EsdrasVP can you push a PR to fix this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for that, fixed it on ManageIQ/manageiq#17945

@himdel himdel mentioned this pull request Sep 5, 2018
Merged
@agrare agrare mentioned this pull request Sep 5, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

@himdel himdel himdel left review comments

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 @miq-bot @agrare @himdel