pagure

Clone

#917 Clean up installing/removing git hooks
Merged 7 years ago by pingou. Opened 7 years ago by cverna.
cverna/pagure pagure_901  into  master

file modified
+34 -8 
@@ -12,6 +12,7 @@ 
 

  import shutil
 

  import wtforms
 

  

+ from pagure.exceptions import FileNotFoundException
 

  from pagure import APP, get_repo_path
 

  

  
@@ -69,13 +70,11 @@ 
 

              # Install the main post-receive file
 

              postreceive = os.path.join(hookfolder, cls.hook_type)
 

              if not os.path.exists(postreceive):
 

-                 shutil.copyfile(
 

-                     os.path.join(hook_files, cls.hook_type),
 

-                     postreceive)
 

-                 os.chmod(postreceive, 0755)
 

+                 os.symlink(os.path.join(hook_files, cls.hook_type),
 

+                            postreceive)
 

  

      @classmethod
 

-     def install(cls, project, dbobj):  # pragma: no cover
 

+     def base_install(cls, repopaths, dbobj, hook_name, filein):
 

          ''' Method called to install the hook for a project.
 

  

          :arg project: a ``pagure.model.Project`` object to which the hook
 
@@ -84,14 +83,41 @@ 
 

              information.
 

  

          '''
 

-         pass
 

+         for repopath in repopaths:
 

+             if not os.path.exists(repopath):
 

+                 raise FileNotFoundException('Repo %s not found' % repopath)
 

+ 

+             hook_files = os.path.join(
 

+                 os.path.dirname(os.path.realpath(__file__)), 'files')
 

+ 

+             # Make sure the hooks folder exists
 

+             hookfolder = os.path.join(repopath, 'hooks')
 

+             if not os.path.exists(hookfolder):
 

+                 os.makedirs(hookfolder)
 

+ 

+             # Install the hook itself
 

+             hook_file = os.path.join(repopath, 'hooks', cls.hook_type + '.'
 

+                                      + hook_name)
 

+ 

+             if not os.path.exists(hook_file):
 

+                 os.symlink(
 

+                     os.path.join(hook_files, filein),
 

+                     hook_file
 

+                 )
 

  

      @classmethod
 

-     def remove(cls, project):  # pragma: no cover
 

+     def base_remove(cls, repopaths, hook_name):
 

          ''' Method called to remove the hook of a project.
 

  

          :arg project: a ``pagure.model.Project`` object to which the hook
 

              should be installed
 

  

          '''
 

-         pass
 

+         for repopath in repopaths:
 

+             if not os.path.exists(repopath):
 

+                 raise FileNotFoundException('Repo %s not found' % repopath)
 

+ 

+             hook_path = os.path.join(repopath, 'hooks', cls.hook_type + '.'
 

+                                      + hook_name)
 

+             if os.path.exists(hook_path):
 

+                 os.unlink(hook_path)

file modified
+4 -24 
@@ -74,25 +74,8 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

-         if not os.path.exists(repopath):
 

-             flask.abort(404, 'No git repo found')
 

- 

-         hook_files = os.path.join(
 

-             os.path.dirname(os.path.realpath(__file__)), 'files')
 

- 

-         # Make sure the hooks folder exists
 

-         hookfolder = os.path.join(repopath, 'hooks')
 

-         if not os.path.exists(hookfolder):
 

-             os.makedirs(hookfolder)
 

- 

-         # Install the hook itself
 

-         hook_file = os.path.join(repopath, 'hooks', 'post-receive.fedmsg')
 

-         if not os.path.exists(hook_file):
 

-             os.symlink(
 

-                 os.path.join(hook_files, 'fedmsg_hook.py'),
 

-                 hook_file
 

-             )
 

+         repopaths = [get_repo_path(project)]
 

+         cls.base_install(repopaths, dbobj, 'fedmsg', 'fedmsg_hook.py')
 

  

      @classmethod
 

      def remove(cls, project):
 
@@ -102,8 +85,5 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

- 

-         hook_path = os.path.join(repopath, 'hooks', 'post-receive.fedmsg')
 

-         if os.path.exists(hook_path):
 

-             os.unlink(hook_path)
 

+         repopaths = [get_repo_path(project)]
 

+         cls.base_remove(repopaths, 'fedmsg')

file modified

file modified

file modified
+5 -14 
@@ -114,22 +114,15 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

+         repopaths = [get_repo_path(project)]
 

  

-         hook_files = os.path.join(
 

-             os.path.dirname(os.path.realpath(__file__)), 'files')
 

-         repo_obj = pygit2.Repository(repopath)
 

+         repo_obj = pygit2.Repository(repopaths[0])
 

  

          # Configure the hook
 

          # repo_obj.config.set_multivar()
 

  

          # Install the hook itself
 

-         #hook_file = os.path.join(hook_files, 'git_irc.py')
 

-         #if not os.path.exists(hook_file):
 

-             #os.symlink(
 

-                 #hook_file,
 

-                 #os.path.join(repopath, 'hooks', 'post-receive.irc')
 

-             #)
 

+         # cls.base_install(repopaths, dbobj, 'irc', 'git_irc.py')
 

  

      @classmethod
 

      def remove(cls, project):
 
@@ -139,8 +132,6 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

+         repopaths = [get_repo_path(project)]
 

  

-         #hook_path = os.path.join(repopath, 'hooks', 'post-receive.irc')
 

-         #if os.path.exists(hook_path):
 

-             #os.unlink(hook_path)
 

+         # cls.base_remove(repopaths, 'irc')

file modified
+5 -16 
@@ -80,11 +80,8 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

- 

-         hook_files = os.path.join(
 

-             os.path.dirname(os.path.realpath(__file__)), 'files')
 

-         repo_obj = pygit2.Repository(repopath)
 

+         repopaths = [get_repo_path(project)]
 

+         repo_obj = pygit2.Repository(repopaths[0])
 

  

          # Configure the hook
 

          repo_obj.config.set_multivar(
 
@@ -96,12 +93,7 @@ 
 

              'multimailhook.environment', '', 'gitolite')
 

  

          # Install the hook itself
 

-         hook_file = os.path.join(repopath, 'hooks', 'post-receive.mail')
 

-         if not os.path.exists(hook_file):
 

-             os.symlink(
 

-                 os.path.join(hook_files, 'git_multimail.py'),
 

-                 hook_file
 

-             )
 

+         cls.base_install(repopaths, dbobj, 'mail', 'git_multimail.py')
 

  

      @classmethod
 

      def remove(cls, project):
 
@@ -111,8 +103,5 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

- 

-         hook_path = os.path.join(repopath, 'hooks', 'post-receive.mail')
 

-         if os.path.exists(hook_path):
 

-             os.unlink(hook_path)
 

+         repopaths = [get_repo_path(project)]
 

+         cls.base_remove(repopaths, 'mail')

file modified
+6 -17 
@@ -83,20 +83,12 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

- 

-         hook_files = os.path.join(
 

-             os.path.dirname(os.path.realpath(__file__)), 'files')
 

-         hook_file = os.path.join(hook_files, 'pagure_force_commit_hook.py')
 

- 

          # Init the git repo in case
 

-         pygit2.Repository(repopath)
 

+         repopaths = [get_repo_path(project)]
 

+         pygit2.Repository(repopaths[0])
 

  

-         # Install the hook itself
 

-         hook_path = os.path.join(
 

-             repopath, 'hooks', 'pre-receive.pagureforcecommit')
 

-         if not os.path.exists(hook_path):
 

-             os.symlink(hook_file, hook_path)
 

+         cls.base_install(repopaths, dbobj, 'pagureforcecommit',
 

+                          'pagure_force_commit_hook.py')
 

  

      @classmethod
 

      def remove(cls, project):
 
@@ -106,8 +98,5 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

-         hook_path = os.path.join(
 

-             repopath, 'hooks', 'pre-receive.pagureforcecommit')
 

-         if os.path.exists(hook_path):
 

-             os.unlink(hook_path)
 

+         repopaths = [get_repo_path(project)]
 

+         cls.base_remove(repopaths, 'pagureforcecommit')

file modified
+2 -19 
@@ -103,20 +103,7 @@ 
 

                  os.path.join(folder, project.path)
 

              )
 

  

-         hook_files = os.path.join(
 

-             os.path.dirname(os.path.realpath(__file__)), 'files')
 

-         hook_file = os.path.join(hook_files, 'pagure_hook.py')
 

- 

-         for repopath in repopaths:
 

-             # Init the git repo in case
 

-             pygit2.Repository(repopath)
 

- 

-             # Install the hook itself
 

-             hook_path = os.path.join(
 

-                 repopath, 'hooks', 'post-receive.pagure')
 

-             hook_file = os.path.join(hook_files, 'pagure_hook.py')
 

-             if not os.path.exists(hook_path):
 

-                 os.symlink(hook_file, hook_path)
 

+         cls.base_install(repopaths, dbobj, 'pagure', 'pagure_hook.py')
 

  

      @classmethod
 

      def remove(cls, project):
 
@@ -134,8 +121,4 @@ 
 

                  os.path.join(folder, project.path)
 

              )
 

  

-         for repopath in repopaths:
 

-             hook_path = os.path.join(
 

-                 repopath, 'hooks', 'post-receive.pagure')
 

-             if os.path.exists(hook_path):
 

-                 os.unlink(hook_path)
 

+         cls.base_remove(repopaths, 'pagure')

file modified
+5 -24 
@@ -100,24 +100,10 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = os.path.join(APP.config['REQUESTS_FOLDER'], project.path)
 

-         if not os.path.exists(repopath):  # pragma: no cover
 

-             # We cannot test this as un-existing repo should be catched
 

-             # at the set_up() stage
 

-             flask.abort(404, 'No git repo found')
 

- 

-         hook_files = os.path.join(
 

-             os.path.dirname(os.path.realpath(__file__)), 'files')
 

-         pygit2.Repository(repopath)
 

+         repopaths = [os.path.join(APP.config['REQUESTS_FOLDER'], project.path)]
 

  

-         # Install the hook itself
 

-         hook_path = os.path.join(
 

-             repopath, 'hooks', 'post-receive.pagure-requests')
 

-         if not os.path.exists(hook_path):
 

-             os.symlink(
 

-                 os.path.join(hook_files, 'pagure_hook_requests.py'),
 

-                 hook_path
 

-             )
 

+         cls.base_install(repopaths, dbobj, 'pagure-requests',
 

+                          'pagure_hook_requests.py')
 

  

      @classmethod
 

      def remove(cls, project):
 
@@ -127,11 +113,6 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = os.path.join(APP.config['REQUESTS_FOLDER'], project.path)
 

-         if not os.path.exists(repopath):
 

-             flask.abort(404, 'No git repo found')
 

+         repopaths = [os.path.join(APP.config['REQUESTS_FOLDER'], project.path)]
 

  

-         hook_path = os.path.join(
 

-             repopath, 'hooks', 'post-receive.pagure-requests')
 

-         if os.path.exists(hook_path):
 

-             os.unlink(hook_path)
 

+         cls.base_remove(repopaths, 'pagure-requests')

file modified
+5 -24 
@@ -99,24 +99,10 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = os.path.join(APP.config['TICKETS_FOLDER'], project.path)
 

-         if not os.path.exists(repopath):  # pragma: no cover
 

-             # We cannot test this as un-existing repo should be catched
 

-             # at the set_up() stage
 

-             flask.abort(404, 'No git repo found')
 

- 

-         hook_files = os.path.join(
 

-             os.path.dirname(os.path.realpath(__file__)), 'files')
 

-         pygit2.Repository(repopath)
 

+         repopaths = [os.path.join(APP.config['TICKETS_FOLDER'], project.path)]
 

  

-         # Install the hook itself
 

-         hook_path = os.path.join(
 

-             repopath, 'hooks', 'post-receive.pagure-ticket')
 

-         if not os.path.exists(hook_path):
 

-             os.symlink(
 

-                 os.path.join(hook_files, 'pagure_hook_tickets.py'),
 

-                 hook_path
 

-             )
 

+         cls.base_install(repopaths, dbobj, 'pagure-ticket',
 

+                          'pagure_hook_tickets.py')
 

  

      @classmethod
 

      def remove(cls, project):
 
@@ -126,11 +112,6 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = os.path.join(APP.config['TICKETS_FOLDER'], project.path)
 

-         if not os.path.exists(repopath):
 

-             flask.abort(404, 'No git repo found')
 

+         repopaths = [os.path.join(APP.config['TICKETS_FOLDER'], project.path)]
 

  

-         hook_path = os.path.join(
 

-             repopath, 'hooks', 'post-receive.pagure-ticket')
 

-         if os.path.exists(hook_path):
 

-             os.unlink(hook_path)
 

+         cls.base_remove(repopaths, 'pagure-ticket')

file modified
+6 -18 
@@ -77,20 +77,10 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

+         repopaths = [get_repo_path(project)]
 

  

-         hook_files = os.path.join(
 

-             os.path.dirname(os.path.realpath(__file__)), 'files')
 

-         hook_file = os.path.join(hook_files, 'pagure_block_unsigned.py')
 

- 

-         # Init the git repo in case
 

-         pygit2.Repository(repopath)
 

- 

-         # Install the hook itself
 

-         hook_path = os.path.join(
 

-             repopath, 'hooks', 'pre-receive.pagureunsignedcommit')
 

-         if not os.path.exists(hook_path):
 

-             os.symlink(hook_file, hook_path)
 

+         cls.base_install(repopaths, dbobj, 'pagureunsignedcommit',
 

+                          'pagure_block_unsigned.py')
 

  

      @classmethod
 

      def remove(cls, project):
 
@@ -100,8 +90,6 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

-         hook_path = os.path.join(
 

-             repopath, 'hooks', 'pre-receive.pagureunsignedcommit')
 

-         if os.path.exists(hook_path):
 

-             os.unlink(hook_path)
 

+         repopaths = [get_repo_path(project)]
 

+ 

+         cls.base_remove(repopaths, 'pagureunsignedcommit')

file modified
+5 -19 
@@ -86,21 +86,9 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

+         repopaths = [get_repo_path(project)]
 

  

-         hook_files = os.path.join(
 

-             os.path.dirname(os.path.realpath(__file__)), 'files')
 

-         hook_file = os.path.join(hook_files, 'rtd_hook.py')
 

- 

-         # Init the git repo in case
 

-         pygit2.Repository(repopath)
 

- 

-         # Install the hook itself
 

-         hook_path = os.path.join(
 

-             repopath, 'hooks', 'post-receive.rtd')
 

-         hook_file = os.path.join(hook_files, 'rtd_hook.py')
 

-         if not os.path.exists(hook_path):
 

-             os.symlink(hook_file, hook_path)
 

+         cls.base_install(repopaths, dbobj, 'rtd', 'rtd_hook.py')
 

  

      @classmethod
 

      def remove(cls, project):
 
@@ -110,8 +98,6 @@ 
 

              should be installed
 

  

          '''
 

-         repopath = get_repo_path(project)
 

-         hook_path = os.path.join(
 

-             repopath, 'hooks', 'post-receive.rtd')
 

-         if os.path.exists(hook_path):
 

-             os.unlink(hook_path)
 

+         repopaths = [get_repo_path(project)]
 

+ 

+         cls.base_remove(repopaths, 'rtd')

file modified
+13 -5 
@@ -19,7 +19,7 @@ 
 

  import pagure.forms
 

  from pagure import APP, SESSION, login_required, is_repo_admin
 

  from pagure.lib.model import BASE
 

- 

+ from pagure.exceptions import FileNotFoundException
 

  # pylint: disable=E1101
 

  

  
@@ -127,11 +127,19 @@ 
 

              # Set up the main script if necessary
 

              plugin.set_up(repo)
 

              # Install the plugin itself
 

-             plugin.install(repo, dbobj)
 

-             flask.flash('Hook %s activated' % plugin.name)
 

+             try:
 

+                 plugin.install(repo, dbobj)
 

+                 flask.flash('Hook %s activated' % plugin.name)
 

+             except FileNotFoundException as err:
 

+                 pagure.APP.logger.exception(err)
 

+                 flask.abort(404, 'No git repo found')
 

          else:
 

-             plugin.remove(repo)
 

-             flask.flash('Hook %s inactived' % plugin.name)
 

+             try:
 

+                 plugin.remove(repo)
 

+                 flask.flash('Hook %s inactived' % plugin.name)
 

+             except FileNotFoundException as err:
 

+                 pagure.APP.logger.exception(err)
 

+                 flask.abort(404, 'No git repo found')
 

  

          SESSION.commit()

Fixes https://pagure.io/pagure/issue/901 + Create symlink for post/pre-receive script and add x permission in git.

I wonder if we should log which repo was not found, to help admins debug this

My first idea was to use self. variables, but this is cool as well :)

ok this won't fly, we need something more flexible than a relying on a list that we'll forget to update when adding a hook.

Relying on the hook_type here might be the solution

Same as below, we need to be more flexible than a list

Few comments but this looks neat :)

we could just pass the full hook_name in arg, ie pre-receive.pagureforcecommit instead of pagureforcecommit

What do you think ?

That should work.

I wonder if we couldn't rely a little more on the fact that we use inheritance, but otherwise it should work :)

rebased
7 years ago

rebased
7 years ago

Fixes #901

Better yet, use APP.logger then it's the same logger as for the rest of the app

the call to the logger must be before the abort, otherwise we won't run it

If the hook_type is no longer used, then we could drop it while at it :)

1 new commit added

  • Use APP.logger to log repo not found
7 years ago

1 new commit added

  • Remove unused import (logging)
7 years ago

hook_type is used by the BaseHook set_up method so we need it

idea (open for discussion) what if we raised an exception here?
This would allow us to keep this part independent of any web-framework.

What do you think?

rebased
7 years ago

Now using hook_type + exception for git repo not found

We should be able to drop this now :)

These two lines should still be true

If fear this will never fail :)

rebased
7 years ago

11 new commits added

  • Fixed exception handling and test
  • Added exception for repo not found
  • Renamed BaseHook install/remove method
  • Remove unused import (logging)
  • Use APP.logger to log repo not found
  • Pass full hookname to BaseHook to simply logic.
  • Use symlink instead of copy for hook setup.
  • Added 404 error on hook remove method
  • Fixed post-receive pre-receive hook name
  • Reworked Basehook install/remove methods and updated all hooks to call these methods
  • Move install/remove logic to BaseHook, works on fedmsg
7 years ago

rebased
7 years ago

So I guess this is no longer needed :)

We should remove the # pragma: no cover now since, we now run this method and therefore it can be taken into account when checking coverage

cf above :)

1 new commit added

  • Cleaning up code before merge
7 years ago

Thanks :)

Pull-Request has been merged by pingou
7 years ago
Metadata
None
No Tags
Changes Summary 13
+34 -8
file changed
pagure/hooks/__init__.py
+4 -24
file changed
pagure/hooks/fedmsg.py
+0 -0
file changed
pagure/hooks/files/post-receive
+0 -0
file changed
pagure/hooks/files/pre-receive
+5 -14
file changed
pagure/hooks/irc.py
+5 -16
file changed
pagure/hooks/mail.py
+6 -17
file changed
pagure/hooks/pagure_force_commit.py
+2 -19
file changed
pagure/hooks/pagure_hook.py
+5 -24
file changed
pagure/hooks/pagure_request_hook.py
+5 -24
file changed
pagure/hooks/pagure_ticket_hook.py
+6 -18
file changed
pagure/hooks/pagure_unsigned_commits.py
+5 -19
file changed
pagure/hooks/rtd.py
+13 -5
file changed
pagure/ui/plugins.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.