[WIP] Add Resource_pool UI for Cloud managers

[Guddetisandeep]

Dependent PR for enabling Resource Pool feature under Clouds section(Compute > Clouds > Resource Pools)

• manageiq Core changes
  Update Resource Pool Identifiers and UI for Cloud and Infrastructure Categorization manageiq#22879

• manageiq-ui-classic changes
  [WIP] Add Resource_pool UI for Cloud managers #8929

• manageiq-schema changes
  Update resource pool feature identifiers to infrastructure-specific manageiq-schema#718

• manageiq-api changes
  Update Resource Pool API to include type attribute manageiq-api#1248

Added Resource pool option for cloud providers and also showing the Resource pool option under compute > clouds > Cloud Resource pools

TODO:

• Fix the issue where selecting Resource Pools under cloud or Infrastructure in the compute section highlights both options. It should only highlight the relevant one based on the selection.

[Guddetisandeep]

When navigating to the resource pools of cloud providers, I encounter a situation where the displayed path at the top of the UI indicates "compute > Infrastructure > Resource Pools." This difference occurs because in the breadcrumb_options method of the resource_pool_controller, the logic seems to be configured to include "Infrastructure" as part of the breadcrumb when navigating to resource pools and here the :menu_section is set to :inf, as a result, the UI continues to highlight "Infrastructure > Resource Pools," even after navigating to compute > clouds > resource_pools.

Now, the question is, Do I need to create separate controllers like resource_pool_cloud_controller and resource_pool_infra_controller by writing modules. Making this decision involves considering potential changes to other files writing seperate files, making changes to routes and YAML configurations. Alternatively, Is there a possibility of implementing conditional logic within the breadcrumb_options method and :menu_section in the existing resource_pool_controller to handle both cloud and infrastructure resource pools.

[agrare]

This looks at least very similar to the textual_resource_pools method in the ResourcePoolHelper could we reuse that one?

[Guddetisandeep]

The h[:link] options in the textual_resource_pools methods of both files looks different. Perhaps we can't reuse it

[jeffibm]

Link from ems_cloud_helper
/ems_cloud/5?display=resource_pools

Link from resource_pool_helper
/resource_pool/show/5?display=resource_pools

[agrare]

cc @jeffibm please review as well

[jeffibm]

Hey @Guddetisandeep , could you please add some screenshots in the description for sprint review documentation purpose.

[jeffibm]

Hey @Guddetisandeep , could you please add some screenshots in the description for sprint review documentation purpose.

I just saw them in the comment in here #8929 (comment)

[jeffibm]

Hey @Fryguy , do we need to change the branch name of this PR?

[agrare]

do we need to change the branch name of this PR?

@jeffibm do you mean because it is based off of his fork's master branch? That is technically okay from our point of view it just makes for a weird workflow for @Guddetisandeep if he works on more than 1 PR at a time in this repo.

[jeffibm]

Hey @Guddetisandeep , we have a failing spec

[Guddetisandeep]

Hey @Guddetisandeep , we have a failing spec

I have updated the PR with the modified spec_file that should address the issue with the failing spec

[jaywcarman]

I have updated the PR with the modified spec_file that should address the issue with the failing spec

Thanks @Guddetisandeep! fyi when you force push one of the merges has to approve the workflow again, but I think if you just push fast-forward commits it will run without approval.

[jaywcarman]

I spent some time taking a closer look at this today and have a couple of questions:

1. Resource pools that belong to a CloudManager won't have any host information. Should we remove all of the "Total Host [CPU Resources / Memory / CPUs / CPU Cores]" textual helpers?
2. Same question for "Parent [Datacenter / Cluster / Host]" under relationships.
3. For PowerVS, resource pools can belong to a PlacementGroup. I wonder if this is generic enough to be added here?

[jaywcarman]

Here's a screenshot showing what this looks like with data from the new resource_pools columns:

[Guddetisandeep]

Here's a view of the Resource Pool page for Cloud Manager with the textual options under Properties and Relationships disabled when they have no data

[Guddetisandeep]

Currently, clicking on compute > clouds > resource_pools highlights both the 'Resource Pools' options under 'cloud' and 'infrastructure,' displaying resource pool data from both sources. However, when selecting either Resource Pools option, we should only see resource pools belonging to the specific Cloud/Infra manager associated with that option. Do I need to use two separate APIs to retrieve resource pool data for Cloud/Infra manager?

[jeffibm]

The line -

manageiq-ui-classic/app/presenters/menu/default_menu.rb

Line 97 in 978a9e1

Menu::Item.new('resource_pool', N_('Resource Pools'), 'resource_pool', {:feature => 'resource_pool_show_list'}, '/resource_pool/show_list'),

Menu::Item.new('resource_pool',

identifies the navigation selection. so you might want to change it a unique name.

[Guddetisandeep]

I changed the identifier name to Menu::Item.new('resource_pool_cloud') for the cloud section, and for the infrastructure_menu_section, I changed it to Menu::Item.new('resource_pool_infra'). After clicking on these two menus, I noticed that neither of them gets highlighted, although they function correctly and display data. Is it also necessary to make similar changes in the miq_product_features.yml file?

[agrare]

@jeffibm ^

[agrare]

Hey @jeffibm can you give this a re-review?

[Fryguy]

I noticed the URL is changing. I wonder if this affects shortcuts? I can't remember if resource pools was a page that was a shortcut candidate.

[Guddetisandeep]

Should separate entries be added for 'resource_pool_cloud' and 'resource_pool_infra' in the miq_shortcuts.yaml file to accommodate the URL changes?

[Fryguy]

did the resource_pools route go away with this change? I was expecting a delete.

[Fryguy]

How does this affect the API? There's currently a /api/resource_pools endpoint, I believe. For other endpoints, subcollections suffice, or you can pass a "type" field in. For vms and templates, there is a "cloud" boolean field that can be passed in. @agrare Thoughts here?

[Fryguy]

But, I can't figure how to handle the situation when ResourcePoolCloudController and ResourcePoolInfraController has been inherited from the ResourcePoolController in the PR.

@jeffibm Do we do this inheriting elsewhere? I don't recall doing that, and so introducing that here has me a little worried, but if we've done that elsewhere then we have a pattern to follow.

[agrare]

For vms and templates, there is a "cloud" boolean field that can be passed in. @agrare Thoughts here?

Yeah I never liked the cloud column on vms since it is pretty limiting, typically we'd pass in a ?type=IntermediateClass to the API to filter that way, so the ResourcePoolInfraController could query e.g. /api/resource_pools?type="ManageIQ::Providers::InfraManager::ResourcePool and the cloud controller /api/resource_pools?type="ManageIQ::Providers::CloudManager::ResourcePool.

These intermediate classes already exist we'd just have to make sure that all providers subclass from these properly and not from the base ::ResourcePool class.

[Fryguy]

@jaywcarman Can we have a call do discuss this? The patterns introduced in this PR don't follow patterns we have on other pages and it feels like it's not structured correctly, but perhaps I'm misunderstanding the intent.

[jeffibm]

hey @Guddetisandeep , could you please squash the commits.

[kbrock]

Per mentioned in ManageIQ/manageiq-api#1248, would you update your api queries to use collection_class instead of the suggested api endpoint introduced?

[jeffibm]

But, I can't figure how to handle the situation when ResourcePoolCloudController and ResourcePoolInfraController has been inherited from the ResourcePoolController in the PR.

@jeffibm Do we do this inheriting elsewhere? I don't recall doing that, and so introducing that here has me a little worried, but if we've done that elsewhere then we have a pattern to follow.

When navigating to the resource pools of cloud providers, I encounter a situation where the displayed path at the top of the UI indicates "compute > Infrastructure > Resource Pools." This difference occurs because in the breadcrumb_options method of the resource_pool_controller, the logic seems to be configured to include "Infrastructure" as part of the breadcrumb when navigating to resource pools and here the :menu_section is set to :inf, as a result, the UI continues to highlight "Infrastructure > Resource Pools," even after navigating to compute > clouds > resource_pools.

Now, the question is, Do I need to create separate controllers like resource_pool_cloud_controller and resource_pool_infra_controller by writing modules. Making this decision involves considering potential changes to other files writing seperate files, making changes to routes and YAML configurations. Alternatively, Is there a possibility of implementing conditional logic within the breadcrumb_options method and :menu_section in the existing resource_pool_controller to handle both cloud and infrastructure resource pools.

the menu items seem to be working now -

To get this working, I can see that, you changed the :feature in default_menu.rb from
Menu::Item.new('resource_pool_cloud', N_('Resource Pools'), 'resource_pool_cloud', {:feature => 'resource_pool_show_list'}, '/resource_pool_cloud/show_list'),
to
Menu::Item.new('resource_pool_cloud', N_('Cloud Resource Pools'), 'resource_pool_cloud', {:feature => 'resource_pool_cloud_show_list'}, '/resource_pool_cloud/show_list'),
and use the same in its controller and manageiq/miq_product_features.yml

Also the breadcrumbs now looks like -
Compute / Infrastructure / Resource Pools and Compute / Cloud / Resource Pools.
Should we change to
Compute / Infrastructure / Infrastructure Resource Pools and Compute / Cloud / Cloud Resource Pools

[jeffibm]

We have a failing spec in bundle exec rspec spec/controllers/resource_pool_controller_spec.rb

Probably because the ResourcePoolController was renamed to ResourcePoolInfraController. I guess it's better to remove this file and create 2 new specs for the cloud and infra controllers.

[agrare]

@miq-bot cross-repo-tests ManageIQ/manageiq#22879

[agrare]

@Guddetisandeep cross-repo test isn't running properly, but I tried merging your UI and core PRs locally and I'm seeing the same failures as we see here so the errors aren't resolved by just having your core PR applied.

[Guddetisandeep]

@Guddetisandeep cross-repo test isn't running properly, but I tried merging your UI and core PRs locally and I'm seeing the same failures as we see here so the errors aren't resolved by just having your core PR applied.

Yes, I see some errors in this repo PR. I will work on resolving them and update the PR accordingly.

[miq-bot]

Checked commit Guddetisandeep@995bd1f with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
38 files checked, 102 offenses detected

app/helpers/application_helper/toolbar/resource_pool_cloud_center.rb

• ❗ - Line 15, Col 113 - Layout/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 18, Col 3 - Layout/FirstArrayElementIndentation - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❗ - Line 20, Col 5 - Layout/FirstArrayElementIndentation - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 30, Col 32 - Layout/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 26 - Layout/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 38, Col 3 - Layout/FirstArrayElementIndentation - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❗ - Line 3, Col 5 - Layout/FirstArrayElementIndentation - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar/resource_pool_infras_center.rb

• ❗ - Line 20, Col 32 - Layout/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 23, Col 3 - Layout/FirstArrayElementIndentation - Indent the right bracket the same as the first position after the preceding left parenthesis.
• ❗ - Line 25, Col 5 - Layout/FirstArrayElementIndentation - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 3, Col 5 - Layout/FirstArrayElementIndentation - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 41, Col 32 - Layout/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 50, Col 32 - Layout/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 53, Col 3 - Layout/FirstArrayElementIndentation - Indent the right bracket the same as the first position after the preceding left parenthesis.

app/helpers/ems_cloud_helper/textual_summary.rb

• ❗ - Line 135, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 135, Col 44 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 139, Col 33 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.

app/helpers/resource_pool_cloud_helper/textual_summary.rb

• ❗ - Line 101, Col 12 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 102, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 114, Col 11 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 118, Col 101 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 124, Col 11 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 128, Col 101 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 134, Col 11 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 138, Col 101 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 144, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 145, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 150, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 151, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 156, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 157, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 162, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 163, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 168, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 169, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 174, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 175, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 180, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 181, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 186, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 187, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 192, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 193, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 198, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 199, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 204, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 205, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 210, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 211, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 216, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 217, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 46, Col 37 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 51, Col 19 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 51, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 53, Col 41 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 57, Col 19 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 57, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 59, Col 37 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 5, Col 1 - Layout/TrailingWhitespace - Trailing whitespace detected.
• ❗ - Line 63, Col 19 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 63, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 65, Col 38 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 69, Col 19 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 69, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 71, Col 38 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 75, Col 80 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 79, Col 79 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 7, Col 18 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 83, Col 19 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 83, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 84, Col 79 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 88, Col 15 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 89, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.

app/helpers/resource_pool_infra_helper/textual_summary.rb

• ❗ - Line 102, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 118, Col 101 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 128, Col 101 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 138, Col 101 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 204, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 205, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 210, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 211, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 216, Col 13 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 217, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 51, Col 19 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 51, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 57, Col 19 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 57, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 63, Col 19 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 63, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 69, Col 19 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 69, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 83, Col 19 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.
• ❗ - Line 83, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 89, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.

app/helpers/vm_helper/textual_summary.rb

• ❗ - Line 259, Col 10 - Rails/HelperInstanceVariable - Do not use instance variables in helpers.

app/views/resource_pool_cloud/show.html.haml

• ⚠️ - Line 2 - Line is too long. [97/80]

app/views/resource_pool_infra/_config.html.haml

• ⚠️ - Line 4 - Avoid using instance variables in partials views

app/views/resource_pool_infra/show.html.haml

• ⚠️ - Line 1 - id attribute must be in lisp-case
• ⚠️ - Line 2 - Line is too long. [97/80]
• ⚠️ - Line 3 - Line is too long. [88/80]
• ⚠️ - Line 7 - Line is too long. [81/80]

app/views/resource_pool_infra/show_list.html.haml

• ⚠️ - Line 1 - id attribute must be in lisp-case