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 Resource Pool UI for Cloud managers #8929

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

Guddetisandeep
Copy link
Contributor

@Guddetisandeep Guddetisandeep commented Oct 4, 2023

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

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 Guddetisandeep requested a review from a team as a code owner October 4, 2023 08:51
@miq-bot miq-bot added the wip label Oct 4, 2023
@Guddetisandeep
Copy link
Contributor Author

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.

Screenshot 2023-10-05 135935 Screenshot 2023-10-05 135948

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.

Comment on lines 134 to 143
def textual_resource_pools
num = @record.try(:resource_pools) ? @record.number_of(:resource_pools) : 0
h = {:label => _('Resource Pools'), :icon => "pficon pficon-resource-pool", :value => num}
if num.positive? && role_allows?(:feature => "resource_pool_show_list")
h[:title] = _("Show all Resource Pools")
h[:link] = ems_cloud_path(@record.id, :display => 'resource_pools')
end
h
end

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@jeffibm jeffibm Oct 17, 2023

Choose a reason for hiding this comment

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

Link from ems_cloud_helper
/ems_cloud/5?display=resource_pools

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

@agrare
Copy link
Member

agrare commented Oct 12, 2023

cc @jeffibm please review as well

@jeffibm
Copy link
Member

jeffibm commented Oct 13, 2023

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

@jeffibm
Copy link
Member

jeffibm commented Oct 13, 2023

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
Copy link
Member

jeffibm commented Oct 13, 2023

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

@agrare
Copy link
Member

agrare commented Oct 16, 2023

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
Copy link
Member

jeffibm commented Oct 17, 2023

Hey @Guddetisandeep , we have a failing spec
image

@Guddetisandeep
Copy link
Contributor Author

Hey @Guddetisandeep , we have a failing spec image

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

@jaywcarman
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@Guddetisandeep
Copy link
Contributor Author

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
Screenshot 2023-11-07 134955

@Guddetisandeep
Copy link
Contributor Author

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?

Screenshot 2023-11-07 161404 Screenshot 2023-11-07 161423

@jeffibm
Copy link
Member

jeffibm commented Jan 2, 2024

The line -

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.

@@ -84,6 +84,7 @@ def clouds_menu_section
Menu::Item.new('orchestration_stack', N_('Stacks'), 'orchestration_stack', {:feature => 'orchestration_stack_show_list'}, '/orchestration_stack/show_list'),
Menu::Item.new('auth_key_pair_cloud', N_('Key Pairs'), 'auth_key_pair_cloud', {:feature => 'auth_key_pair_cloud_show_list'}, '/auth_key_pair_cloud/show_list'),
Menu::Item.new('placement_group', N_('Placement Groups'), 'placement_group', {:feature => 'placement_group_show_list'}, '/placement_group/show_list'),
Menu::Item.new('resource_pool_cloud', N_('Resource Pools'), 'resource_pool_cloud', {:feature => 'resource_pool_show_list'}, '/resource_pool_cloud/show_list'),
Copy link
Contributor Author

@Guddetisandeep Guddetisandeep Jan 4, 2024

Choose a reason for hiding this comment

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

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?

Screenshot 2024-01-04 200750

Copy link
Member

Choose a reason for hiding this comment

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

@agrare
Copy link
Member

agrare commented Jan 25, 2024

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

@jeffibm
Copy link
Member

jeffibm commented Apr 5, 2024

hey @Guddetisandeep , could you please squash the commits.

@kbrock
Copy link
Member

kbrock commented Apr 5, 2024

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
Copy link
Member

jeffibm commented Apr 8, 2024

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.

Screenshot 2023-10-05 135935 Screenshot 2023-10-05 135948
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 -

image image

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
Copy link
Member

jeffibm commented Apr 8, 2024

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
Copy link
Member

agrare commented Apr 18, 2024

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

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Apr 18, 2024
@agrare
Copy link
Member

agrare commented Apr 18, 2024

@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
Copy link
Contributor Author

@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
Copy link
Member

miq-bot commented May 16, 2024

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

app/helpers/application_helper/toolbar/resource_pool_infras_center.rb

app/helpers/ems_cloud_helper/textual_summary.rb

app/helpers/resource_pool_cloud_helper/textual_summary.rb

app/helpers/resource_pool_infra_helper/textual_summary.rb

app/helpers/vm_helper/textual_summary.rb

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

@Guddetisandeep Guddetisandeep changed the title [WIP] Add Resource_pool UI for Cloud managers Add Resource_pool UI for Cloud managers Jul 11, 2024
@agrare
Copy link
Member

agrare commented Jul 11, 2024

@GilbertCherrie can you review this PR?

@agrare agrare assigned GilbertCherrie and unassigned jeffibm Jul 11, 2024
@Guddetisandeep Guddetisandeep changed the title Add Resource_pool UI for Cloud managers Add Resource Pool UI for Cloud managers Jul 11, 2024
@miq-bot miq-bot removed the wip label Jul 11, 2024
@GilbertCherrie
Copy link
Member

GilbertCherrie commented Jul 18, 2024

@agrare This pr looks good. I was able to check the infrastructure resource pools but don't have any data for cloud resource pools. Do you think you can check the cloud resource pools page?

@agrare agrare merged commit 7fcec09 into ManageIQ:master Aug 22, 2024
15 checks passed
jrafanie pushed a commit that referenced this pull request Aug 30, 2024
This reverts commit 7fcec09, reversing
changes made to 9fbcf7b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants