restored source branch
pr288-vettedToggle commit list
@J08nY started a discussion on an old version of the diff
261 295 # - http -- Internal wsgi-based web interface 262 296 # - locks -- Lock state changes 263 297 # - mischief -- Various types of hostile activity 298 # - plugin -- Plugin logs
@J08nY started a discussion on an old version of the diffsrc/mailman/interfaces/plugin.py 0 → 100644
35 """A plugin hook called in the second initialization step. 36 37 This is called after the database is initialized. 38 """ 39 40 resource = Attribute("""\ 41 The object for use as the root of this plugin's REST resources. 42 43 This is the resource which will be hooked up to the REST API, and 44 served at the /<api>/plugins/<plugin.name>/ location. All parsing 45 below that location is up to the plugin. 46 47 This attribute should be None if the plugin doesn't provide a REST 48 resource. 49 50 The resource must support getattr() and dir().
dir()might not be necessary, as in, nothing I tested broke without it, but I wanted the REST proxy instance which holds the resource and proxies to it, look like the resource the most, in case Falcon does something with
dir()of the object or whatever.
dir()didn't seem to make any difference so I removed it.
Overall awesome changes. Really fixed the things I haven't thought about, however I have some remarks as well.
I don't think making the
[plugin]pathoption a filesystem directory and forcing
[plugin.<name>]to be the package name is the right thing. I chose to make it a package path because it should be possible to setup two plugin instances of the same plugin package with two different configs, so in the original implementation this, is possible:
[plugin.one] path: some_plugin.package class: some_plugin.plugin.RedPlugin enabled: yes configuration: /etc/mailman/red.cfg [plugin.other] path: class: some_plugin.plugin.BluePlugin enabled: yes configuration: /etc/mailman.blue.cfg
And then query the plugins REST endpoints on
Also I think
sys.pathis something a site admin should handle via a proper use of virtual environments or in other ways he/she seems appropriate, but that Mailman should just look into what's already on it when it runs and not alter it based on it's own config.
Thanks for the explanation, this is a great use case we'll want to preserve.
I've been thinking about this. In one sense I agree that
sys.pathshould be under the control of the sysadmin, but I also suspect that they will appreciate the ability to extend the path within Mailman's configuration files, rather than in all the places that would potentially have to call Mailman.
I also think that if we provide that functionality, it should be outside the plugin configs. As I want to think about that some more, I'll remove
pathfrom this PR and add it to a separate one when the time comes.
What about an option under the
[mailman]section, something like
[mailman]pathswhere a site admin can list all the directories that should get added to sys.path. Also this should be done as soon in the initialization process as possible.
Then in the plugin config:
[plugin.one] package: some_package.whatever class: some_package.plugin.Plugin enabled: yes configuration: /etc/mailman/one.cfg
That solution allows the admin to modify
sys.pathin the Mailman config, as well as keeps the option of specifying the plugin package and plugin name separately(so many instances of same plugin class possible).
package:specifies the root package of where Mailman should look for pluggable components, and
class:specifies the class to load as the plugin instance (and run hooks on, and get REST from).
classin my example has
packageas the root part of the path, I didn't want to enforce this. For example:
[plugin.some] # some_components dir looks like this: # - rules # - this_rule.py # - that_rule.py # - chains # - this_that_chain.py package: some_multi_package.some_components class: some_multi_package.some.SomePlugin enabled: yes configuration: /var/somewhere [plugin.other] # other_compoments dir looks like this: # - handlers # - what_a_handler.py # - pipelines # - pipeline.py package: some_multi_package.other_components class: some_multi_package.other.OtherPlugin enabled: yes configuration: /var/other_config
So that allows a plugin creator to structure his plugin package in any way he/she wants, the only thing necessary is to tell Mailman where is the root of the standard plugin structure(
path:in the original MR) and what class to load(
class:) and run hooks, and get REST from.
@J08nY Thanks, but I think I'm still missing something. In the first example, wouldn't the directory
some_multi_packagehave to be on
sys.path? And that directory would have a
some_componentsdirectory under that. But then where does the
some(directory? module?) come from in the directory structure, and of what use is
some_components? Can you lay out the full directory structure of the above example so I can see how the Python dot-paths in
classcorrespond to the filesystem layout?
@warsaw Okay, so imagine a plugin package with a dir structure as so:
plugin_package - blue_components - handlers - some_handler <- module - pipelines - some_pipeline <- module - styles - some_style <- module - red_components - rules - some_rule <- module - chains - some_chain <- module - styles - some_style <- module - red_plugin <- module - blue_plugin <- module - yellow_plugin <- module
Then the plugin author publishes said package on PyPI. A site admin then installs then install the plugin (so that it is on
sys.path), and can then choose whether he/she would like to enable:
- only the chains + rules components (by creating a plugin configuration with
- or the handlers + pipelines components (by creating a plugin configuration with
- or not using any components and only the instanciating the yellow plugin (by creating a plugin configuration with
It should be possible to make this all one option and just force the path from where the plugin class gets loaded to
<package>.pluginand that implements
IPlugin. However this way it allows for these multi-packages to exist, where for example the
blue_componentspackage provides some list style that selects it's own custom pipeline from
red_componentscreates a list style that selects it's own custom chain from
some_chain, the respective
red/blue_pluginclasses provide some basic initialization(for example register events they need to listen to) and the
yellow_plugindoes something completely different and doesn't need any components.
- only the chains + rules components (by creating a plugin configuration with
@J08nY Okay thanks! I think I see what you're saying now. Let me slightly rephrase that to see if I understand what you're getting at.
IPluginimplementations define only pre- and post- hooks, and the REST resource at their root.
Other pluggable components are auto-detected by searching for the component directory under the plugin name, and I think this is where your suggested layout and configuration options come into play. You'd like to be able to control which components get loaded by which plugins.
Do I understand correctly?
The way you're suggesting will work, but I want to think about whether we can do it differently. I'm inclined to say that it should be more explicit than in my branch, but I want to get the configuration and layout right so it can be easily explained and documented.
One hesitation that I have is that if you wanted, say the same rule enabled in both
red_componentsyou couldn't easily share that between then (short of symlinks, or copying the file, both of which are a bit icky to require). Also in your example, you wouldn't be able to easily share styles or enable the same styles in both component directories.
I've been thinking about this some more and I think we're conflating two configurations that should be separated. First, we're talking about
IPluginimplementations, which provide hooks and a REST resource. Second, we're defining some paths that get searched for additional components like rules, chains, styles, etc. As a shorthand, we're currently using plugins as an anchor point for searching for those extra components, and that's the part I'd like to separate.
In #390 we're already going to separately extend
sys.pathand it seems like we could similarly add component search paths. I'm not sure we want to put this in
[paths]but for now let's say we do. If we slightly rearrange your directory layout:
.../plugin_package - blue_components - handlers - some_handler <- module - pipelines - some_pipeline <- module - styles - some_style <- module - red_components - rules - some_rule <- module - chains - some_chain <- module - styles - some_style <- module - plugins - red_plugin <- module - blue_plugin <- module - yellow_plugin <- module
Then we'd add something like this to our
[paths.foo] sys_path_extensions: .../plugin_package/plugins .../plugin_package/red_components [plugin.example] class: red_plugin.Red enable: yes
That would give us both the
Redplugin and the
red_components. Of course we could do something like this to give us just the yellow plugin and no components:
[paths.foo] sys_path_extensions: .../plugin_package/plugins [plugin.example] class: yello_plugin.Yellow enable: yes
It doesn't solve the duplication/symlink issue, but I can live with that.
I realized there's one other thing we have to change. Right now
plugin.nameas the package to search for new components. And now I see why you added
packageto the plugin's config.
I've thought about several alternatives, but I'm settling on a hybrid of your approach and the current one.
packagedoesn't convey enough information for me though, so I'm going to try
component_packageand make that optional. If not given, then
plugin.namewill be used.
resolved all discussionsToggle commit list
Sorry for the delayed response, I was at an event for the past three days.
I went through the current state of this MR. The only noteworthy thing I found is that you log to
mailman.pluginin a few places: mailman.plugins.initialize:31 mailman.core.initialize:155 mailman.core.initialize:166 mailman.core.initialize:196 and I think those should log to
IPlugin.resourcedocstring still states that the REST resource has to support
dir(), while as you said, it's not necessary.
Other than that I think it's ready to be merged.
Thanks! I was actually mistaken,
__dir__()is required and wasn't removed. Removing it breaks tests, so I'm leaving it in. I did update the logger names.
I'll merge this after a day or so to see if anybody else responds on the mailing list.
mergedToggle commit list