PR#4862: Add PAGURE_PLUGIN setting in Pagure configuration file. - pagure

pagure

#4862 Add PAGURE_PLUGIN setting in Pagure configuration file.

Merged 3 years ago by pingou. Opened 3 years ago by xyzzyx.

xyzzyx/pagure master into master

Add PAGURE_PLUGINS_CONFIG setting in pagure configuration file.

zPlus • 3 years ago

4479b18

doc/configuration.rst

file modified

+17 -3

		`@@ -492,7 +492,6 @@`
		`below)`


		`-`
		`Web-hooks notifications`
		`-----------------------`

		`@@ -510,6 +509,7 @@`

		`.. _redis-section:`

		`+`
		`Redis options`
		`-------------`

		`@@ -538,7 +538,6 @@`
		Defaults to: ``0``.


		`-`
		`Authentication options`
		`----------------------`

		`@@ -604,7 +603,6 @@`
		Defaults to: ``None``.


		`-`
		`Stomp Options`
		`-------------`

		`@@ -2086,6 +2084,13 @@`
		Defaults to: ``False``.


		`+ PAGURE_PLUGINS_CONFIG`
		`+ ~~~~~~~~~~~~~~~~~~~~~~`
		`+`
		`+ This option can be used to specify the configuration file used for loading`
		`+ plugins. It is not set by default, instead if must be declared explicitly.`
		+ Also see the documentation on plugins at :ref:`plugins`.
		`+`

		`Deprecated configuration keys`
		`-----------------------------`
		`@@ -2188,3 +2193,12 @@`
		`This configuration key allowed specifying the gitolite backend.`
		`This has now been replaced by GIT_AUTH_BACKEND, please see that option`
		`for information on valid values.`
		`+`
		`+ PAGURE_PLUGIN`
		`+ ~~~~~~~~~~~~~`
		`+`
		`+ This configuration key allows to specify the path to the plugins configuration`
		`+ file. It is set as an environment variable. It has been replaced by`
		`+ PAGURE_PLUGINS_CONFIG. The new variable does not modify the behavior of the old`
		`+ variable, however unlike PAGURE_PLUGIN it can be set in the main Pagure`
		`+ configuration.`

doc/index.rst

file modified

		`@@ -40,6 +40,7 @@`
		`install_pagure_logcom`
		`install_crons`
		`configuration`
		`+ plugins`
		`custom_gitolite_conf`
		`development`
		`contributing`

doc/plugins.rst

file added

+43

		`@@ -0,0 +1,43 @@`
		`+ .. _plugins:`
		`+`
		`+ Plugins`
		`+ =======`
		`+`
		`+ Pagure provides a mechanism for loading 3rd party plugins in the form of Flask`
		`+ Blueprints. The plugins are loaded from a separate configuration file that is`
		`+ specified using the PAGURE_PLUGINS_CONFIG option. There are at least two`
		`+ reasons for keeping plugins initialization outside the main pagure`
		`+ configuration file:`
		`+`
		`+ #. avoid circular dependencies errors. For example if the pagure configuration`
		`+ imports a plugin, which in turn imports the pagure configuration, the plugin`
		`+ could try to read a configuration option that has not been imported yet and`
		`+ thus raise an error`
		`+ #. the pagure configuration is also loaded by other processes such as Celery`
		`+ workers. The Celery tasks might only be interested in reading the`
		`+ configuration settings without having to load any external plugin`
		`+`
		`+`
		`+ Loading the configuration`
		`+ -------------------------`
		`+`
		`+ The configuration file can be loaded by setting the variable`
		+ ``PAGURE_PLUGINS_CONFIG`` inside the pagure main configuration file, for
		+ example inside ``/etc/pagure/pagure.cfg``. Alternatively, it is also possible
		+ to set the environment variable ``PAGURE_PLUGINS_CONFIG`` before starting the
		`+ pagure server. If both variables are set, the environment variable takes`
		`+ precedence over the configuration file.`
		`+`
		`+`
		`+ The configuration file`
		`+ ----------------------`
		`+`
		`+ After Pagure has imported the configuration file defined in`
		`+ PAGURE_PLUGINS_CONFIG it will look for Flask Blueprints in a variable called`
		+ ``PLUGINS`` defined in the same file, for example
		+ ``PLUGINS = [ plugin1.blueprint, plugin2.blueprint, ... ]``. Pagure will then
		`+ proceed to register any Blueprint into the main Flask app, in the same order as`
		+ they are listed in ``PLUGINS``.
		`+`
		+ An example configuration can be seen in ``files/plugins.cfg.sample`` inside
		`+ the Pagure repository.`

files/pagure.cfg.sample

file modified

		`@@ -241,3 +241,7 @@`
		`# 'push_cert': {'cert': '',`
		`# 'key': ''}}`
		`REPOSPANNER_REGIONS = {}`
		`+`
		`+ # Path to the plugins configuration file that is used to load plugins. Please`
		`+ # look at files/plugins.cfg.sample for a configuration example.`
		`+ # PAGURE_PLUGINS_CONFIG = "/etc/pagure/plugins.cfg"`

files/plugins.cfg.sample

file added

+35

		`@@ -0,0 +1,35 @@`
		`+ # This file demonstrates how to load plugins in pagure.`
		`+ # Pagure uses Flask Blueprints as plugins, so what we need to do is import all`
		`+ # the Blueprints into a variable called PLUGINS.`
		`+ # See the "Plugins" section in the pagure documentation for more information.`
		`+ ###############################################################################`
		`+`
		`+`
		`+ import os`
		`+ import sys`
		`+`
		`+`
		`+ # For plugins that are already available in sys.path, for example packages that`
		`+ # have been installed on the system, we simply import them`
		`+ import plugin1`
		`+ import plugin2`
		`+ ...`
		`+`
		`+`
		`+ # For any other custom plugin that is not in sys.path we need to add our`
		`+ # folder to sys.path first`
		`+ PLUGINS_PATH = "/path/to/plugins/folder/"`
		`+ if PLUGINS_PATH not in sys.path:`
		`+ sys.path.append(PLUGINS_PATH)`
		`+`
		`+`
		`+ # Then we can import all the plugins`
		`+ import myplugin1`
		`+ import myplugin2`
		`+ ...`
		`+`
		`+`
		`+ # Finally, create the PLUGINS list of Blueprints that we want pagure to register`
		`+ PLUGINS = [ plugin1.APP,`
		`+ myplugin2.APP,`
		`+ ... ]`

pagure/flask_app.py

file modified

+24

		`@@ -16,6 +16,7 @@`
		`import string`
		`import time`
		`import os`
		`+ import warnings`

		`import flask`
		`import pygit2`
		`@@ -132,7 +133,30 @@`
		`# Import 3rd party blueprints`
		`plugin_config = flask.config.Config("")`
		`if "PAGURE_PLUGIN" in os.environ:`
		`+ # Warn the user about deprecated variable (defaults to stderr)`
		`+ warnings.warn(`
		`+ "The environment variable PAGURE_PLUGIN is deprecated and will be "`
		`+ "removed in future releases of Pagure. Please replace it with "`
		`+ "PAGURE_PLUGINS_CONFIG instead.",`
		`+ FutureWarning,`
		`+ )`
		`+`
		`+ # Log usage of deprecated variable`
		`+ logger.warning(`
		`+ "Using deprecated variable PAGURE_PLUGIN. "`
		`+ "You should use PAGURE_PLUGINS_CONFIG instead."`
		`+ )`
		`+`
		`plugin_config.from_envvar("PAGURE_PLUGIN")`
		`+`
		`+ elif "PAGURE_PLUGINS_CONFIG" in os.environ:`
		`+ plugin_config.from_envvar("PAGURE_PLUGINS_CONFIG")`
		`+`
		`+ elif "PAGURE_PLUGINS_CONFIG" in app.config:`
		`+ # If the os.environ["PAGURE_PLUGINS_CONFIG"] is not set, we try to load`
		`+ # it from the pagure config file.`
		`+ plugin_config.from_pyfile(app.config.get("PAGURE_PLUGINS_CONFIG"))`
		`+`
		`for blueprint in plugin_config.get("PLUGINS") or []:`
		`logger.info("Loading blueprint: %s", blueprint.name)`
		`app.register_blueprint(blueprint)`

runserver.py

file modified

+18 -3

		`@@ -1,12 +1,17 @@`
		`#!/usr/bin/env python`

		`+ ###############################################################################`
		`+ # Please note that this script is only used for development purposes. #`
		`+ # Do not start the Flask app with this script in a production environment, #`
		`+ # use files/pagure.wsgi instead! #`
		`+ ###############################################################################`
		`+`
		`from __future__ import unicode_literals, absolute_import`

		`import argparse`
		`import sys`
		`import os`

		`-`
		`parser = argparse.ArgumentParser(description="Run the Pagure app")`
		`parser.add_argument(`
		`"--config",`
		`@@ -15,7 +20,12 @@`
		`help="Configuration file to use for pagure.",`
		`)`
		`parser.add_argument(`
		`- "--plugins", dest="plugins", help="Configuration file for pagure plugin."`
		`+ "--plugins",`
		`+ dest="plugins",`
		`+ help="Configuration file for pagure plugins. This argument takes "`
		`+ "precedence over the environment variable PAGURE_PLUGINS_CONFIG, which in "`
		`+ "turn takes precedence over the configuration variable "`
		`+ "PAGURE_PLUGINS_CONFIG.",`
		`)`
		`parser.add_argument(`
		`"--debug",`
		`@@ -65,7 +75,12 @@`
		`if not config.startswith("/"):`
		`here = os.path.join(os.path.dirname(os.path.abspath(__file__)))`
		`config = os.path.join(here, config)`
		`- os.environ["PAGURE_PLUGIN"] = config`
		`+ os.environ["PAGURE_PLUGINS_CONFIG"] = config`
		`+`
		`+ # If this script is ran with the deprecated env. variable PAGURE_PLUGIN`
		`+ # and --plugins, we need to override it too.`
		`+ if "PAGURE_PLUGIN" in os.environ:`
		`+ os.environ["PAGURE_PLUGIN"] = config`

		`if args.perfverbose:`
		`os.environ["PAGURE_PERFREPO"] = "true"`

xyzzyx commented 3 years ago

Plugins are imported from a separate configuration file that is loaded
either by setting a PAGURE_PLUGIN environment variable, or with the
--plugins flag of the runserver.py script.
This change introduces a new variable called PAGURE_PLUGIN in the Pagure configuration file. The new variable allows to specify the plugins configuration file directly in the main configuration.

ngompa commented on line 12 of pagure/default_config.py 3 years ago

Please set the example to /etc/pagure/pagure_plugins.cfg. This prevents it from being byte-compiled, among other things.

Edited 3 years ago by ngompa

ngompa commented on line 8 of files/pagure.cfg.sample 3 years ago

PAGURE_PLUGIN_CONFIG, please?

ngompa commented 3 years ago

@xyzzyx Aside from my two comments, this looks pretty good. :100:

xyzzyx commented 3 years ago

PAGURE_PLUGIN_CONFIG

I can change it, no problem. I wonder though, wouldn't it be better to keep the same name of the environment variable since they are they same thing? The only difference is that the variable is either set in the config file or in the environment. Having 2 names for the same thing could be confusing. A different option is to change the environment variable to PAGURE_PLUGIN_CONFIG but this change will break any current instance that is using it.

ngompa commented 3 years ago

@xyzzyx We can change it and deprecate the old name, slate that for removal in 6.0. Until that point, using either should trigger the same behavior.

rebased onto ba356b9fba6db1a77a8108624aec1609cbbd1a04

3 years ago

ngompa commented 3 years ago

@xyzzyx Can you rename the file in files to have the .cfg.sample extension like how the pagure.cfg.sample file exists?

rebased onto 551424131f4599b656053826d5cf0104d8f3efd2

3 years ago

pingou commented on line 13 of pagure/default_config.py 3 years ago

Since nothing is defined here, I don't think it's really worth adding this here. Having it in the doc/configuration and in files where you've nicely documented it is sufficient, imho

rebased onto ab921abd7a42bbf2e67be1fab143dbf5415b0561

3 years ago

pingou commented on line 2 of doc/plugins.rst 3 years ago

Oh nice, thanks for starting this!

pingou commented on line 61 of doc/configuration.rst 3 years ago

I'm not sure this last line is worth keeping in the doc, it's an internal implementation details (runserver is only there for development purposes, you do not want to use this for a production deployment, it's not even shipped in the releases tarball)

pingou commented on line 5 of pagure/flask_app.py 3 years ago

We should check if PAGURE_PLUGIN is set and emit a DeprecationWarning if it is and keep the plugin_config.from_envvar which is below otherwise we're breaking backward compatibility.

pingou commented on line 35 of runserver.py 3 years ago

That's what I meant above, this warning should be in the pagure code itself, runserver is really just a convenience tool but it's not something people will/should actually use

rebased onto de9fc3e04437128205f0957e6a970135af789a38

3 years ago

xyzzyx commented 3 years ago

plugin_config.from_envvar has not been removed.

Edited 3 years ago by xyzzyx

rebased onto 5edd3160545714ea5c7c02e15753ae59d5a70b98

3 years ago

rebased onto e195618c3d42e3033605ed3df7896f916b1b42ca

3 years ago

xyzzyx commented 3 years ago

@pingou Should I also remove all other mentions of runserver.py in the documentation?

ngompa commented 3 years ago

@xyzzyx Please do so. :smile:

rebased onto e6c8a67ac068077bd1290705891e1772c69a2daa

3 years ago

ngompa commented 3 years ago

:thumbsup:

pingou commented on line 5 of pagure/flask_app.py 3 years ago

After some research, there is a built-in mechanism in python for deprecation warnings as part of the warning module: https://docs.python.org/3/library/warnings.html I believe this is the proper way to do this/.

pingou commented 3 years ago

I have one final comment but I've tested this locally and it works fine so once we've tweaked our warning I think we can get this in :)

xyzzyx commented 3 years ago

I was aware of the warnings module but it's not used anywhere else in Pagure. All the other warnings in Pagure use the logging module via logging.getLogger and just print to the log file. Do you prefer to use the warnings module here? Or maybe both?

ngompa commented 3 years ago

I'd say we should merge as-is and change to using warnings module everywhere in a followup PR.

pingou commented 3 years ago

The other warnings are warnings that are important to log but I think this deserves even being even more prominent since it may impact pagure instance running.

I'm ok with both (I like the belt+suspender approach for these kinds of changes)

rebased onto 3348bc75855a063f78fa082f5cd0ec1f384dea60

3 years ago

ngompa commented 3 years ago

:thumbsup:

rebased onto 1d4781d5a896db7b6ae801790a9d35edb44eb9f0

3 years ago

pingou commented 3 years ago

Looks good to me, I'll give it a final test locally and merge it (but tomorrow at this point)

xyzzyx commented 3 years ago

I'm not sure why the jenkins test has failed.. If I run tests/test_style.py locally it passes. I'll try to commit again.

rebased onto d37e9d6a4046ff43b5a2fff598ad251c86473cdb

3 years ago

xyzzyx commented 3 years ago

@ngompa @pingou Can you please check tests/test_style.py? It passes for me locally but jenkins says it failed. I have no clue why.

Edited 3 years ago by xyzzyx

pingou commented 3 years ago

Flake8 seems to be the issue
You can see the output here: https://ci.centos.org/job/pagure-pr/3443/artifact/pagure/results_fedora-pip-py3/py-test_style/*view*/

Copied here as after a while they disapear from jenkins:

-------------------- >> begin captured stdout << ---------------------
(b"/pagure/pagure/api/__init__.py:36:1: E402 module level import not at top of file\n/pagure/pagure/hooks/files/default_hook.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/default_hook.py'\n/pagure/pagure/hooks/files/git_multimail.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/git_multimail.py'\n/pagure/pagure/hooks/files/mirror.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/mirror.py'\n/pagure/pagure/hooks/files/pagure_block_unsigned.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/pagure_block_unsigned.py'\n/pagure/pagure/hooks/files/pagure_force_commit_hook.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/pagure_force_commit_hook.py'\n/pagure/pagure/hooks/files/pagure_hook.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/pagure_hook.py'\n/pagure/pagure/hooks/files/pagure_hook_requests.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/pagure_hook_requests.py'\n/pagure/pagure/hooks/files/pagure_hook_tickets.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/pagure_hook_tickets.py'\n/pagure/pagure/hooks/files/rtd_hook.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/rtd_hook.py'\n/pagure/pagure/lib/git_auth.py:707:13: E741 ambiguous variable name 'l'\n/pagure/pagure/ui/filters.py:350:21: F504 '...' % ... has unused named argument(s): templ_edited\n/pagure/pagure/ui/repo.py:850:40: E741 ambiguous variable name 'l'\n", None)

--------------------- >> end captured stdout << ----------------------