Physical infra pages

[juliancheal]

Adds the Physical Infra pages.

There are still issues.

• Missing toolbar at the top

• Such as on the add page, need to add types of physical infra

• Discover page issue

This PR links to ManageIQ/manageiq#13587

[AparnaKarve]

Need the Gemfile changes in ManageIQ/manageiq#11846 for tests to pass.

[miq-bot]

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

[juliancheal]

[juliancheal]

@miq-bot remove_label wip

[juliancheal]

@miq-bot assign @dclarizio

[juliancheal]

Hey @dclarizio could you see about getting this merged. It's all working technically and been reviewed by both @AparnaKarve and myself.

[dclarizio]

@juliancheal will look at this today and discuss with @AparnaKarve

[dclarizio]

@juliancheal asking @himdel and @mzazrivec to review this to make sure we're following some of the latest UI guidelines.

[martinpovolny]

There's exactly 0 tests in this PR. And there are new controllers. Please include at least one test for every new screen.

[himdel]

This introduces another copy of angular topology controler - won't force you to do the refactoring now (since we already have 5 of them), but.. created https://www.pivotaltracker.com/story/show/140220953

[himdel]

Uh, and here I thought mixins are supposed to be for shared code.

The pattern of having controllers A and B include mixin M, which checks controller name and does something based on it .. feels silly (and more importantly, wrong). Can we just call a method that's defined in all those controllers?

[AparnaKarve]

Agree, there is a good scope for a refactor here, but just not in this PR.
Note that most of the code here is adhering to the existing code pattern, which has gotten crazy over the last few months when folks keep adding more and more controllers here.

[martinpovolny]

Well if we keep saying that and postponing cleanups then we are doomed.

The need to add new code that follows a bad pattern should be the trigger for refactoring :-(

[AparnaKarve]

The need to add new code that follows a bad pattern should be the trigger for refactoring :-(

@martinpovolny Could not agree more.
However, it goes without saying that the refactoring effort should not be part of this (already gigantic) PR. That said, what I can offer here is this commit that I just pushed:
1dd3e43
It renders the ems_physical_infra-specific json using it's own controller method, making sure it does not add to the mess in the mixin.
Does that work?

[blomquisg]

Well if we keep saying that and postponing cleanups then we are doomed.

This is a fine sentiment, but this ems_common has been bad for a very very long time. Pinning this level of cleanup on this PR is a mistake, imo.

Refactoring ems_common to be what it really should be (common code used by multiple controllers) is a monumental effort. Best suited for a separate dedicated PR that can be reviewed in that specific context.

[himdel]

these 4 lines are obsolete, this functionality got removed in #204 (https://github.com/ManageIQ/manageiq-ui-classic/pull/204/files#diff-127a850b7d7b20173ad3fba88f8a25e3L203)

[juliancheal]

Got spot, tried to find all of these.

[himdel]

Really? Can't we just do i = [ textual_topology ]?

[blomquisg]

Sure? Until there's more than one...

In reality, this should be a private method somewhere hiding the details of building this "textual_" prefixed items since there's already 17[1] other occurrences of this exact line. And, if you want to optimize on whether there's only one item, then the private method would be a fantastic place to do that.

$ git grep -Ii "items.collect { |m| send(\"textual_#{m}" 
app/helpers/container_build_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/container_group_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/container_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/container_image_helper/textual_summary.rb:      items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/container_image_registry_helper/textual_summary.rb:      items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/container_node_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/container_project_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/container_replicator_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/container_route_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/container_service_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/container_template_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/ems_cloud_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/ems_container_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/ems_infra_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/ems_middleware_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/ems_network_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact
app/helpers/persistent_volume_helper/textual_summary.rb:    items.collect { |m| send("textual_#{m}") }.flatten.compact

[AparnaKarve]

Leaving this as-is for now unless this is absolutely an issue.

[himdel]

So... what is the @record that this deals with?

Seems like this is just copy-pasted from ems_infra, where the condition dealt specifically with openstack (introduced in ManageIQ/manageiq#3556) - and makes little sense here.

[AparnaKarve]

@record is a record of the new provider type -- ManageIQ::Providers::PhysicalInfraManager.
Not really sure if we can draw a connection here with Openstack or not or how closely this Provider type is related to ems_infra.
@juliancheal Any idea?

[juliancheal]

@himdel It looks like it's copy-pasta, because it is :) Ems Infra has been a good start to base Physical Infra on, but sometimes we do have unneeded code.

Didn't realise that's what @record here was used for, as there weren't any comments 😌

[himdel]

same here

[himdel]

Unknown .. missing translation (2 other occurences in this function)

[himdel]

This is obsolete and doesn't work anymore. Please see #274 on how to fix this.

Also, @juliancheal .. clearly this screen is not being tested since it doesn't work and the tests pass ;).

[AparnaKarve]

I believe the Lenovo guys did test the Textual Summary page, and AFAIK, everything seemed to work OK.
But we'll address the obsolete part.

[juliancheal]

Tests are still WIP :)

[juliancheal]

@martinpovolny @AparnaKarve yay finally green!

[Fryguy]

Any possibility to clean up all/most of the rubocops?

[juliancheal]

Any possibility to clean up all/most of the rubocops?

The current broken cops are in style with the rest of the UI. I think it would be best to leave those for now and fix in a later PR with other classes that are also wrong with this style.

[AparnaKarve]

Folks, we have been through many many rounds of rebasing and resolving merge conflicts.

Can someone please do the needful? Thanks.

[himdel]

missing i18n

[himdel]

should this be timeline_usage or 2 lines?

[miq-bot]

Some comments on commits https://github.com/juliancheal/manageiq-ui-classic/compare/219131e374178987571ed9b2b85959cabab51175~...3d7e8df215c9b57389d01ccbc81313aeca63f478

spec/controllers/ems_physical_infra_controller_spec.rb

• ⚠️ - 12 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

[miq-bot]

Checked commits https://github.com/juliancheal/manageiq-ui-classic/compare/219131e374178987571ed9b2b85959cabab51175~...3d7e8df215c9b57389d01ccbc81313aeca63f478 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
25 files checked, 92 offenses detected

app/controllers/ems_physical_infra_controller.rb

• ❗ - Line 31, Col 12 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id_filtered.
• ❕ - Line 28, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for ems_physical_infra_form_fields is too high. [34.29/20]

app/helpers/application_helper/toolbar/ems_physical_infra_center.rb

• ❗ - Line 19, Col 122 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 25, Col 12 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 32, Col 123 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 35, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❗ - Line 37, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 47, Col 32 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 52, Col 26 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 58, Col 101 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 61, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❗ - Line 63, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 75, Col 44 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 78, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❗ - Line 7, Col 10 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 80, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 91, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 92, Col 59 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 95, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❕ - Line 19, Col 121 - Metrics/LineLength - Line is too long. [123/120]
• ❕ - Line 32, Col 121 - Metrics/LineLength - Line is too long. [124/120]

app/helpers/application_helper/toolbar/ems_physical_infras_center.rb

• ❗ - Line 105, Col 29 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 108, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❗ - Line 17, Col 29 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 24, Col 45 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 31, Col 25 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 39, Col 28 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 48, Col 29 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 51, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❗ - Line 53, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 68, Col 29 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 76, Col 29 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 85, Col 29 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 88, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❗ - Line 90, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❕ - Line 15, Col 121 - Metrics/LineLength - Line is too long. [133/120]
• ❕ - Line 1, Col 1 - Metrics/ClassLength - Class has too many lines. [107/100]
• ❕ - Line 46, Col 121 - Metrics/LineLength - Line is too long. [134/120]

app/helpers/ems_physical_infra_helper/textual_summary.rb

• ❗ - Line 58, Col 17 - Style/SpaceAroundOperators - Operator => should be surrounded by a single space.
• ❗ - Line 58, Col 19 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 58, Col 49 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 58, Col 51 - Style/SpaceAroundOperators - Operator => should be surrounded by a single space.
• ❗ - Line 58, Col 53 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❕ - Line 58, Col 121 - Metrics/LineLength - Line is too long. [122/120]

app/presenters/menu/default_menu.rb

• ❗ - Line 80, Col 11 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 80, Col 138 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 80, Col 47 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 80, Col 89 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 81, Col 59 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 82, Col 9 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❕ - Line 80, Col 121 - Metrics/LineLength - Line is too long. [164/120]
• ❕ - Line 81, Col 121 - Metrics/LineLength - Line is too long. [143/120]

app/services/physical_infra_topology_service.rb

• ❕ - Line 26, Col 121 - Metrics/LineLength - Line is too long. [122/120]

app/views/ems_physical_infra/_form.html.haml

• ⚠️ - Line 10 - Prefer to_s over string interpolation.
• ⚠️ - Line 26 - Use hash rockets syntax.

app/views/ems_physical_infra/edit.html.haml

• ⚠️ - Line 1 - Prefer to_s over string interpolation.

app/views/ems_physical_infra/new.html.haml

• ⚠️ - Line 2 - Prefer to_s over string interpolation.

app/views/layouts/listnav/_ems_physical_infra.html.haml

• ⚠️ - Line 17 - Use @record.number_of(:physical_servers).zero? instead of @record.number_of(:physical_servers) == 0.

app/views/physical_infra_topology/show.html.haml

• ⚠️ - Line 1 - Use snake_case for variable names.

spec/controllers/ems_physical_infra_controller_spec.rb

• ❗ - Line 119, Col 42 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 120, Col 42 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 121, Col 84 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 126, Col 57 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 128, Col 84 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 133, Col 3 - Metrics/BlockLength - Block has too many lines. [79/25]
• ❗ - Line 145, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 146, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 147, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 148, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 149, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 150, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 151, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 152, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 153, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 158, Col 5 - Metrics/BlockLength - Block has too many lines. [30/25]
• ❗ - Line 161, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 162, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 163, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 164, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 165, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 166, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 167, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 168, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 169, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 1, Col 1 - Metrics/BlockLength - Block has too many lines. [193/25]
• ❗ - Line 24, Col 3 - Metrics/BlockLength - Block has too many lines. [47/25]
• ❗ - Line 58, Col 45 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❕ - Line 121, Col 121 - Metrics/LineLength - Line is too long. [128/120]
• ❕ - Line 128, Col 121 - Metrics/LineLength - Line is too long. [128/120]

spec/routing/ems_physical_infra_routing_spec.rb

• ❗ - Line 3, Col 1 - Metrics/BlockLength - Block has too many lines. [47/25]

[chessbyte]

@dclarizio @martinpovolny 2 requests here for future PRs:

• Please alphabetize this list
• Please move the list to a well-named constant so code is more readable

[martinpovolny]

@chessbyte : Toolbar chooser was extracted but not yet refactored :-(

[martinpovolny]

For the record: I don't think we should merge new code that has so many Rubocop issues. No matter it might match other places in the code. It's not the way forward and someone will have to fix it sooner or later anyway.

[himdel]

@juliancheal Not sure if that's a know problem or not but...

Clicking on "Validate" when adding an phinfra provider just returns success, without even trying. (Added the provider with 127.0.0.1 and port 80 and it didn't even show up in my access.log.)

[himdel]

@juliancheal , @AparnaKarve the textual summary still has a link to Topology, it even seems like there is a PhysicalInfraTopology controller, but the topology itself is not available from the menu, and, perhaps more importantly, completely broken right now :).

FYI ;)

[AparnaKarve]

the textual summary still has a link to Topology, it even seems like there is a PhysicalInfraTopology controller, but the topology itself is not available from the menu, and, perhaps more importantly, completely broken right now :).

@himdel The only way to get to Physical Infra Topology is via the summary screen -- so that's a known thing.

As far as the Topology itself goes, do you have enough data to get a decent Topology graph?
I never got a chance to play with this because I never had access to an actual Physical infra provider.

Can you also post a screenshot of your summary screen...I'm curious to know what Relationships you're seeing. Thanks.

[himdel]

only way to get to Physical Infra Topology is via the summary screen

OK, cool .. because all the others have a Topology entry in the menu too..

do you have enough data to get a decent Topology graph?

Not really, I just have that one provider, without any additional data :). (I just used the bug that you can just add it without validation, so... I have a provider, but that's it.)

[himdel]

@AparnaKarve this is my summary screen.. but not sure it helps :)