move plugin initializer code into Vmdb::Plugins #13941
Conversation
the reasoning behind this is, that in a rails console `reload!` will reset all classes and also this Vmdb::Plugins singleton Moving the code that registers all plugins to the initializer makes sure the all plugins are registered even after a reload.
|
@durandom Cannot apply the following label because they are not recognized: enhacement |
|
@miq-bot add_labels enhancement |
|
Checked commit durandom@de3d2ed with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
| initializer :register_vmdb_plugins, :before => :load_vmdb_settings do | ||
| Rails.application.railties.each do |railtie| | ||
| next unless railtie.class.name.start_with?("ManageIQ::Providers::") || railtie.try(:vmdb_plugin?) | ||
| Vmdb::Plugins.instance.register_vmdb_plugin(railtie) |
There was a problem hiding this comment.
I'm fine with moving the logic to Vmdb::Plugins#initialize, but I think we would minimally need to call Vmdb::Plugins.instance in this initializer. Otherwise, I don't think the plugins will have registered by the time Vmdb::Settings.init is called, right?
There was a problem hiding this comment.
calling Vmdb::Plugins.instance will register all plugins. So whoever calls the .instance first, will initialize it.
I think thats the nice improvement, that we dont need an initializer...
--> #0 Vmdb::Plugins.initialize at /Users/hild/src/manageiq/lib/vmdb/plugins.rb:13
ͱ-- #1 Class.new(*args) at /Users/hild/.rbenv/versions/2.3.1/lib/ruby/2.3.0/singleton.rb:142
#2 block in #<Class:Vmdb::Plugins>.block in instance at /Users/hild/.rbenv/versions/2.3.1/lib/ruby/2.3.0/singleton.rb:142
ͱ-- #3 Thread::Mutex.synchronize at /Users/hild/.rbenv/versions/2.3.1/lib/ruby/2.3.0/singleton.rb:140
#4 #<Class:Vmdb::Plugins>.instance at /Users/hild/.rbenv/versions/2.3.1/lib/ruby/2.3.0/singleton.rb:140
#5 #<Class:Vmdb::Settings>.template_roots at /Users/hild/src/manageiq/lib/vmdb/settings.rb:118
#6 #<Class:Vmdb::Settings>.template_sources at /Users/hild/src/manageiq/lib/vmdb/settings.rb:125
#7 block in #<Class:Vmdb::Settings>.block in build_template at /Users/hild/src/manageiq/lib/vmdb/settings.rb:82
ͱ-- #8 Kernel.tap at /Users/hild/src/manageiq/lib/vmdb/settings.rb:81
#9 #<Class:Vmdb::Settings>.build_template at /Users/hild/src/manageiq/lib/vmdb/settings.rb:81
#10 #<Class:Vmdb::Settings>.build_without_local(resource#Symbol) at /Users/hild/src/manageiq/lib/vmdb/settings.rb:109
#11 #<Class:Vmdb::Settings>.build(resource#Symbol) at /Users/hild/src/manageiq/lib/vmdb/settings.rb:88
#12 #<Class:Vmdb::Settings>.for_resource(resource#Symbol) at /Users/hild/src/manageiq/lib/vmdb/settings.rb:57
#13 #<Class:Vmdb::Settings>.init at /Users/hild/src/manageiq/lib/vmdb/settings.rb:16
#14 block in block in <class:Application> at /Users/hild/src/manageiq/config/application.rb:118
There was a problem hiding this comment.
I don't think this answers the question, but it's difficult to tell. The :load_vmdb_settings initializer creates the toplevel Settings instance and won't be reloaded in production unless something changes the actual setting values. If the plugin is providing its own settings.yml, those have to be in place before the :load_vmdb_settings initializer is called. This is why initializer :register_vmdb_plugins, :before => :load_vmdb_settings
There was a problem hiding this comment.
reloaded in production
Why would anything be reloaded in production? And even if, by moving the logic into the Plugins#initialize it is always initiliazed correctly vs having it being setup in an initializer, which only is executed at boot time.
providing its own settings.yml, those have to be in place before the :load_vmdb_settings initializer is called
But they are. Because they are in Plugins#initialize - which ultimately makes sure, the plugins are registered before something queries them...
So, I think my response answers the question 🤷♂️
|
@bdunne does my reply make sense? I don't see a reason for keeping an initializer... merge? |
| Rails.application.railties.each do |railtie| | ||
| next unless railtie.class.name.start_with?("ManageIQ::Providers::") || railtie.try(:vmdb_plugin?) | ||
| register_vmdb_plugin(railtie) | ||
| end |
There was a problem hiding this comment.
If reload! is reinstantiating this class, then won't it reregister things like settings paths, automate domains and such? This seems dangerous.
There was a problem hiding this comment.
it doesnt? I thought thats whats done in register_vmdb_plugin ?!
There was a problem hiding this comment.
Oh, sorry, I mis-read your comment.
Indeed it would add the engine root again to DescendantLoader.instance.descendants_paths here - I can fix that by making it a proper method that does not allow double entries. But it makes does no real harm for now...
register_automate_domain only adds new AutomateDomain to a newly created array @registered_automate_domains
|
The initial reason for having this initializer ordering was to make sure the settings initializer had access to What if the : load_inflections initializer suddenly needs By moving it to Maybe I'm missing something very subtle here, but for me this reduces dependencies |
|
@Fryguy @bdunne I leave it up to you to merge or close this. I still think this makes sense because it prevents developer confusion and bug hunting when they Maybe even without a console reload, but also in rails auto class reloading in devel mode Maybe even in production if a initializer dependency sneaks in (se my last comment) |
|
@bdunne sorry for resurrecting an old PR. I still think this is a good improvement. If you dont feel like it, just close the PR, no offence taken |
|
This pull request is not mergeable. Please rebase and repush. |
|
This pull request has been automatically closed because it has not been updated for at least 6 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! |
the reasoning behind this is, that in a rails console
reload!willreset all classes and also this Vmdb::Plugins singleton
Moving the code that registers all plugins to the initializer makes
sure the all plugins are registered even after a reload.
I think it will save future developers some frustration, when doing stuff in the console or even development mode, when more and more parts of the app rely on the arrays
@vmdb_pluginsand@registered_automate_domainsto be filled.Its also safe to assume the singleton is accessed after all rails initializers are run - if there is a way to check this, I could add it to the initializer of the singleton class.
@miq-bot add_labels developer, enhacement
@miq-bot assign @bdunne
cc @Fryguy