Implement toolbar extension mechanism for providers #4262

martinpovolny · 2018-07-09T08:39:30Z

Context

We want to allow the provider authors to extend ManageIQ with Ansible playbooks and roles. ManageIQ core provides the capability to execute an Asible playbook from the provider repository. The ManageIQ UI provides a way to call that playbook from a toolbar. Before calling the playbook additional parameters can be collected using a custom dialog.

Provider workflow

Implement your Ansible playbooks and place them in the provider repo manageiq-provider-xxx
Implement your custom dialogs as React components or ManageIQ dynamic (service) dialog, place them in the provider repo.
Create a toolbar extension class (example below) and place it in the provider repo

Example provider PR

(ui part): [WIP] Provider dialogs #4231
Ansible playbook: [WIP] Use ansible power actions manageiq-providers-amazon#453

Related UI PRs:

Goal of this PR

This allows providers to extend existing toolbars by creating classes such as:

app/helpers/manageiq/providers/amazon/toolbar_overrides/ems_cloud_center.rb in the manageiq-providers-amazon repo.

module ManageIQ
  module Providers
    module Amazon
      module ToolbarOverrides
        class EmsCloudCenter < ::ApplicationHelper::Toolbar::Override
          button_group('magic', [
            button(
              :magic,
              'fa fa-magic fa-lg',
              t = N_('Magic'),
              t,
              :data  => {'function'      => 'sendDataWithRx',
                         'function-data' => {:controller     => 'provider_dialogs', # this one is required
                                             :button         => :magic,
                                             :modal_title    => N_('Create a Security Group'),
                                             :component_name => 'CreateAmazonSecurityGroupForm',
                                              }.to_json},
              :klass => ApplicationHelper::Button::ButtonWithoutRbacCheck),
            button(
              :magic_dialog,
              'fa fa-magic fa-lg',
              t = N_('Magic'),
              t,
              :data  => {'function'      => 'sendDataWithRx',
                         'function-data' => {:controller => 'provider_dialogs', # this one is required
                                             :button     => :magic_dialog}.to_json},
              :klass => ApplicationHelper::Button::ButtonWithoutRbacCheck),
          ])
        end
      end
    end
  end
end

Any toolbar can be overridden but we focus on providers on the entity detail screen in this PR. Examples:

Instance detail screen,
Cloud provider detail screen (the example above).

For these (detail) screens. The toolbar is not only extended, but the proper extension is selected by matching @record with the toolbar's class.

For screens where there is no @record all available extensions are loaded.

How it is done?

Toolbar base class is reorganized as follows:

Toolbar::Basic is moved to a base class Toolbar::Base
Toolbar::Override (practically Toolbar::Base) is where the provider
extentions should inherit from.
Toolbar::Basic newly includes the extention mechanism.

ToolbarBuilder now passed the @record to the toolbar definition method (now a method). And it filters the available extensions by matching the record's class with the extension class (the provider base module).

martinpovolny · 2018-07-10T14:58:11Z

The toolbar overrides live under provider repositories under app/toolbar_overrides like this:

devil - [~/Projects/manageiq-providers-amazon] (toolber_overrides)$ ls -l app/toolbar_overrides/*
-rw-rw-r--. 1 martin martin 1497 Jul  9 16:09 app/toolbar_overrides/ems_cloud_center.rb
-rw-rw-r--. 1 martin martin 1490 Jul  9 09:24 app/toolbar_overrides/x_vm_cloud_center.rb

The class names are like this ManageIQ::Providers::Amazon::ToolbarOverrides::EmsCloudCenter -- Provider namespace + ToolbarOverrides + name of the toolbar that we are overriding.

To get rid of the require statement, I can add

config.autoload_paths << Rails.root.join("app", "toolbar_overrides").to_s

to application.rb in manageiq (core).

With that:

[9] pry(#<ApplicationHelper::Toolbar::EmsCloudCenter>)> ManageIQ::Providers::Amazon::ToolbarOverrides::EmsCloudCenter                                                                                              
NameError: uninitialized constant ManageIQ::Providers::Amazon::ToolbarOverrides                          
from (pry):1:in `block in extension_classes'

I believe the autoloades splits the class name on "::" and resolves each module in a turn and it does not see a module name ManageIQ::Providers::Amazon::ToolbarOverrides until the file with ManageIQ::Providers::Amazon::ToolbarOverrides::EmsCloudCenter is loaded:

manageiq-providers-amazon/app/toolbar_overrides/ems_cloud_center.rb :

module ManageIQ                                                                                     
  module Providers                                                                                  
    module Amazon                                                                                   
      module ToolbarOverrides                                                                       
        class EmsCloudCenter < ::ApplicationHelper::Toolbar::Override                               
          button_group('magic', [  
...

I can "fix" that:

[10] pry(#<ApplicationHelper::Toolbar::EmsCloudCenter>)> module ManageIQ::Providers::Amazon::ToolbarOverrides; end                                                                                                 
=> nil                                              
[11] pry(#<ApplicationHelper::Toolbar::EmsCloudCenter>)> ManageIQ::Providers::Amazon::ToolbarOverrides::EmsCloudCenter 
D, [2018-07-10T15:38:14.400085 #13946] DEBUG -- :   ManageIQ::Providers::CloudManager Load (0.5ms)  SELECT  "ext_management_systems".* FROM "ext_management_systems" WHERE "ext_management_systems"."type" IN ('ManageIQ::Providers::CloudManager', 'ManageIQ::Providers::Amazon::CloudManager', 'ManageIQ::Providers::Azure::CloudManager', 'ManageIQ::Providers::Google::CloudManager', 'ManageIQ::Providers::Openstack::CloudManager', 'ManageIQ::Providers::Vmware::CloudManager') ORDER BY "ext_management_systems"."id" ASC LIMIT $1  [["LIMIT", 1]]
D, [2018-07-10T15:38:14.463206 #13946] DEBUG -- :   ManageIQ::Providers::CloudManager Inst Including Associations (61.2ms - 1rows)                                                                                 
LoadError: Unable to autoload constant EmsCloudCenter, expected /home/martin/Projects/manageiq-providers-amazon/app/toolbar_overrides/ems_cloud_center.rb to define it                                             
from /home/martin/.rvm/gems/ruby-2.5.0/gems/activesupport-5.0.7/lib/active_support/dependencies.rb:513:in `load_missing_constant'                                                                                  
[12] pry(#<ApplicationHelper::Toolbar::EmsCloudCenter>)> ManageIQ::Providers::Amazon::ToolbarOverrides::EmsCloudCenter                                                                                             
=> ManageIQ::Providers::Amazon::ToolbarOverrides::EmsCloudCenter

So the problem is the :ToolbarOverrides:: in the class name. But I'd like to keep i there.

What's the best way to fix that?

martinpovolny · 2018-07-10T15:03:16Z

ping @Fryguy

Fryguy · 2018-07-10T15:40:18Z

app/helpers/application_helper/toolbar/basic.rb

+        instance = [plugin.name.sub('::Engine',''), 'ToolbarOverrides', self.class.name.to_s.split('::').last].join('::')
+        instance.constantize
+      end
+    end.compact


I think this can all live in the Vmdb::Plugins class itself, but this is mostly pretty good 👍

Fryguy · 2018-07-10T15:40:54Z

app/helpers/application_helper/toolbar/override.rb

+class ApplicationHelper::Toolbar::Override < ApplicationHelper::Toolbar::Basic
+  def definition
+    @definition
+  end


attr_reader :definition seems better.

Fryguy · 2018-07-10T15:41:53Z

app/helpers/application_helper/toolbar/basic.rb

+      @loaded = true
+    end
+
+    @definition


Prefer something like @definition ||= load_overrides and then have load_overrides just return the thing.

Ugh...I see that custom_content injects itself via the ivars...might be better if those methods all go through the definition accessor. That way they are loaded "first". I guess I don't understand the DSL in this class and what is trying to be solved, so perhaps my suggestions should be ignored 😁

I will be redoing that all. So while your comments make sense, I'll just dump the code and do it differently once the autoloading works.

Fryguy · 2018-07-10T15:44:44Z

app/helpers/application_helper/toolbar/basic.rb

+      name = "#{name}.rb"
+      if File.exist?(name)
+        require name
+        instance = [plugin.name.sub('::Engine',''), 'ToolbarOverrides', self.class.name.to_s.split('::').last].join('::')


plugin.name.sub('::Engine','')

Slightly better: plugin.name.chomp("::Engine")

self.class.name

What is an example of self.class.name here? Trying to understand what this does....I think it might be easier with a simple .gsub of the middle section, but I'm not sure.

Fryguy · 2018-07-10T16:06:42Z

app/helpers/application_helper/toolbar/basic.rb

+    end.compact
+  end
+
+  def load_overrides


~~probably should be a private method.~~ Didn't realize this was in the private section, so ignore :)

Fryguy · 2018-07-10T16:07:10Z

app/helpers/application_helper/toolbar/basic.rb

+
  def initialize
+    @loaded = false
    @definition = {}


Unset both of these and let the accessor deal with the lazy loading on first access.

slemrmartin · 2018-07-10T16:22:19Z

@martinpovolny I'm not sure what exactly you need.
It seems like you want to make dependency from core to amazon and use it from ui gem?
With the autoload_paths you mentioned above in core's application.rb, you can use ::ToolbarOverrides::.., without provider prefix

martinpovolny · 2018-07-11T07:30:16Z

@slemrmartin: What I need to do:

We have toolbar classes in the form of ApplicationHelper::Toolbar::CloudTenantCenter. These toolbar classes contain a tooolbar definition. CloudTenantCenter is what I call here a toolbar_name.

The providers will have a way of extending the toolbar class. So I need to query and load class in the form of:

<provider_namespace>::ToolbarOverrides::<toolbar_name>

In case of Amazon provider and the CloudTenantCenter toolbar it would be:

ManageIQ::Providers::Amazon::ToolbarOverrides::CloudTenantCenter

I have no problem doing that with a require statement:

    Vmdb::Plugins.collect do |plugin|                                                               
      name = plugin.root.join('app/toolbar_overrides', self.class.name.to_s.split('::').last.underscore)
      if File.exist?("#{name}.rb")
        require "#{name}.rb"                                                                  
        instance = [plugin.name.chomp('::Engine'), 'ToolbarOverrides', self.class.name.to_s.split('::').last].join('::')
        instance.constantize                                                                        
      end                                                                                           
    end.compact

And placing the toolbar override classes in the provider repo under 'app/toolbar_overrides'. That works just fine and I am happy.

However I'd like to do that w/o that require statement with the Rails autoloading mechanism.

I am looking for a way

how to place the files with the classes
how to query the existing classes (so that I can merge the toolbar definitions)
how to name the classes and wrapping modules etc.

To get the same or similar effect but w/o the require statement using the Rails autoloader.

So far I have no success. I am obviously structuring the modules and classes and files in a way that is not compatible with how the autoloading works. Help welcome.

slemrmartin · 2018-07-11T08:31:53Z

@martinpovolny I think you need to follow namespaces when building directory structure - for autoloader
I think this should work:

in plugin manageiq-providers-amazon:
/lib/manageiq/providers/amazon/engine.rb

...
class Engine < ::Rails::Engine
...
  config.autoload_paths << File.expand_path('../../../../app/toolbar_overrides', __FILE__)

then you can have:
/app/toolbar_overrides/manageiq/providers/amazon/toolbar_overrides/cloud_tenant_center.rb

class ManageIQ::Providers::Amazon::ToolbarOverrides::CloudTenantCenter
end

martinpovolny · 2018-07-11T10:30:22Z

Problem solved. Thx @slemrmartin, @Fryguy, @Ladas !

* Toolbar::Basic is moved to a base class Toolbar::Base * Toolbar::Override (practically Toolbar::Base) is where the provider extentions should inherit from. * Toolbar::Basic newly includes the extention mechanism.

martinpovolny · 2018-07-11T14:23:32Z

app/helpers/application_helper/toolbar/basic.rb

+    # "ManageIQ::Providers::Amazon::ToolbarOverrides::EmsCloudCenter" is a match for
+    #   "ManageIQ::Providers::Amazon::CloudManager
+    extension_classes.find_all do |ext|
+      ext.name.split('::')[0..-3] == record.class.name.split('::')[0..-2]


@Ladas, @slemrmartin : Do we have a better way to do the matching?

You compare just the ManageIQ::Providers::Amazon, right? So maybe
ext.name.split('::')[0..2] == record.class.name.split('::')[0..2] looks less fragile

Not sure about a better way

martinpovolny · 2018-07-12T08:10:05Z

@himdel, can you, please, review and merge this one?

miq-bot · 2018-07-12T08:12:38Z

Checked commits martinpovolny/manageiq-ui-classic@bb21aa7~...787442e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🍰

Fryguy · 2018-07-16T17:08:43Z

I would have liked a second chance to review this, considering I had issues 😕

martinpovolny · 2018-07-16T17:56:26Z

@Fryguy : sorry for that, will address any feedback in a separate PR.

mzazrivec · 2018-07-17T06:08:03Z

@Fryguy Whups, sorry about that. I'm sure we can address any problems in future PRs.

martinpovolny added enhancement wip gaprindashvili/no labels Jul 9, 2018

himdel mentioned this pull request Jul 9, 2018

[WIP] Add JS in general, amazon-security-group component ManageIQ/manageiq-providers-amazon#462

Closed

Fryguy reviewed Jul 10, 2018

View reviewed changes

Implement toolbar extension mechanism for providers.

bb21aa7

* Toolbar::Basic is moved to a base class Toolbar::Base * Toolbar::Override (practically Toolbar::Base) is where the provider extentions should inherit from. * Toolbar::Basic newly includes the extention mechanism.

martinpovolny force-pushed the toolbar_override branch from 403126f to bb21aa7 Compare July 11, 2018 13:50

martinpovolny changed the title ~~[WIP] Work on toolbar pluggability from Provider repos~~ Implement toolbar extension mechanism for providers Jul 11, 2018

miq-bot removed the wip label Jul 11, 2018

martinpovolny commented Jul 11, 2018

View reviewed changes

martinpovolny requested a review from himdel July 12, 2018 08:08

martinpovolny assigned himdel Jul 12, 2018

Toolbar: change class matching according to PR review.

787442e

mzazrivec assigned mzazrivec and unassigned himdel Jul 16, 2018

mzazrivec added this to the Sprint 90 Ending Jul 16, 2018 milestone Jul 16, 2018

mzazrivec merged commit 7711a87 into ManageIQ:master Jul 16, 2018

This was referenced Jul 18, 2018

[WIP] Toolbar overrides ManageIQ/manageiq-providers-amazon#460

Closed

[DO NOT MERGE] Initial support for AnsibleRunner-driven CRUD ManageIQ/manageiq-providers-nuage#103

Closed

Implement toolbar extension mechanism for providers #4262

Implement toolbar extension mechanism for providers #4262

Uh oh!

Conversation

martinpovolny commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Provider workflow

Goal of this PR

How it is done?

Uh oh!

martinpovolny commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinpovolny commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fryguy Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slemrmartin commented Jul 10, 2018

Uh oh!

martinpovolny commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slemrmartin commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinpovolny commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinpovolny commented Jul 12, 2018

Uh oh!

miq-bot commented Jul 12, 2018

Uh oh!

Fryguy commented Jul 16, 2018

Uh oh!

martinpovolny commented Jul 16, 2018

Uh oh!

mzazrivec commented Jul 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

martinpovolny commented Jul 9, 2018 •

edited

Loading

martinpovolny commented Jul 10, 2018 •

edited

Loading

Fryguy Jul 10, 2018 •

edited

Loading

martinpovolny commented Jul 11, 2018 •

edited

Loading

slemrmartin commented Jul 11, 2018 •

edited

Loading