Parse Physical Chassis #149
Conversation
|
The plural and singular spellings of chassis are the same. Can you change all instances of 'chassi' or 'Chassi' to 'chassis' or 'chassis'? |
caiocmpaes
changed the title
caiocmpaes
changed the title
|
This pull request is not mergeable. Please rebase and repush. |
caiocmpaes
changed the title
| class PhysicalChassisParser < ComponentParser | ||
| class << self | ||
| # parse a node object to a hash with physical chassis data | ||
| # @param [XClarityClient::Node] node - object containing physical chassis data |
There was a problem hiding this comment.
I think that you will want to change the @param to its new name chassi_hash and add some comment about the rack param.
| @@ -13,12 +13,13 @@ class << self | |||
| # | |||
| # @return [Hash] containing the physical server information | |||
| # | |||
| def parse_physical_server(node, rack = nil) | |||
| def parse_physical_server(node_hash, rack = nil, chassis = nil) | |||
There was a problem hiding this comment.
Please, also update the param name on doc comment
There was a problem hiding this comment.
Would be good add something about the new params rack and chassis, too
| @@ -0,0 +1,28 @@ | |||
| module ManageIQ::Providers | |||
| class Lenovo::PhysicalInfraManager::PhysicalChassis < ::PhysicalChassis | |||
| delegate :product_name, | |||
There was a problem hiding this comment.
I'm wondering if this delegate strategy is good here, seems like it is a good thing when you want to support some code that already access this properties like chassis.product_name, for example, which is not the case once it is a new component. What do you think @agrare , is there some pattern about this strategy?
| expect(result[:physical_chassis].size).to eq(1) | ||
| end | ||
|
|
||
| it 'will parse physical chassi fields' do |
There was a problem hiding this comment.
I think that you wanted to write chassis here
| end | ||
|
|
||
| it 'will parse physical chassi fields' do | ||
| physical_chassi = result[:physical_chassis].first |
There was a problem hiding this comment.
Also here should be physical_chassis
| physical_rack = result[:physical_racks].first | ||
| physical_chassis = result[:physical_chassis].first | ||
|
|
||
| expect(physical_chassis[:physical_rack][:ems_ref]).to eq(physical_rack[:ems_ref]) |
There was a problem hiding this comment.
Seems like this is a redundant test if you are verifying the equality between physical_chassis[:physical_rack] and physical_rack as you do bellow.
| physical_server = @result[:physical_servers][1] | ||
| physical_chassis = @result[:physical_chassis][0] | ||
|
|
||
| expect(physical_server[:physical_chassis][:ems_ref]).to eq(physical_chassis[:ems_ref]) |
There was a problem hiding this comment.
Same redundancy that I commented before 😄
|
This pull request is not mergeable. Please rebase and repush. |
|
@douglasgabriel I fixed all the corrections that you pointed. Thank you! Also, I rebased my code after the refactoring of "relative requires". |
|
This pull request is not mergeable. Please rebase and repush. |
|
@caiocmpaes can you rebase this PR? |
|
Thanks @caiocmpaes , LGTM now 👍 Only needing the rebase that @agrare mentioned |
|
Rebased @agrare |
|
@caiocmpaes this changeset still does not apply cleanly |
|
Now Its is rebased again, @agrare |
|
Checked commit https://github.com/caiocmpaes/manageiq-providers-lenovo/commit/69e65780de780eafd353bb7f1aa1a5f3c487145b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 **
|
Depends on:
ManageIQ/manageiq-schema#183 [Merged]