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.

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

Add physical racks and chassis #33

Merged
merged 2 commits into from
Sep 26, 2018

Conversation

tadeboro
Copy link
Contributor

@tadeboro tadeboro commented Sep 20, 2018

Depends on #32

@miq-bot add_label wip

@miq-bot miq-bot changed the title Add physical racks and chassis [WIP] Add physical racks and chassis Sep 20, 2018
@miq-bot miq-bot added the wip label Sep 20, 2018
@miq-bot
Copy link
Member

miq-bot commented Sep 24, 2018

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

@tadeboro tadeboro force-pushed the add-physical-racks-and-chassis branch from 733e690 to aa08797 Compare September 24, 2018 20:59
@tadeboro
Copy link
Contributor Author

@miq-bot remove_label wip
@miq-bot assign @agrare

@miq-bot miq-bot changed the title [WIP] Add physical racks and chassis Add physical racks and chassis Sep 24, 2018
@miq-bot miq-bot removed the wip label Sep 24, 2018
@tadeboro
Copy link
Contributor Author

@miq-bot add_label hammer/yes

end

def physical_chassis
rf_client.Chassis.Members.reject { |c| c.ChassisType == "Rack" }
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this could be cached so you don't have to make the same call twice.
Could you add a private method chassis_members where you @chassis_members ||= rf_client.Chassis.Members?
Then you can just chassis_members.select / chassis_members.reject

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redfish client already does caching on its own, so adding another layer of cache is not strictly necessary since data is already only fetched once. But I can add it if you feel that this would make things more obvious.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay didn't know this

next unless enclosure_id

rack = persister.physical_racks.lazy_find(enclosure_id)
server.assign_attributes(:physical_rack => rack)
Copy link
Member

Choose a reason for hiding this comment

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

Hm since you're doing the .build right above you can probably just do this above and pass these in to the .build arg.

:ems_ref => c["@odata.id"],
:health_state => c.Status.Health,
:name => c.Id,
:type => "ManageIQ::Providers::Redfish::PhysicalInfraManager::PhysicalChassis",
Copy link
Member

Choose a reason for hiding this comment

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

Try leaving the type out and see if it gets the right STI class, we should be automatically building this now.

next unless parent_id

parent_chassis = persister.physical_chassis.lazy_find(parent_id)
chassis.assign_attributes(:parent_physical_chassis => parent_chassis)
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment here you should be able to just pass these into the build, instead of the next you can just do parent_chassis = persister.physical_chassis.lazy_find(parent_id) if parent_id

def physical_chassis_details
collector.physical_chassis.each do |c|
chassis = persister.physical_chassis.lazy_find(c["@odata.id"])
location = get_chassis_location(c)
Copy link
Member

Choose a reason for hiding this comment

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

More like this 👍

@agrare agrare added the enhancement New feature or request label Sep 26, 2018
@tadeboro tadeboro force-pushed the add-physical-racks-and-chassis branch from aa08797 to 1bc87c8 Compare September 26, 2018 15:49
@miq-bot
Copy link
Member

miq-bot commented Sep 26, 2018

Checked commits xlab-si/manageiq-providers-redfish@2f6bcc7~...1bc87c8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
14 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Yaml - missing config files

@agrare agrare merged commit 1bc87c8 into ManageIQ:master Sep 26, 2018
agrare added a commit that referenced this pull request Sep 26, 2018
@agrare agrare added this to the Sprint 96 Ending Oct 8, 2018 milestone Sep 26, 2018
simaishi pushed a commit that referenced this pull request Oct 1, 2018
Add physical racks and chassis

(cherry picked from commit e691021)
@simaishi
Copy link
Contributor

simaishi commented Oct 1, 2018

Hammer backport details:

$ git log -1
commit 934aff7851b3f92a762501299ba28283fdb50bfb
Author: Adam Grare <[email protected]>
Date:   Wed Sep 26 12:12:52 2018 -0400

    Merge pull request #33 from xlab-si/add-physical-racks-and-chassis
    
    Add physical racks and chassis
    
    (cherry picked from commit e691021147f5f876f53dd7f0302cd452f8703771)

@tadeboro tadeboro deleted the add-physical-racks-and-chassis branch October 25, 2018 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hammer/backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants