Pluggy based Plugins #311

justanr · 2017-08-13T12:51:29Z

Addresses #183

WIP PR for adding pluggy based plugins to FlaskBB.

Currently this includes:

Defining some specs for plugins to implement (by no means exhaustive)
Creates manager and loads plugins when application is created -- after extensions
Creates a DB interface for toggling plugins enabled
Creates KV store in the database for plugins to store settings in so they don't need to populate the settings table -- these appear as a dictionary like interface on the plugin registry instance for the plugin (that's all the collection class and association proxy stuff in the PluginStore/PluginRegistry relationship)

I'm having some issues with loading CLI commands from plugins because the Flask-Click bridge only creates an application when absolutely needed, however plugins are loaded on application creation, so even with overriding the _load_plugin_commands hook in the FlaskGroup doesn't work as the FlaskBB instance hasn't been created yet.

Next steps here are:

Documentation for writing pluggy based plugins
Converting Portal to pluggy style
Add hooks for rendering HTML fragments into templates at certain points
Add hooks for pre/post handling on posting, topic, message, etc.
Figure out how to load CLI commands.

justanr · 2017-08-13T12:52:07Z

flaskbb/app.py

-from flaskbb.extensions import (db, login_manager, mail, cache, redis_store,
-                                debugtoolbar, alembic, themes, plugin_manager,
-                                babel, csrf, allows, limiter, celery, whooshee)
+from flaskbb.extensions import (alembic, allows, babel, cache, celery, csrf,


Bit of noise here from when the pluggy instance was created here instead of on the application.

but it's sorted alphabetically now :)

justanr · 2017-08-13T12:54:03Z

flaskbb/app.py

+        with app.app_context():
+            plugins = PluginRegistry.query.all()
+
+    except OperationalError:


This is thrown if the table isn't created yet, so upgrades won't bomb. It can potentially be removed later, but I'd need to test a fresh install without it. My gut says the install would fail, but it's been wrong before.

sh4nks · 2017-09-03T15:59:21Z

Looks good so far!

A few more things that need to be addressed (I'll update this list when I find something):

Make translations work with pluggy
Make migrations work with pluggy
Convert AdminCP to use 'pluggy' instead of 'Flask-Plugins'
- Display Plugins in Overview
- Enable/Disable Plugins in AdminCP

sh4nks · 2017-09-04T22:11:23Z

I am using the metadata from setup.py to display some info about the plugin in the admin cp. I removed a few cli commands for now (install/enable/etc) except flaskbb plugins list which will display all enabled and disabled plugins using the package name - later I'll probably make the switch to just display the plugin name.

Un/installing the fixtures and enabling/disabling plugins should also work via the admin cp although it's not very user friendly at the moment. changing settings still needs to be done.

justanr · 2017-09-04T22:44:16Z

flaskbb/plugins/models.py

+    id = db.Column(db.Integer, primary_key=True)
+    key = db.Column(db.Unicode(255), nullable=False)
+    value = db.Column(db.PickleType, nullable=False)
+    # Enum?


Sounds good, do we go the Enum34 route or just an older SQLA enum (think it's either a list of strings or a space separated string)

I'd go for Enum34 and change the data type to ChoiceType as we have sqlalchemy-utils already as dependency.

justanr · 2017-09-04T22:45:55Z

flaskbb/utils/helpers.py

@@ -619,9 +621,20 @@ def __call__(self, environ, start_response):


 def real(obj):
-    """
-    Unwraps a werkzeug.local.LocalProxy object if given one, else returns the object
+    """Unwraps a werkzeug.local.LocalProxy object if given one,


What's our line length? My yapf file is set to 100, but that's only because I copied it from work. Should we stick to 79/80?

I have always tried to stick to 79/80 but in times with widescreen monitors I think that 100 would make sense :)

justanr · 2017-09-04T22:46:47Z

setup.py

@@ -77,7 +77,6 @@ def run_tests(self):
        'Flask-Limiter',


If portal is becoming an external package, should we install it by default?

I think we should install it by default. I have to create a proper package for PyPI first though..

justanr · 2017-09-05T10:28:52Z

with ctx.ensure_object(ScriptInfo).load_app().app_context():

That might be the secret sauce to getting plugin cli commands loaded. However, we'll end up loading the application twice in many instances.

~~Actually wrapping the load plugin commands method with with_appcontext looks like it's working.~~

sh4nks · 2017-09-05T11:51:57Z

Looks like this is the solution. I stumbled upon this the other day and it seems that they are doing something similar.

justanr · 2017-09-05T12:04:21Z

I poked around Indico at the behest of ThiefMaster when I was shopping around for plugin solutions, but yes. That looks like the ticket.

justanr

Some more thoughts.

justanr · 2017-09-07T01:45:14Z

flaskbb/cli/plugins.py

            )
+
+    # TODO: is there a better way for doing this?


Yes, wrap this using with_appcontext and load the information off of current_app.pluggy, with a fix I pushed this'll end up loading plugins twice which might cause issues.

Actually, that shouldn't be needed anymore as we keep track of disabled plugins :) pluggy.list_disabled_plugins() should provide all information needed for this.

justanr · 2017-09-07T01:47:49Z

flaskbb/management/views.py


+    plugin.delete()


I'm wary of this because it only removes the plugin from the blocked list. The next time the application runs, it'll reappear much to the frustration of the user. We should probably include instructions about uninstalling via pip on this page. We should probably also add an uninstall hook for plugins to implement so they can do any decoupling needed.

justanr · 2017-09-07T01:49:14Z

flaskbb/plugins/manager.py

+        for ep in iter_entry_points(entrypoint_name):
+            # is the plugin registered or blocked?
+            if self.get_plugin(ep.name) or self.is_blocked(ep.name):
+                self._disabled_plugins.append((ep.name, ep.dist))


We should only append here if the plugin is actually disabled, not merely already loaded.

Oops, yeah you are right. Thanks for pointing it out :)

justanr · 2017-09-07T01:54:56Z

flaskbb/plugins/models.py

+    value = db.Column(db.PickleType, nullable=False)
+    # Enum?
+    # Available types: string, integer, float, boolean, select, selectmultiple
+    value_type = db.Column(db.Unicode(20), nullable=False)


Just noticed these extra nullable fields break the association_proxy on PluginRegistry. Should we keep that or remove it? At least it breaks for setting.

mhh thats true.. what do you suggest? We could also revamp the way we create the setting forms but thats probably a different issue :).

Mm, not sure. I like the idea of a simple KV store but if we need more information that what we can get out of that, it's not worth trying to make it work. I'm tempted to say we could infer the value's type, but that could lead to issues.

justanr · 2017-09-07T01:57:07Z

flaskbb/utils/translations.py

@@ -38,7 +35,7 @@ def __init__(self, app):
        with self.app.app_context():
            self.plugin_translations = [
                os.path.join(plugin.path, "translations")
-                for plugin in get_enabled_plugins()
+                for plugin in self.app.pluggy.get_plugins()


What if we did self.plugin_translations = self.app.pluggy.load_translations() and have the expectation be the plugin returns a list of translation paths (or maybe a dictionary of language -> path/List[path]). That's all we'd need to complete the translation compilation, right?

Good point! I just blindly changed it to make flaskbb runable again.. I really should stop doing that :P

djsilcock · 2017-10-12T20:24:02Z

I like the idea - I'm not entirely familiar with the inner workings of click but I think all the subcommands would need the same signature ie can't mix
flaskbb plugins <pluginname> <subcommand>
with
flaskbb plugins <subcommand> <pluginname>

This would leave you with something like

flaskbb plugins db myplugin upgrade

which would at least appear under flaskbb plugins --help

On the other hand, it might be an idea not to bombard the end user with all these options, after all most of the db commands are involved in generating migration files rather than applying them - other than db upgrade and db downgrade which might be served better by incorporating them into some kind of plugin install command

Work in progress..

See https://www.python.org/dev/peps/pep-0427/#the-dist-info-directory

justanr · 2017-11-18T13:43:26Z

docs/_static/custom.css

@@ -8,7 +8,7 @@ html {
 body {
  /* Margin bottom by footer height */
  margin-bottom: 60px;
-  padding-top: 50px;
+  /*padding-top: 50px; */ /* uncomment if navbar should be fixed */


Fixed as in "position: fixed"?

if navbar-fixed-top is used

justanr · 2017-11-18T13:45:16Z

docs/hooks.rst

+
+.. currentmodule:: flaskbb.plugins.spec
+
+.. autofunction:: flaskbb_extensions


We should probably give more details about when these are called. I'll knock out some docs on it.

Ok, I'll wait for the merge then.

justanr · 2017-11-18T13:48:54Z

docs/plugin_development.rst

+Each hook specification has a corresponding hook implementation. By default,
+all hooks that are prefix with ``flaskbb_`` will be marked as a standard
+hook implementation. It is possible to modify the behavior of hooks.
+For example, default hooks are called in LIFO registered order.


We should clarify that registration order isn't deterministic -- unless I misunderstood something in the pluggy docs or there's something in setuptools that guarantees that.

justanr · 2017-11-18T14:05:53Z

Wooo, all of this looks good. I'll knock out the changes to the docs above and handle this conflict we have.

# Conflicts: # flaskbb/configs/default.py

List pluggy as a dependency

cfc7d9a

justanr added enhancement plugin labels Aug 13, 2017

justanr self-assigned this Aug 13, 2017

justanr requested a review from sh4nks August 13, 2017 12:51

justanr commented Aug 13, 2017

View reviewed changes

justanr force-pushed the Pluggy branch from c955df4 to bfab284 Compare August 13, 2017 12:54

Define hooks, loader and registry

0511196

justanr force-pushed the Pluggy branch from bfab284 to 0511196 Compare August 13, 2017 12:57

Fix plugin registry repr error

113fbbc

justanr added this to the 2.0 milestone Sep 4, 2017

sh4nks added 2 commits September 4, 2017 23:57

Move portal plugin to seperate repo

a3f4e1b

Further work to replace Flask-Plugins with pluggy

cae9525

justanr commented Sep 4, 2017

View reviewed changes

sh4nks and others added 5 commits September 5, 2017 23:25

Provide method to generate a settings form

d738e70

Subclass PluginManager to add some flaskbb specific stuff

37bb1fb

Don't ignore the plugins folder

a0e99ca

Use the same logic for modifying flaskbb and plugin settings

734f062

Make Plugins work on CLI

9b6ccb7

justanr commented Sep 7, 2017

View reviewed changes

justanr force-pushed the Pluggy branch from fe7acb7 to 9b6ccb7 Compare September 10, 2017 13:02

justanr added 3 commits September 10, 2017 09:02

Merge branch 'master' into Pluggy

63593d3

Make setting value types enum

f503559

Fix merge issues

c66dcc1

sh4nks added 2 commits October 12, 2017 19:05

'removed' is never initialized if config is set to False

bb28295

Display plugin info for disabled plugins

bc8bfe1

sh4nks added 8 commits October 19, 2017 18:54

Allow hooks to be called in templates

2bb6b4b

Working on some docs

605b077

Work in progress..

Cleanup

030efb9

Remove fixed navbar from docs

4454d09

Use singular for enum

ee64c2a

Add missing hooks

75ee7f8

Add proper docs about plugins and hooks

987bfc4

Include hooks and settings in toc

69b035b

sh4nks force-pushed the Pluggy branch from 93b71ee to 69b035b Compare October 31, 2017 14:54

sh4nks added 7 commits November 4, 2017 12:39

Update plugin documentation

6a7ccdb

Add metadata for disabled plugins

104976d

Add option to overwrite plugin settings

f06d02b

Add management commands for plugins

cc2fb24

Check if plugin is enabled before trying it to install

8b234d4

Try METADATA for bdist and PKG-INFO for sdist

b821094

See https://www.python.org/dev/peps/pep-0427/#the-dist-info-directory

Add portal plugin to requirements

d51e23b

sh4nks approved these changes Nov 18, 2017

View reviewed changes

justanr commented Nov 18, 2017

View reviewed changes

justanr added 4 commits November 18, 2017 09:10

Merge branch 'master' into Pluggy

8d6155a

# Conflicts: # flaskbb/configs/default.py

Redate plugin migration so its less confusing

1d7e983

Clarify plugin docs

311f8bf

Use SettingValueType enum in test fixture

9d1dca3

sh4nks merged commit ff6798e into flaskbb:master Nov 18, 2017

sh4nks mentioned this pull request Nov 18, 2017

Plugin System #183

Closed

justanr deleted the Pluggy branch November 19, 2017 00:22

sh4nks added this to Done in 2.0 Getting Hooked Up Mar 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pluggy based Plugins #311

Pluggy based Plugins #311

justanr commented Aug 13, 2017 •

edited

justanr Aug 13, 2017

sh4nks Sep 3, 2017

justanr Aug 13, 2017

sh4nks commented Sep 3, 2017 •

edited

sh4nks commented Sep 4, 2017

justanr Sep 4, 2017

sh4nks Sep 7, 2017

justanr Sep 4, 2017

sh4nks Sep 5, 2017

justanr Sep 4, 2017

sh4nks Sep 5, 2017

justanr commented Sep 5, 2017 •

edited

sh4nks commented Sep 5, 2017

justanr commented Sep 5, 2017

justanr left a comment

justanr Sep 7, 2017

sh4nks Sep 7, 2017

justanr Sep 7, 2017

justanr Sep 7, 2017

sh4nks Sep 7, 2017

justanr Sep 7, 2017

sh4nks Sep 7, 2017

justanr Sep 7, 2017

justanr Sep 7, 2017 •

edited

sh4nks Sep 7, 2017

djsilcock commented Oct 12, 2017

justanr Nov 18, 2017

sh4nks Nov 18, 2017

justanr Nov 18, 2017

sh4nks Nov 18, 2017

justanr Nov 18, 2017

justanr commented Nov 18, 2017


		.. currentmodule:: flaskbb.plugins.spec

		.. autofunction:: flaskbb_extensions

Pluggy based Plugins #311

Pluggy based Plugins #311

Conversation

justanr commented Aug 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sh4nks commented Sep 3, 2017 • edited

sh4nks commented Sep 4, 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

justanr commented Sep 5, 2017 • edited

sh4nks commented Sep 5, 2017

justanr commented Sep 5, 2017

justanr left a 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

justanr Sep 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djsilcock commented Oct 12, 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

justanr commented Nov 18, 2017

justanr commented Aug 13, 2017 •

edited

sh4nks commented Sep 3, 2017 •

edited

justanr commented Sep 5, 2017 •

edited

justanr Sep 7, 2017 •

edited