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

Implement toolbar extension mechanism for providers #4262

Merged
merged 2 commits into from Jul 16, 2018

Conversation

@martinpovolny
Copy link
Contributor

martinpovolny commented Jul 9, 2018

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

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

Example provider PR

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

This comment has been minimized.

Copy link
Contributor

martinpovolny commented Jul 10, 2018

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

This comment has been minimized.

Copy link
Contributor

martinpovolny commented Jul 10, 2018

ping @Fryguy

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

This comment has been minimized.

@Fryguy

Fryguy Jul 10, 2018

Member

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

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

This comment has been minimized.

@Fryguy

Fryguy Jul 10, 2018

Member

attr_reader :definition seems better.

@loaded = true
end

@definition

This comment has been minimized.

@Fryguy

Fryguy Jul 10, 2018

Member

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

This comment has been minimized.

@Fryguy

Fryguy Jul 10, 2018

Member

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 😁

This comment has been minimized.

@martinpovolny

martinpovolny Jul 11, 2018

Contributor

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.

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

This comment has been minimized.

@Fryguy

Fryguy Jul 10, 2018

Member

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.

end.compact
end

def load_overrides

This comment has been minimized.

@Fryguy

Fryguy Jul 10, 2018

Member

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

def initialize
@loaded = false
@definition = {}

This comment has been minimized.

@Fryguy

Fryguy Jul 10, 2018

Member

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

@slemrmartin

This comment has been minimized.

Copy link
Contributor

slemrmartin commented Jul 10, 2018

@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

This comment has been minimized.

Copy link
Contributor

martinpovolny commented Jul 11, 2018

@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

  1. how to place the files with the classes
  2. how to query the existing classes (so that I can merge the toolbar definitions)
  3. 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

This comment has been minimized.

Copy link
Contributor

slemrmartin commented Jul 11, 2018

@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

This comment has been minimized.

Copy link
Contributor

martinpovolny commented Jul 11, 2018

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

Implement toolbar extension mechanism for providers.
 * 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 martinpovolny force-pushed the martinpovolny:toolbar_override branch from 403126f to bb21aa7 Jul 11, 2018

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

@miq-bot miq-bot removed the wip label Jul 11, 2018

# "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]

This comment has been minimized.

@martinpovolny

martinpovolny Jul 11, 2018

Contributor

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

This comment has been minimized.

@Ladas

Ladas Jul 11, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

martinpovolny commented Jul 12, 2018

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

@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Jul 12, 2018

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. 🍰

@mzazrivec mzazrivec assigned mzazrivec and unassigned himdel Jul 16, 2018

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

2 of 4 checks passed

Scrutinizer Analysis: 180 updated code elements – Tests: failed
Details
codeclimate 6 issues to fix
Details
Hakiri No security warnings were found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Fryguy

This comment has been minimized.

Copy link
Member

Fryguy commented Jul 16, 2018

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

@martinpovolny

This comment has been minimized.

Copy link
Contributor

martinpovolny commented Jul 16, 2018

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

@mzazrivec

This comment has been minimized.

Copy link
Collaborator

mzazrivec commented Jul 17, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment