Enable container start pages #380
Conversation
Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
|
@moolitayer @cben please review. @miq-bot add_label providers/containers |
|
@simon3z Cannot apply the following label because they are not recognized: providers/containers |
|
Checked commit simon3z@15af4b0 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
| @@ -41,7 +36,6 @@ def start_page_options | |||
|
|
|||
| def start_page_allowed?(start_page) | |||
| return false if STORAGE_START_PAGES.include?(start_page) | |||
There was a problem hiding this comment.
This line looks like a bug. Makes no sense to disallow unconditionally?
It used to also have && !::Settings.product.storage condition before #204: simon3z@acf224a#diff-4c149c5e0e75ed63331744d901546b7cL44
cc @ZitaNemeckova @himdel
There was a problem hiding this comment.
Looks right, that storage setting is gone, so never true, and StorageManagerController still exists, is in the menu (under the storage_manager_show_list feature) .. not seeing a problem :)
There was a problem hiding this comment.
(Unless this was broken before, or unless StorageManager should have been under that setting too)
There was a problem hiding this comment.
I think it prevents "Storage / Storage Managers" from appearing under EVM -> My Settings -> Visual -> "Start Page" dropdown.
Aha, it would not be there anyway because it's not in MiqShortcut.start_pages because
https://github.com/manageiq/manageiq/blob/master/db/fixtures/miq_shortcuts.yml#L197-L200
doesn't set :startup: true.
Anyway I think STORAGE_START_PAGES & this line should be dropped like this PR did for CONTAINERS_START_PAGES. Before #204 the logic was
return false if STORAGE_START_PAGES.include?(start_page) && !::Settings.product.storage
which "allowed" it to be seen conditionally on the setting (if only it had :startup: true), until the setting disappeared without revising this file.
[#378 deleted most UI checks of the setting; can't find it in settings.yml history, maybe it was never there, only set by devs locally?]
|
@miq-bot assign himdel |
|
oh I see you already added what you needed in ManageIQ/manageiq#13906 |
|
LGTM 👍 @simon3z , @moolitayer .. does this PR depend on that ManageIQ/manageiq#13906 ? Or can I merge regardless of that one? |
|
Does not depend @himdel Thanks 🍺 |
|
Thanks :) merged :) |
This enables the container pages to be used as start pages.