-
Notifications
You must be signed in to change notification settings - Fork 900
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
Separate Storage Managers By Type in UI #12399
Separate Storage Managers By Type in UI #12399
Conversation
@h-kataria @dclarizio @roliveri please review. |
c98f728
to
9d8327a
Compare
Rubocop warnings addressed and re-pushed. |
9d8327a
to
0231389
Compare
Tests added and fixed and all have passed. Still waiting for comments from @h-kataria with regard to headings on the list screens but otherwise this could have WIP removed... |
a052de2
to
795f39e
Compare
All issues have been resolved. More tests have been added. Removing the WIP label from the title. |
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
@chessbyte was the WIP label added back into the title here automagically or purposefully? I had removed it when I believed the PR ready to merge. Thanks. |
@h-kataria @roliveri @dclarizio can someone review this - thanks! |
782e7af
to
7c17428
Compare
@miq-bot remove_label wip |
@martinpovolny please review code and for menu/UI guidelines. Thx, Dan @jerryk55 we'll try to get this reviewed, but it's quite large and we're tackling blockers right now. |
@dclarizio thanks. @martinpovolny let me know if you have any questions. By the way the Travis failure is a generic issue related to bower. I'm not sure who is working on that issue. |
@jerryk55 I restarted the VMDB tests, they are running green, so will keep an eye on it. |
This looks good to me. We may want to include someone on the performance team to evaluate DB access performance, and scaleability. cc: @dmetzger57 |
Has anyone had a chance to review? @martinpovolny? @dmetzger57? Bueller? It would be nice to have this merged if possible. Thanks. |
7c17428
to
81ec9d2
Compare
@@ -62,7 +62,7 @@ def show | |||
private | |||
|
|||
def get_session_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could use the GenericSessionMixin
here instead of having an own method (and altering the way @title
is calculated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinpovolny I didn't add this controller or even this method as part of this PR. I only changed the string value of the title.
@@ -0,0 +1,43 @@ | |||
class EmsBlockStorageController < ApplicationController |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess you need a menu_section
line in this controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinpovolny just curious - what is that supposed to do - the menu code clearly works for me without the line in both the EmsBlockStorageController and EmsObjectStorageController - neither of which have it. Also I've moved some of the menu sections around for these controllers and it appears that there may be other changes needed if this actually has an effect.Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind that - @h-kataria has explained it to me. I have made the required changes to each controller represented by a modified entry in the Menu::DefaultMenu.
end | ||
|
||
def show_list | ||
opts = {:supported_features_filter => "supports_object_storage?", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to force the use of GenericListMixin
for all the show_list
implementations one by one. So would be nice if you could follow that pattern but I understand that you are following another pattern here and it's hard to tell which one to choose :-(
So I can fix this later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks.
w/o the Overall the code looks good. Do we have a volunteer to run this in the UI and check if there are any problem? |
81ec9d2
to
16a17e0
Compare
@martinpovolny i have a db from @jerryk55 i am going to test it in UI. |
16a17e0
to
7429b02
Compare
The latest commit has been added to resolve several issues discovered by @h-kataria during testing. |
1. Add the ability to show Storage Managers by the type of storage they support. 2. Add supports_block_storage? and supports_object_storage? methods and add them to the Cinder and Swift managers, respectively. 3. Change the UI so that it Shows Block and Object storage and the managers for each separately. 4. Make menu and headings for Storage Managers consistent (remove any indication that the Managers are "Providers").
Fix all Rubocop warnings for this PR. Note tthat warnings for app/views/ems_block_storage/*.haml and app/views/ems_object_storage/*.haml will not be addressed since the code is consistent with many other examples in app/views.
Add tests for the new menus for Block Storage and Object Storage.
Final functional commit for this PR fixes the list headings for the Object and Block Storage Managers. Additionally it adds in the center toolbars and the proper hover over text on the toolbar menus.
Add tests for show_list functionality of Block and Object Storage Manager Controllers.
…Menu Each controller representing a MenuItem in the Menu::DefaultMenu now contains a menu_section directive indicating the MenuSection in which it is located. Some of these need to be changed and some added due to modifications in this PR.
This commit addresses the following issues discovered during testing of this PR: 1) The Block Storage Manager's Relationships Accordion failed to open. It should contain a link to the Cloud Volumes for the Manager. 2) Both the Properties Accordion's link to the Timeline as well as the drop-down Monitoring menu item for the Timeline were not grayed out when there are not any events for the Storage Manager.
The default menu list items for the Block and Object Storage Managers each were set to be selected when the "StorageManager" controller was run, which occurs when one of the storage managers has its detailed info displayed. The menu was updated to the correct controllers.
7429b02
to
df6f056
Compare
Checked commits jerryk55/manageiq@6a1ee1a~...df6f056 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 app/helpers/application_helper/toolbar/ems_block_storages_center.rb
app/helpers/application_helper/toolbar/ems_object_storages_center.rb
app/helpers/application_helper/toolbar/ems_storage_center.rb
app/helpers/application_helper/toolbar/ems_storages_center.rb
app/views/ems_block_storage/edit.html.haml
app/views/ems_block_storage/new.html.haml
app/views/ems_object_storage/edit.html.haml
app/views/ems_object_storage/new.html.haml
|
This final (hopefully) commit fixes an issue that resulted in the Storage Manager AND Object Manager menu items being selected when the detailed info was displayed for any one of either of these managers. Incorrect entries in the default menu pointing to the storage_manager controller caused this. All issues discovered by @h-kataria during testing have been resolved. This is ready to go after final review. Thanks all! |
Tested in UI, looks good. |
and add them to the Cinder and Swift managers, respectively.
separately.
Managers are "Providers").
Block Storage Menus:
Object Storage Menus:
Block Storage Managers:
Object Storage Managers:
Hover Text:
This PR is dependent upon #12310 to add the functionality to let the UI filter items in show_list by the "supports_feature?"
Links