Skip to content
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

[WIP] Load Vmdb::Plugins via bundler #15461

Closed

Conversation

NickLaMuro
Copy link
Member

Creates a registry interface for loading Vmdb::Plugins, adds not only the existing Rails Engine mechanism as one of the registry adapters (still the default), but includes an additional registry interface for loading via Bundler.

This also lays the groundwork for a more verbose plugin registry system that is "pluggable", and hopefully less dependent on needing "all the things" to run. This is in no way implemented in this PR, but will hopefully help facilitate that in the future™.

Steps for Testing/QA

Ideally this is a low risk PR, so nothing should change by default with these changes.

Takes the existing mechanism for determining Vmdb::Plugins, and moves
the logic into it's own class with a interface that can be repeated with
other Registry providers (working title...)

This can allow other things to provide the plugin registry, like
bundler, or a yaml config instead of relying on Rails to do that for us.

This doesn't remove the concept of using Rails engines to additional
components to our routes and such, but removes it from being a
dependency for defining plugins in the manageiq application.
@miq-bot miq-bot added the wip label Jun 28, 2017
@NickLaMuro NickLaMuro mentioned this pull request Jun 28, 2017
10 tasks
Uses Bundler as the source for determining vmdb plugins to be loaded.
@NickLaMuro NickLaMuro force-pushed the load_vmdb_plugins_via_bundler branch from 2524a5c to 5c36ea1 Compare June 28, 2017 00:28
@miq-bot
Copy link
Member

miq-bot commented Jun 28, 2017

Checked commits NickLaMuro/manageiq@383eb13~...5c36ea1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

end

def plugin_gems
::Bundler.locked_gems.specs.select do |spec|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, dumb question... Is this the main or only place we're talking to Bundler? I guess I was expecting more based on the namespacing and PR title. I want to make sure I'm not missing something.

Copy link
Member Author

@NickLaMuro NickLaMuro Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrafanie Yes, and this is also only used in fetch_plugins.

Sidenote: I originally appended the map {|spec| Plugin.new to the end of this block, and had all of it inside of the fetch_plugins method, but "rubocop didn't like that" (he says in a whiney sarcastic voice).

The reason I mention that this was originally in fetch_plugins is because Vmdb::Plugins::Registry::Rails.fetch_plugins is also the only place that Rails.application is ever accessed as well in it's respective class. The only difference is that the engine interface exposes root and name directly, which is what we end up using to build the plugin map, but a "proxy interface", aka Vmdb::Plugins::Registry::Bundler::Plugin class above, was needed to allow that same interface to exist for the bundler one.

NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Jul 14, 2017
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Jul 14, 2017
@NickLaMuro
Copy link
Member Author

So I think similar to what I said in my recent comment in #15336 is valid here, and whether or not this should be closed should be determined by the result of that.

That said, I think the [WIP] can basically be removed at this point, and we should just re-merge master and see if tests pass. That said, there is probably a larger discussion that is necessary to determine if doing this makes sense, but that can happen after the comment from above is addressed.

@miq-bot
Copy link
Member

miq-bot commented Mar 7, 2018

This pull request is not mergeable. Please rebase and repush.

@NickLaMuro
Copy link
Member Author

Closing this in favor of #17643 since that seems to be about read to be merged.

Honestly, was just waiting for this to hit the 1 year mark before I closed it. Totes serious, mmkay.

...

...

...

:trollface:

@NickLaMuro NickLaMuro closed this Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants