Catalog Item type list is dependent on installed providers #16559

mkanoor · 2017-11-29T21:07:52Z

https://bugzilla.redhat.com/show_bug.cgi?id=1515371

Currently we display all catalog item types irrespective of what
providers are installed in the appliance. This PR links the catalog
items to different providers and sets the display attribute based
on the presence of specific provider(s). This would be used by the
UI to list the valid catalog item types

Amazon Provider Changes are in PR
ManageIQ/manageiq-providers-amazon#377

VMware Provider changes in PR
ManageIQ/manageiq-providers-vmware#151

Openshift Provider changes in
ManageIQ/manageiq-providers-openshift#74

SCVMM Provider changes in PR
ManageIQ/manageiq-providers-scvmm#54

Openstack Provider PR
ManageIQ/manageiq-providers-openstack#177

Azure Provider PR
ManageIQ/manageiq-providers-azure#185

External Ansible Tower PR
ManageIQ/manageiq-providers-ansible_tower#42

Redhat Provider PR
ManageIQ/manageiq-providers-ovirt#174

Google Provider PR
ManageIQ/manageiq-providers-google#35

mkanoor · 2017-11-29T21:09:35Z

@Fryguy @bzwei @h-kataria
Please review

bzwei · 2017-11-29T21:42:08Z

app/models/service_template.rb

+  end
+
+  def self.generic_ansible_playbook_provider_present?
+    ExtManagementSystem.where(:type => "ManageIQ::Providers::EmbeddedAnsible::AutomationManager").any?


should check whether it is enabled.

bzwei · 2017-11-29T21:44:37Z

app/models/service_template.rb

+  ORCHESTRATION_PROVIDER_TYPES = %w(
+    ManageIQ::Providers::Amazon::CloudManager
+    ManageIQ::Providers::Azure::CloudManager
+    ManageIQ::Providers::Google::CloudManager


no google, but we have ManageIQ::Providers::VMWare::CloudManager

@bzwei Is there a way to dynamically build this list?

@gmcculloug Yes we can. Working with Madhu now.

bzwei · 2017-11-29T21:44:59Z

app/models/service_template.rb

+    ManageIQ::Providers::Azure::CloudManager
+    ManageIQ::Providers::Google::CloudManager
+    ManageIQ::Providers::Openstack::CloudManager
+    ManageIQ::Providers::Openstack::InfraManager


no InfraManager

bzwei · 2017-11-29T21:47:39Z

app/models/service_template.rb

@@ -69,6 +69,70 @@ class ServiceTemplate < ApplicationRecord
  scope :with_existent_service_template_catalog_id, ->         { where.not(:service_template_catalog_id => nil) }
  scope :displayed,                                 ->         { where(:display => true) }

+  def self.catalog_item_types
+    CATALOG_ITEM_TYPES.each.with_object({}) do |(key, description), hash|


How does the UI use it - gray out non-displayable item from the dropdown? Or should we simply remove non-qualified types?

@bzwei @h-kataria mentioned that there is some bootstrap magic that does that.

@bzwei yes UI will show those items as grayed/disabled in the drop down, see screenshot in ManageIQ/manageiq-ui-classic#2908

bzwei · 2017-11-29T21:51:07Z

app/models/service_template.rb

+    ManageIQ::Providers::Openstack::InfraManager
+  ).freeze
+
+  def self.generic_orchestration_provider_present?


It does not solve the case where providers are added but there is no orchestration template. The same question for OpenShift template.

@bzwei so what is the call to check if there is an orchestration template?

For each orderable orchestration template, find its eligible_managers

This PR uses changes from backend in ManageIQ/manageiq#16559 to determine whether options in the drop down should be available or disabled based upon results returned in hash by backend method. https://bugzilla.redhat.com/show_bug.cgi?id=1515371

bzwei · 2017-12-01T21:44:37Z

app/models/service_template.rb

+
+  def self.generic_orchestration_provider_present?
+    orchestration_providers = OrchestrationTemplate.distinct.pluck(:type).collect do |otype|
+      otype.constantize.eligible_managers.collect(&:type)


#eligible_managers has Rbac (https://github.com/ManageIQ/manageiq/blob/master/app/models/orchestration_template.rb#L127). Should we check the same for other provider_present? in Ln 104, 108 etc?

@bzwei

manageiq/app/models/orchestration_template.rb

Line 127 in d66dbcc

Rbac::Filterer.filtered(ExtManagementSystem, :named_scope => [[:with_eligible_manager_types, eligible_manager_types]])

Does not accept a User object so its most probably relying on the User.current_user being set in the UI controller based on the currently logged in user so it should do the right thing here. We dont have to pass the user object.

@bzwei I checked with @kbrock and his suggestion was the RBAC calls might slow this down a bit. So for the first round can we just use the ActiveModel queries and then we can make the RBAC changes.

you can pass in :user into the filtered() call

Rbac slows everything down. It is faster to just have no security.

The goal is to use rbac in your query along with other sql filtering (e.g. with_eligible_manager_types)

Does this work just calling the scope directly?

Rbac::Filterer.filtered(ExtManagementSystem.with_eligible_manager_types(eligible_manager_types))

bzwei · 2017-12-01T21:46:31Z

app/models/service_template.rb

+  end
+
+  def self.openstack_provider_present?
+    ExtManagementSystem.where(:type => "ManageIQ::Providers::Openstack::InfraManager").any?


We have both CloudManager and InfraManager for OpenStack.

Fryguy · 2017-12-04T20:17:19Z

app/models/service_template.rb

+
+  def self.azure_provider_present?
+    ExtManagementSystem.where(:type => "ManageIQ::Providers::Azure::CloudManager").any?
+  end


👎 for provider specifics in the manageiq repo...there has to be a way to do this more generically / pluggable.

also, the fact that this needs to be defined for every provider should send off warning bells.

Fryguy

I don't like the hardcoding of providers in here as it's just one more impossible-to-detangle place.

kbrock

This will make too many queries to the db.
Would be best to bring back catalog items and the corresponding type (from there, the ui should be able to provide a filter)

Having said that, we want to avoid asking the ui to hard code the class names in their checks.

kbrock · 2017-12-05T18:33:26Z

app/models/service_template.rb

+
+  def self.generic_orchestration_provider_present?
+    orchestration_providers = OrchestrationTemplate.distinct.pluck(:type).collect do |otype|
+      otype.constantize.eligible_managers.collect(&:type)


you can pass in :user into the filtered() call

Rbac slows everything down. It is faster to just have no security.

The goal is to use rbac in your query along with other sql filtering (e.g. with_eligible_manager_types)

Does this work just calling the scope directly?

Rbac::Filterer.filtered(ExtManagementSystem.with_eligible_manager_types(eligible_manager_types))

kbrock · 2017-12-05T18:34:06Z

app/models/service_template.rb

+
+  def self.azure_provider_present?
+    ExtManagementSystem.where(:type => "ManageIQ::Providers::Azure::CloudManager").any?
+  end


also, the fact that this needs to be defined for every provider should send off warning bells.

https://bugzilla.redhat.com/show_bug.cgi?id=1515371 Currently we display all catalog item types irrespective of what providers are installed in the appliance. This PR links the catalog items to different providers and sets the display attribute based on the presence of specific provider(s). This would be used by the UI to list the valid catalog item types

mkanoor · 2017-12-08T19:37:22Z

@Fryguy @agrare
Please review

Fryguy · 2017-12-09T01:52:48Z

app/models/service_template.rb

+  def self.catalog_item_types
+    ci_types = Rbac.filtered(ExtManagementSystem.all).flat_map(&:supported_catalog_types).uniq
+    ci_types << 'generic_orchestration' if Rbac.filtered(OrchestrationTemplate).exists?
+    ci_types << 'generic'


Since you are building this up as an index for lookup purposes (with uniqueness), you would be better off using a Set object (and not manually calling unique, but letting Set do that for you).

Fryguy

This looks much better than previously 👍

@agrare

@agrare I'll leave this one up to you.

miq-bot · 2017-12-11T17:14:28Z

Checked commits mkanoor/manageiq@aca00ba~...32eef9e with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 👍

agrare · 2017-12-11T18:05:34Z

All dependent provider PRs merged

agrare

Looks good to me @mkanoor 👍

Catalog Item type list is dependent on installed providers (cherry picked from commit 0bedb65) https://bugzilla.redhat.com/show_bug.cgi?id=1520613

simaishi · 2017-12-11T18:50:53Z

Gaprindashvili backport details:

$ git log -1
commit 1c20502be8c4fed237bc49612a62722d7d1851c6
Author: Adam Grare <[email protected]>
Date:   Mon Dec 11 13:06:28 2017 -0500

    Merge pull request #16559 from mkanoor/catalog_item_types
    
    Catalog Item type list is dependent on installed providers
    (cherry picked from commit 0bedb65ae134f00b2f15511ce5270208aa5b6ac3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1520613

mkanoor added the bug label Nov 29, 2017

mkanoor requested review from Fryguy and bzwei November 29, 2017 21:07

bzwei reviewed Nov 29, 2017

View reviewed changes

chessbyte assigned gmcculloug Dec 1, 2017

h-kataria mentioned this pull request Dec 1, 2017

UI changes to show options as disabled if those are not supported. ManageIQ/manageiq-ui-classic#2908

Merged

bzwei reviewed Dec 1, 2017

View reviewed changes

Fryguy reviewed Dec 4, 2017

View reviewed changes

Fryguy previously requested changes Dec 4, 2017

View reviewed changes

kbrock reviewed Dec 5, 2017

View reviewed changes

mkanoor force-pushed the catalog_item_types branch from 12243fd to aca00ba Compare December 8, 2017 19:23

mkanoor mentioned this pull request Dec 8, 2017

Added supported_catalog_types ManageIQ/manageiq-providers-amazon#377

Merged

Added supported_catalog_types for embedded_ansible

0327cbb

This was referenced Dec 8, 2017

Added supported_catalog_types ManageIQ/manageiq-providers-ansible_tower#42

Merged

Added supported_catalog_types ManageIQ/manageiq-providers-ovirt#174

Merged

Added supported_catalog_types ManageIQ/manageiq-providers-google#35

Merged

Fryguy reviewed Dec 9, 2017

View reviewed changes

Changed uniq to Set.new

32eef9e

gmcculloug approved these changes Dec 11, 2017

View reviewed changes

agrare approved these changes Dec 11, 2017

View reviewed changes

agrare merged commit 0bedb65 into ManageIQ:master Dec 11, 2017

agrare added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 11, 2017

agrare added lifecycle/services providers gaprindashvili/yes labels Dec 11, 2017

simaishi pushed a commit that referenced this pull request Dec 11, 2017

Merge pull request #16559 from mkanoor/catalog_item_types

1c20502

Catalog Item type list is dependent on installed providers (cherry picked from commit 0bedb65) https://bugzilla.redhat.com/show_bug.cgi?id=1520613

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Dec 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catalog Item type list is dependent on installed providers #16559

Catalog Item type list is dependent on installed providers #16559

mkanoor commented Nov 29, 2017 •

edited

Loading

mkanoor commented Nov 29, 2017

bzwei Nov 29, 2017

bzwei Nov 29, 2017

gmcculloug Dec 1, 2017

bzwei Dec 1, 2017

bzwei Nov 29, 2017

bzwei Nov 29, 2017

mkanoor Nov 30, 2017

h-kataria Dec 1, 2017

bzwei Nov 29, 2017

mkanoor Nov 29, 2017

bzwei Nov 30, 2017

bzwei Dec 1, 2017

mkanoor Dec 1, 2017

mkanoor Dec 4, 2017

kbrock Dec 5, 2017

bzwei Dec 1, 2017

Fryguy Dec 4, 2017

kbrock Dec 5, 2017

Fryguy left a comment

kbrock left a comment

kbrock Dec 5, 2017

kbrock Dec 5, 2017

mkanoor commented Dec 8, 2017

Fryguy Dec 9, 2017

Fryguy left a comment

miq-bot commented Dec 11, 2017

agrare commented Dec 11, 2017

agrare left a comment

simaishi commented Dec 11, 2017

Catalog Item type list is dependent on installed providers #16559

Catalog Item type list is dependent on installed providers #16559

Conversation

mkanoor commented Nov 29, 2017 • edited Loading

mkanoor commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy left a comment

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkanoor commented Dec 8, 2017

Choose a reason for hiding this comment

Fryguy left a comment

Choose a reason for hiding this comment

miq-bot commented Dec 11, 2017

agrare commented Dec 11, 2017

agrare left a comment

Choose a reason for hiding this comment

simaishi commented Dec 11, 2017

mkanoor commented Nov 29, 2017 •

edited

Loading