Configuration manager non explorer version #6782

h-kataria · 2020-03-20T22:04:21Z

New screens under Configuration main tab with non-explorer version of

Providers
Profiles
Configured Systems

These screens replace subtab Management under Configuration maintab.

@skateman it's a work in progress to address #6756 (comment). just creating a wip PR as i am working thru further cleanup and fixing things as i compare new screens with original explorer version to match all the current feature implementations. I will also add some screesnhots, once i have this bit more cleaned up.

Pending core ManageIQ/manageiq#19949
Schema PR ManageIQ/manageiq-schema#464

before changes screenshots
Sub menu under Configuration main menu

Configuration Manager node selection

Configuration Profile node selection

Configured System node selection

after changes screenshots
New sub menus under Configuration main menu

Blank screen with Add Provider button where there are no Configuration Manager Providers yet in db

List of all Configuration Managers with advanced search/filters

Configuration Manager Summary

List of All Configuration Profiles with advanced search/filters

Configuration Profile Summary

List of All Configured Systems

Configured System Summary

skateman · 2020-03-25T07:53:02Z

Looks great on the screenshots, I think we should support filters on the two empty sidebars as well. I'm not sure how complex it is to do so, so it might be better to do it in a separate PR. Anyway, great job 👍

h-kataria · 2020-03-25T12:48:14Z

Looks great on the screenshots, I think we should support filters on the two empty sidebars as well. I'm not sure how complex it is to do so, so it might be better to do it in a separate PR. Anyway, great job

That's an easy one to do, i will add that in here

app/helpers/application_helper.rb

skateman · 2020-03-26T18:05:36Z

app/controllers/application_controller/ci_processing.rb

@@ -143,7 +143,7 @@ def process_elements(elements, klass, task, display_name = nil, order_field = ni
  end

  def explorer_controller?
-    %w[vm_cloud vm_infra vm_or_template infra_networking].include?(controller_name)
+    %w[vm_cloud vm_infra vm_or_template infra_networking automation_manager].include?(controller_name)


shouldn't be configuration_manager?

no, configuration_manger is no longer an explorer controller but shares some code with automation_manager controller and that is explorer style screen, so i need to add this here so i can redirect appropriately from shared methods.

skateman · 2020-03-30T10:37:24Z

Tried, seems working, @mzazrivec wanna test too?

Fryguy · 2020-04-23T16:11:41Z

app/controllers/ems_configuration_controller.rb

+  end
+
+  def provider_class
+    ManageIQ::Providers::Foreman::Provider


Why is Foreman still here?

@Fryguy This is still here because currently UI does not have a type drop down in Add a new Provider form, i was hoping once we have new Data driven Provider form in place we will have a type drop down for user to select what type of Provider they want to add with their respective fields. Do you think i should i add a Type drop down on the Add form, if yes, what should be the options, then we can change this code to use value from the type field.

How do we do it for other providers?

Or is this related to the problem with the Provider model vs ExtManagementSystem model?

other provider have a type field on the form, based upon the type selected a Provider object is created. I am working on adding a Type field to the Add Configuration Manager form with similar logic that we have for other providers. Should get that in shortly.

Is there a point to all this?

Consider that for now Foreman::Provider is precisely the class this controller needs.

None of this code should exists in the first place, aren't we just wasting time that could be spent on finishing the proper way of doing that via the API?

(I mean, introducing little tech debt is probably cheaper than designing technical debt to introduce :) .)

@Fryguy I agree with @himdel that's the the reason i did not change Add new provider form in this PR. Even if we decide to make changes to existing form , i would prefer a separate follow up PR, this PR is getting way too big.

@h-kataria happy to help add permissions and supported_for_create to the provider, would rather that than hard coding Foreman

If these are meant to be temporary, then I'm fine with that, especially as the goal is to move towards pluggable DDF for this feature.

aren't we just wasting time that could be spent on finishing the proper way of doing that via the API?

Isn't that what we are doing with this discussion? It's clear there's a general design issue with respect to provider/manager split, so discussing it is something we need to do anyway for the API. It helps to have that discussion in context with the code that is actually consuming it, hence me bringing in @agrare, as he has some opinions on how best to deal with the provider/manager split.

Also this thread started as me noticing Foreman in the code when the purpose of the PR is to remove Foreman from the code, so it stood out. It's not clear from the code that the purpose for the method is to drive the drop-down for adding a new provider.

line of code in question does not drive the drop-down as currently there is no Type drop-down in the UI. Currently users can only add a Foreman Provider and this line is providing class for the new record, when the Add button is UI is pressed.

app/controllers/mixins/manager_controller_mixin.rb

Fryguy · 2020-04-23T16:16:29Z

app/controllers/restful_redirect_controller.rb

-        if %w[ManageIQ::Providers::ConfigurationManager].include?(record.type) || record.type.starts_with?('ManageIQ::Providers::Foreman')
-          redirect_to(:controller => 'provider_foreman', :action => 'show', :id => params[:id])
-        elsif %w[ManageIQ::Providers::AnsibleTower::AutomationManager].include?(record.type)
+        if %w[ManageIQ::Providers::AnsibleTower::AutomationManager].include?(record.type)


Can we prefer if record.kind_of?(ManageIQ::Providers::ExternalAutomationManager), so we can remove the plugin reference entirely?

I see technically this was original code (didn't realize initially), so this can be a followup

Fryguy · 2020-04-23T16:16:51Z

app/controllers/restful_redirect_controller.rb

-        if %w[ManageIQ::Providers::ConfigurationManager].include?(record.type) || record.type.starts_with?('ManageIQ::Providers::Foreman')
-          redirect_to(:controller => 'provider_foreman', :action => 'show', :id => params[:id])
-        elsif %w[ManageIQ::Providers::AnsibleTower::AutomationManager].include?(record.type)
+        if %w[ManageIQ::Providers::AnsibleTower::AutomationManager].include?(record.type)
          redirect_to(:controller => 'automation_manager', :action => 'show', :id => params[:id])
        elsif %w[ManageIQ::Providers::EmbeddedAnsible::AutomationManager].include?(record.type)


Same here - if record.kind_of?(ManageIQ::Providers::EmbeddedAutomationManager)

I see technically this was original code (didn't realize initially), so this can be a followup

app/helpers/application_helper/toolbar/ems_configurations_center.rb

Fryguy · 2020-04-23T16:24:49Z

spec/controllers/application_controller/advanced_search_spec.rb

-    stub_user(:features => :all)
-    controller.instance_variable_set(:@sb, {})
-  end
+describe EmsConfigurationController, "::AdvancedSearch" do


Not for this PR, but it's weird to me to have a EmsConfigurationController, "::AdvancedSearch" in a file named application_controller/advanced_search_spec.rb as the latter is generic and the former is specific.

spec/controllers/restful_redirect_controller_spec.rb

Fryguy · 2020-04-23T16:28:02Z

spec/presenters/tree_node/ext_management_system_spec.rb

@@ -28,7 +28,6 @@
    :ems_vmware_cloud                 => { :tip_prefix => 'Ems Cloud' },
    # Other remaining providers
    :automation_manager_ansible_tower => { :key_prefix => 'at-' },
-    :configuration_manager_foreman    => { :key_prefix => 'fr-' },


should the configuration_manager entry about 12 lines down be uncommented and/or deleted?

@h-kataria Is this a valid concern? ☝️

@Fryguy now that the tree is removed from the left side in Configuration Managers area, we do not need this entry.

- ConfiguredSystem controller changed to use updated named_scope - Moved EmsConfiguration specific code into controller that's being used when pressing new/edit buttons in UI. - Added/Updated accidentally deleted code back in RestfulRedirect controller and it's spec test. - Fixed a typo in ems_configurations_center_tb - removed redundant `provider_manager_foreman` entry from tree_node/ext_management_system_spec

miq-bot · 2020-04-23T20:11:18Z

Checked commits h-kataria/manageiq-ui-classic@6173e3a~...3f8593e with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
59 files checked, 76 offenses detected

app/controllers/ems_configuration_controller.rb

⚠️ - Line 71, Col 3 - Lint/IneffectiveAccessModifier - private (on line 69) does not make singleton methods private. Use private_class_method or private inside a class << self block instead.

app/controllers/mixins/automation_manager_controller_mixin.rb

❗ - Line 166, Col 22 - Naming/UncommunicativeMethodParamName - Method parameter must be at least 3 characters long.
❗ - Line 184, Col 23 - Naming/UncommunicativeMethodParamName - Method parameter must be at least 3 characters long.
❗ - Line 192, Col 32 - Naming/UncommunicativeMethodParamName - Method parameter must be at least 3 characters long.
❗ - Line 202, Col 32 - Naming/UncommunicativeMethodParamName - Method parameter must be at least 3 characters long.
❗ - Line 46, Col 25 - Naming/UncommunicativeMethodParamName - Method parameter must be at least 3 characters long.
❗ - Line 80, Col 14 - Naming/UncommunicativeMethodParamName - Method parameter must be at least 3 characters long.

app/helpers/application_helper/toolbar/configured_system_center.rb

❗ - Line 13, 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 16, Col 3 - Layout/IndentFirstArrayElement - Indent the right bracket the same as the first position after the preceding left parenthesis.
❗ - Line 3, Col 5 - Layout/IndentFirstArrayElement - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar/configured_systems_center.rb

❗ - Line 17, Col 80 - 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 20, Col 3 - Layout/IndentFirstArrayElement - Indent the right bracket the same as the first position after the preceding left parenthesis.
❗ - Line 22, Col 5 - Layout/IndentFirstArrayElement - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
❗ - Line 35, 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 38, Col 3 - Layout/IndentFirstArrayElement - Indent the right bracket the same as the first position after the preceding left parenthesis.
❗ - Line 3, Col 5 - Layout/IndentFirstArrayElement - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar/ems_configuration_center.rb

❗ - Line 22, Col 12 - 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 40, Col 5 - Layout/IndentFirstArrayElement - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
❗ - Line 50, 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 53, Col 3 - Layout/IndentFirstArrayElement - Indent the right bracket the same as the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar/ems_configurations_center.rb

❗ - Line 26, Col 27 - 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.

app/presenters/menu/default_menu.rb

❗ - Line 16, Col 11 - Layout/IndentFirstArrayElement - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/views/automation_manager/_form.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.

app/views/configuration_manager/_shared_form.html.haml

⚠️ - Line 98 - Layout/TrailingBlankLines: Final newline missing.

app/views/configuration_profile/show.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.

app/views/configuration_profile/show_list.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.
⚠️ - Line 1 - id attribute must be in lisp-case

app/views/configured_system/show.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.

app/views/configured_system/show_list.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.
⚠️ - Line 1 - id attribute must be in lisp-case

app/views/ems_configuration/edit.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.

app/views/ems_configuration/new.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.

app/views/ems_configuration/show.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.

app/views/ems_configuration/show_list.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.
⚠️ - Line 1 - id attribute must be in lisp-case

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

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

⚠️ - Line 17 - Avoid using instance variables in partials views
⚠️ - Line 17 - Style/BracesAroundHashParameters: Missing curly braces around a hash parameter.
⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.
⚠️ - Line 22 - Avoid using instance variables in partials views
⚠️ - Line 22 - Style/BracesAroundHashParameters: Missing curly braces around a hash parameter.
⚠️ - Line 4 - Avoid using instance variables in partials views
⚠️ - Line 9 - Avoid using instance variables in partials views
⚠️ - Line 9 - Style/BracesAroundHashParameters: Missing curly braces around a hash parameter.

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

Fryguy · 2020-04-28T20:14:51Z

app/controllers/mixins/automation_manager_controller_mixin.rb

+    end
+
+    def breadcrumb_name(_model)
+      "#{ui_lookup(:ui_title => 'foreman')} #{_('Provider')}"


Is ui_title correct here (i.e. is it still 'foreman')?

this line was added in ManageIQ#6782, should keep the next line that uses `arel_table`

…lorer_version Configuration manager non explorer version (cherry picked from commit fdc0b7a)

simaishi · 2020-05-21T19:56:20Z

Jansa backport details:

$ git log -1
commit 978f54bfd2a26a3802d9181de86a8841c3cb26ad
Author: Jason Frey <[email protected]>
Date:   Thu May 7 22:06:12 2020 -0400

    Merge pull request #6782 from h-kataria/configuration_manager_non_explorer_version

    Configuration manager non explorer version

    (cherry picked from commit fdc0b7a2e3794dc15e077ff71e9841486f4c8524)

this line was added in ManageIQ#6782, should keep the next line that uses `arel_table`

h-kataria added wip configuration management ivanchuk/no labels Mar 20, 2020

h-kataria requested a review from skateman March 20, 2020 22:04

h-kataria requested a review from martinpovolny as a code owner March 20, 2020 22:04

chessbyte mentioned this pull request Mar 21, 2020

Support Pluggable Configuration Manager ManageIQ/manageiq#19992

Closed

3 tasks

h-kataria force-pushed the configuration_manager_non_explorer_version branch from 4b56a07 to 9501afc Compare March 23, 2020 14:26

h-kataria added the enhancement label Mar 23, 2020

h-kataria requested review from himdel and removed request for martinpovolny March 24, 2020 19:30

h-kataria assigned himdel Mar 24, 2020

h-kataria force-pushed the configuration_manager_non_explorer_version branch from ccc47fd to 35cca40 Compare March 24, 2020 22:26

skateman reviewed Mar 25, 2020

View reviewed changes

app/helpers/application_helper.rb Outdated Show resolved Hide resolved

h-kataria changed the title ~~[WIP] - Configuration manager non explorer version~~ Configuration manager non explorer version Mar 25, 2020

h-kataria removed the wip label Mar 25, 2020

h-kataria force-pushed the configuration_manager_non_explorer_version branch 2 times, most recently from bb6aa63 to 3c482ce Compare March 25, 2020 21:15

h-kataria added the pending core label Mar 25, 2020

skateman reviewed Mar 26, 2020

View reviewed changes

This was referenced Mar 27, 2020

Renaming of provider_foreman files, code to configuration_manager #6756

Closed

Removed provider_foreman explorer and added new non-explorer style screens under Configuration main tab. ManageIQ/manageiq#19949

Merged

h-kataria force-pushed the configuration_manager_non_explorer_version branch 4 times, most recently from f8d41fc to 36436dc Compare April 1, 2020 13:21

h-kataria added the wip label Apr 2, 2020

h-kataria force-pushed the configuration_manager_non_explorer_version branch from 36436dc to ec82ba1 Compare April 2, 2020 14:25