PR#3978: Make it possible to create hooks that don't have DB entries - pagure

pagure

#3978 Make it possible to create hooks that don't have DB entries

Merged 5 years ago by pingou. Opened 5 years ago by bkabrda.

bkabrda/pagure non-model-hooks into master

Make the default hook be active all the time on all projects

Slavek Kabrda • 5 years ago

adbb5f9

Make it possible to create hooks that don't have DB entries. Fixes #3972

Slavek Kabrda • 5 years ago

370e678

pagure/hooks/__init__.py

file modified

+22 -1

		`@@ -145,6 +145,7 @@`
		`name = None`
		`form = None`
		`description = None`
		`+ backref = None`
		`db_object = None`
		`# hook_type is not used in hooks that use a Runner class, as those can`
		`# implement run actions on whatever is useful to them.`
		`@@ -259,6 +260,26 @@`
		`if not cls.runner:`
		`raise ValueError("BaseHook.remove called for runner-less hook")`

		`+ @classmethod`
		`+ def is_enabled_for(cls, project):`
		`+ """ Determine if this hook should be run for given project.`
		`+`
		`+ On some Pagure instances, some hooks should be run on all projects`
		`+ that fulfill certain criteria. It is therefore not necessary to keep`
		`+ database objects for them.`
		`+`
		`+ If a hook's backref is set to None, this method is run to determine`
		`+ whether the hook should be run or not. These hooks also won't show`
		`+ up on settings page, since they can't be turned off.`
		`+`
		`+ :arg project: The project to inspect`
		`+ :type project: pagure.lib.model.Project`
		`+ :return: True if this hook should be run on the given project,`
		`+ False otherwise`
		`+`
		`+ """`
		`+ return False`
		`+`

		`def run_project_hooks(`
		`session,`
		`@@ -352,7 +373,7 @@`

		`# Now we run the hooks for plugins`
		`haderrors = False`
		`- for plugin, _ in get_enabled_plugins(project, with_default=True):`
		`+ for plugin, _ in get_enabled_plugins(project):`
		`if not plugin.runner:`
		`if debug:`
		`print(`

pagure/hooks/default.py

file modified

+4 -58

		`@@ -13,16 +13,7 @@`
		`import logging`

		`import pygit2`
		`- import sqlalchemy as sa`
		`import six`
		`- import wtforms`
		`-`
		`- try:`
		`- from flask_wtf import FlaskForm`
		`- except ImportError:`
		`- from flask_wtf import Form as FlaskForm`
		`- from sqlalchemy.orm import relation`
		`- from sqlalchemy.orm import backref`

		`import pagure.config`
		`import pagure.exceptions`
		`@@ -31,43 +22,12 @@`
		`import pagure.lib.tasks_services`
		`import pagure.utils`
		`from pagure.hooks import BaseHook, BaseRunner`
		`- from pagure.lib.model import BASE, Project`


		`_config = pagure.config.reload_config()`
		`_log = logging.getLogger(__name__)`


		`- class DefaultTable(BASE):`
		`- """ Stores information about the default hook of a project.`
		`-`
		`- Table -- hook_default`
		`- """`
		`-`
		`- __tablename__ = "hook_default"`
		`-`
		`- id = sa.Column(sa.Integer, primary_key=True)`
		`- project_id = sa.Column(`
		`- sa.Integer,`
		`- sa.ForeignKey("projects.id", onupdate="CASCADE", ondelete="CASCADE"),`
		`- nullable=False,`
		`- unique=True,`
		`- index=True,`
		`- )`
		`- active = sa.Column(sa.Boolean, nullable=False, default=False)`
		`-`
		`- project = relation(`
		`- "Project",`
		`- remote_side=[Project.id],`
		`- backref=backref(`
		`- "default_hook",`
		`- cascade="delete, delete-orphan",`
		`- single_parent=True,`
		`- uselist=False,`
		`- ),`
		`- )`
		`-`
		`-`
		`def send_fedmsg_notifications(project, topic, msg):`
		`""" If the user asked for fedmsg notifications on commit, this will`
		`do it.`
		`@@ -317,19 +277,6 @@`
		`)`


		`- class DefaultForm(FlaskForm):`
		`- """ Form to configure the default hook. """`
		`-`
		`- active = wtforms.BooleanField("Active", [wtforms.validators.Optional()])`
		`-`
		`- def __init__(self, args, *kwargs):`
		`- """ Calls the default constructor with the normal argument but`
		`- uses the list of collection provided to fill the choices of the`
		`- drop-down list.`
		`- """`
		`- super(DefaultForm, self).__init__(args, *kwargs)`
		`-`
		`-`
		`class Default(BaseHook):`
		`""" Default hooks. """`

		`@@ -337,9 +284,8 @@`
		`description = (`
		`"Default hooks that should be enabled for each and every project."`
		`)`
		`-`
		`- form = DefaultForm`
		`- db_object = DefaultTable`
		`- backref = "default_hook"`
		`- form_fields = ["active"]`
		`runner = DefaultRunner`
		`+`
		`+ @classmethod`
		`+ def is_enabled_for(cls, project):`
		`+ return True`

pagure/lib/plugins.py

file modified

+22 -16

		`@@ -15,8 +15,16 @@`
		`from pagure.lib.model_base import BASE`


		`- def get_plugin_names(blacklist=None):`
		`- """ Return the list of plugins names. """`
		`+ def get_plugin_names(blacklist=None, without_backref=False):`
		`+ """ Return the list of plugins names.`
		`+`
		`+ :arg blacklist: name or list of names to not return`
		`+ :type blacklist: string or list of strings`
		`+ :arg without_backref: whether or not to include hooks that`
		`+ have backref "None"`
		`+ :type without_backref: bool`
		`+ :return: list of plugin names (strings)`
		`+ """`
		`from pagure.hooks import BaseHook`

		`plugins = load("pagure.hooks", subclasses=BaseHook)`
		`@@ -26,7 +34,8 @@`
		`blacklist = [blacklist]`

		`output = [`
		`- plugin.name for plugin in plugins if plugin.name not in blacklist`
		`+ plugin.name for plugin in plugins`
		`+ if plugin.name not in blacklist and (plugin.backref or without_backref)`
		`]`
		`# The default hook is not one we show`
		`if "default" in output:`
		`@@ -50,28 +59,25 @@`
		`return plugin`


		`- def get_enabled_plugins(project, with_default=False):`
		`+ def get_enabled_plugins(project):`
		`""" Returns a list of plugins enabled for a specific project.`

		`Args:`
		`project (model.Project): The project to look for.`
		`- with_default (boolean): Wether or not the returned list should`
		`- include the default hook/plugin.`
		`Returns: (list): A list of tuples (pluginclass, dbobj) with the plugin`
		`classess and dbobjects for plugins enabled for the project.`
		`"""`
		`from pagure.hooks import BaseHook`
		`- from pagure.hooks.default import Default`

		`enabled = []`
		`- if with_default:`
		`- enabled = [(Default(), None)]`
		`for plugin in load("pagure.hooks", subclasses=BaseHook):`
		`- if plugin.name == "default":`
		`- continue`
		`- plugin.db_object()`
		`- if hasattr(project, plugin.backref):`
		`- dbobj = getattr(project, plugin.backref)`
		`- if dbobj and dbobj.active:`
		`- enabled.append((plugin, dbobj))`
		`+ if plugin.backref is None:`
		`+ if plugin.is_enabled_for(project):`
		`+ enabled.append((plugin, None))`
		`+ else:`
		`+ plugin.db_object()`
		`+ if hasattr(project, plugin.backref):`
		`+ dbobj = getattr(project, plugin.backref)`
		`+ if dbobj and dbobj.active:`
		`+ enabled.append((plugin, dbobj))`
		`return enabled`

pagure/lib/tasks.py

file modified

-11

		`@@ -299,17 +299,6 @@`
		`master_ref = temp_gitrepo.lookup_reference("HEAD").resolve()`
		`tempclone.push("pagure", master_ref.name, internal="yes")`

		`- # Install the default hook`
		`- plugin = pagure.lib.plugins.get_plugin("default")`
		`- dbobj = plugin.db_object()`
		`- dbobj.active = True`
		`- dbobj.project_id = project.id`
		`- session.add(dbobj)`
		`- session.flush()`
		`- plugin.set_up(project)`
		`- plugin.install(project, dbobj)`
		`- session.commit()`
		`-`
		`task = generate_gitolite_acls.delay(`
		`namespace=project.namespace,`
		`name=project.name,`

tests/test_pagure_flask_ui_plugins_default_hook.py

file modified

+11 -106

		`@@ -26,6 +26,8 @@`
		`sys.path.insert(0, os.path.join(os.path.dirname(`
		`os.path.abspath(__file__)), '..'))`

		`+ import pagure.hooks.default`
		`+ import pagure.lib.plugins`
		`import pagure.lib.query`
		`import tests`

		`@@ -33,116 +35,19 @@`
		`class PagureFlaskPluginDefaultHooktests(tests.Modeltests):`
		`""" Tests for default_hook plugin of pagure """`

		`- def test_plugin_default_ui(self):`
		`- """ Test the default hook plugin on/off endpoint. """`
		`+ def test_plugin_default_active_on_project(self):`
		`+ """ Test that the default hook is active on random project. """`

		`tests.create_projects(self.session)`
		`- tests.create_projects_git(os.path.join(self.path, 'repos'))`
		`-`
		`- user = tests.FakeUser(username='pingou')`
		`- with tests.user_set(self.app.application, user):`
		`- output = self.app.get('/test/settings/default')`
		`- self.assertEqual(output.status_code, 403)`
		`-`
		`- def test_plugin_default_install(self):`
		`- """ Check that the default plugin is correctly installed when a`
		`- project is created.`
		`- """`
		`-`
		`- task = pagure.lib.query.new_project(`
		`- self.session,`
		`- user='pingou',`
		`- name='test',`
		`- repospanner_region=None,`
		`- blacklist=[],`
		`- allowed_prefix=[],`
		`- description=None,`
		`- url=None, avatar_email=None,`
		`- parent_id=None,`
		`- add_readme=False,`
		`- userobj=None,`
		`- prevent_40_chars=False,`
		`- namespace=None`
		`+ test = pagure.lib.query.search_projects(self.session)[0]`
		`+ self.assertIsNone(pagure.hooks.default.Default.backref)`
		`+ self.assertTrue(`
		`+ pagure.hooks.default.Default.is_enabled_for(test)`
		`)`
		`- self.assertEqual(task.get(),`
		`- {'endpoint': 'ui_ns.view_repo',`
		`- 'repo': 'test',`
		`- 'namespace': None})`
		`-`
		`- self.assertTrue(os.path.exists(os.path.join(`
		`- self.path, 'repos', 'test.git', 'hooks', 'post-receive')))`
		`-`
		`- def test_plugin_default_remove(self):`
		`- """ Check that the default plugin can be correctly removed if`
		`- somehow managed.`
		`- """`
		`-`
		`- task = pagure.lib.query.new_project(`
		`- self.session,`
		`- user='pingou',`
		`- name='test',`
		`- repospanner_region=None,`
		`- blacklist=[],`
		`- allowed_prefix=[],`
		`- description=None,`
		`- url=None, avatar_email=None,`
		`- parent_id=None,`
		`- add_readme=False,`
		`- userobj=None,`
		`- prevent_40_chars=False,`
		`- namespace=None`
		`+ self.assertEqual(`
		`+ [(pagure.hooks.default.Default, None)],`
		`+ pagure.lib.plugins.get_enabled_plugins(test)`
		`)`
		`- self.assertEqual(task.get(),`
		`- {'endpoint': 'ui_ns.view_repo',`
		`- 'repo': 'test',`
		`- 'namespace': None})`
		`-`
		`- repo = pagure.lib.query.get_authorized_project(self.session, 'test')`
		`- plugin = pagure.lib.plugins.get_plugin('default')`
		`- dbobj = plugin.db_object()`
		`-`
		`- plugin.remove(repo)`
		`-`
		`- self.assertFalse(os.path.exists(os.path.join(`
		`- self.path, 'repos', 'test.git', 'hooks', 'post-receive.default')))`
		`- self.assertTrue(os.path.exists(os.path.join(`
		`- self.path, 'repos', 'test.git', 'hooks', 'post-receive')))`
		`-`
		`- def test_plugin_default_form(self):`
		`- """ Check that the default plugin's form.`
		`- """`
		`- with self._app.test_request_context('/') as ctx:`
		`- flask.g.session = self.session`
		`- flask.g.fas_user = tests.FakeUser(username='foo')`
		`-`
		`- task = pagure.lib.query.new_project(`
		`- self.session,`
		`- user='pingou',`
		`- name='test',`
		`- repospanner_region=None,`
		`- blacklist=[],`
		`- allowed_prefix=[],`
		`- description=None,`
		`- url=None, avatar_email=None,`
		`- parent_id=None,`
		`- add_readme=False,`
		`- userobj=None,`
		`- prevent_40_chars=False,`
		`- namespace=None`
		`- )`
		`- self.assertEqual(task.get(),`
		`- {'endpoint': 'ui_ns.view_repo',`
		`- 'repo': 'test',`
		`- 'namespace': None})`
		`-`
		`- repo = pagure.lib.query.get_authorized_project(self.session, 'test')`
		`- plugin = pagure.lib.plugins.get_plugin('default')`
		`- dbobj = plugin.db_object()`
		`- form = plugin.form(obj=dbobj)`
		`- self.assertEqual(`
		`- str(form.active),`
		`- '<input id="active" name="active" type="checkbox" value="y">'`
		`- )`


		`if __name__ == '__main__':`

tests/test_pagure_lib_plugins.py

file added

+76

		`@@ -0,0 +1,76 @@`
		`+ # coding=utf-8`
		`+ """`
		`+ (c) 2015-2018 - Copyright Red Hat Inc`
		`+`
		`+ Authors:`
		`+ Slavek Kabrda <bkabrda@redhat.com>`
		`+`
		`+ """`
		`+`
		`+ from __future__ import unicode_literals`
		`+`
		`+ __requires__ = ['SQLAlchemy >= 0.8']`
		`+ import pkg_resources`
		`+`
		`+ import os`
		`+ import sys`
		`+`
		`+ from mock import patch`
		`+`
		`+ sys.path.insert(0, os.path.join(os.path.dirname(`
		`+ os.path.abspath(__file__)), '..'))`
		`+`
		`+ import pagure.hooks`
		`+ import pagure.lib.plugins`
		`+ import tests`
		`+`
		`+`
		`+ class EnabledForAll(pagure.hooks.BaseHook):`
		`+ name = "EnabledForAll"`
		`+`
		`+ @classmethod`
		`+ def is_enabled_for(cls, project):`
		`+ return True`
		`+`
		`+ class DisabledForAll(pagure.hooks.BaseHook):`
		`+ name = "DisabledForAll"`
		`+ # disabled for all is the default`
		`+`
		`+ class PagureLibtests_plugins(tests.Modeltests):`
		`+ """`
		`+ Test the pagure.lib.plugins module`
		`+ """`
		`+`
		`+ maxDiff = None`
		`+`
		`+ @patch("pagure.lib.plugins.load")`
		`+ def test_plugin_is_enabled_for(self, load):`
		`+ """ Test the is_enabled_for method of plugins is properly`
		`+ handled by pagure.lib.plugins.get_enabled_plugins.`
		`+ """`
		`+ tests.create_projects(self.session)`
		`+ project = pagure.lib.query._get_project(self.session, "test")`
		`+`
		`+ load.return_value = [EnabledForAll]`
		`+ self.assertEqual(`
		`+ pagure.lib.plugins.get_enabled_plugins(project),`
		`+ [(EnabledForAll, None)]`
		`+ )`
		`+`
		`+ load.return_value = [DisabledForAll]`
		`+ self.assertEqual(`
		`+ pagure.lib.plugins.get_enabled_plugins(project),`
		`+ []`
		`+ )`
		`+`
		`+ @patch("pagure.lib.plugins.load")`
		`+ def test_get_plugin_names(self, load):`
		`+ """ Test the get_plugin_names method with plugins that don't`
		`+ have backref.`
		`+ """`
		`+ load.return_value = [EnabledForAll]`
		`+ self.assertEqual(pagure.lib.plugins.get_plugin_names(), [])`
		`+ self.assertEqual(`
		`+ pagure.lib.plugins.get_plugin_names(without_backref=True),`
		`+ ['EnabledForAll']`
		`+ )`

bkabrda commented 5 years ago

This makes it possible to add hooks that don't have an associated DB model and form and get executed based on a dynamic criteria evaluated in the is_enabled_for method. It's also not possible to turn these off through project settings.

Fixes #3972

pingou commented on line 7 of pagure/lib/plugins.py 5 years ago

This doesn't seem to be used anywhere with without_backref = True, is this only here for future needs?

pingou commented 5 years ago

Should we already port the default plugin to this structure?

bkabrda commented on line 7 of pagure/lib/plugins.py 5 years ago

Yes, that's for future use. I also consider this to be useful even if the argument is never used, because people working on stuff using this function will (just by seeing the argument) realize that this function will not return hooks without backref by default.

bkabrda commented 5 years ago