Restructuring hooks in dist-git

[Pierre-Yves Chibon]

Good Morning Everyone,

Looking into fixing the current git hook situation on dist-git I came accross
the following architecture and I would like to discuss it with you.

Each repo contains:
- post-receive pointing to /usr/share/git-core/post-receive-chained
- a folder post-receive-chained.d containing:
  - post-receive-alternativearch -> /usr/share/git-core/post-receive-alternativearch
  - post-receive-email -> /usr/share/git-core/mail-hooks/gnome-post-receive-email
  - post-receive-fedmsg -> /usr/share/git-core/post-receive-fedmsg

And /usr/share/git-core/post-receive-chained contains:

pee \
    $GIT_DIR/hooks/post-receive-chained.d/post-receive-fedmsg \
    $GIT_DIR/hooks/post-receive-chained.d/post-receive-alternativearch

So each git repo contains symlinks to the different git hooks, but there is no
need for them.

I would propose the following changes:

- Drop post-receive-chained.d entirely from each and every dist-git repo
- Replace /usr/share/git-core/post-receive-chained to be:

pee \
    /usr/share/git-core/post-receive-fedmsg \
    /usr/share/git-core/post-receive-alternativearch

Adjust the script in ansible in roles/git/checks/files/check-perms.py to
implement these changes.

Bonus, adding/removing hooks would then just become a matter of updating
roles/git/hooks/files/post-receive-chained in ansible.
So we could just add the pagure hooks:
    /usr/lib/python2.7/site-packages/pagure/hooks/files/default_hook.py
    /usr/lib/python2.7/site-packages/pagure/hooks/files/pagure_hook.py
to it to solve both the lack of logging of commits activity and to add support
for Merges/Fixes/Relates keywords in commit messages.


Finally it looks like the check-perms.py script isn't being run, that may be my
fault (I might have remove the script/cron when cleaning up after deploying
pagure).

What do you think? I know we are in freeze but this shouldn't impact directly
anything release-related. Worth trying now or should it wait some more?


Thanks,
Pierre

[PATCH 1/2] Add pagure's hook to the post-receive hook and point to the main hooks

[Pierre-Yves Chibon]

---
 roles/git/hooks/files/post-receive-chained | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/roles/git/hooks/files/post-receive-chained
b/roles/git/hooks/files/post-receive-chained
index 4718f57..e61cb72 100755
--- a/roles/git/hooks/files/post-receive-chained
+++ b/roles/git/hooks/files/post-receive-chained
@@ -4,8 +4,10 @@
 # You need to explicitly add your hook to the following list
 # for it to be invoked.
 pee \
-    $GIT_DIR/hooks/post-receive-chained.d/post-receive-fedmsg \
-    $GIT_DIR/hooks/post-receive-chained.d/post-receive-alternativearch
+    /usr/share/git-core/post-receive-fedmsg \
+    /usr/share/git-core/post-receive-alternativearch \
+    /usr/lib/python2.7/site-packages/pagure/hooks/files/default_hook.py \
+    /usr/lib/python2.7/site-packages/pagure/hooks/files/pagure_hook.py
 
 # We used to send emails directly from the git hook here, but now we send to
 # fedmsg which routes to FMN which routes to packagers and the mailing list.
-- 
2.9.5

[PATCH 2/2] Fix the check-perms.py script/cron

[Pierre-Yves Chibon]

---
 roles/git/checks/files/check-perms.py | 41 ++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/roles/git/checks/files/check-perms.py
b/roles/git/checks/files/check-perms.py
index 7e6501f..8fd8516 100755
--- a/roles/git/checks/files/check-perms.py
+++ b/roles/git/checks/files/check-perms.py
@@ -226,19 +226,25 @@ def set_post_receive_hook_version3(gitdir, fix=False):
         error('%s:  %s is not a directory.' % (gitdir, dest_prefix))
         return False
 
+    # Remove the old hooks
+    hooks = [
+        os.path.join(dest_prefix, 'post-receive-email'),
+        os.path.join(dest_prefix, 'post-receive-fedmsg'),
+        os.path.join(dest_prefix, 'post-receive-alternativearch'),
+    ]
+
+    for hook in hooks:
+        if os.path.exists(hook):
+            if not fix:
+                error('%s should be removed' % hook)
+            else:
+                os.unlink(hook)
+
     # Symlink mail notification and fedmsg scripts to post-receive hook
     scripts = {
-        '/usr/share/git-core/mail-hooks/gnome-post-receive-email':
-            os.path.join(dest_prefix, 'post-receive-email'),
-        '/usr/share/git-core/post-receive-fedmsg':
-            os.path.join(dest_prefix, 'post-receive-fedmsg'),
-        '/usr/share/git-core/post-receive-alternativearch':
-            os.path.join(dest_prefix, 'post-receive-alternativearch'),
-
         # This one kicks off all the others.
         '/usr/share/git-core/post-receive-chained':
             os.path.join(gitdir, 'hooks', 'post-receive'),
-
     }
 
     for script, hook in scripts.items():
@@ -250,19 +256,20 @@ def set_post_receive_hook_version3(gitdir, fix=False):
                 error('%s: Hook (%s) not installed.' % (gitdir, hook))
                 return False
 
-        if os.path.exists(hook):
+        if not os.path.islink(hook):
+            if os.path.exists(hook):
+                try:
+                    os.remove(hook)
+                except Exception, e:
+                    errstr = hasattr(e, 'strerror') and e.strerror or e
+                    error('%s: Error removing %s: %s' % (gitdir, hook, errstr))
+                    return False
             try:
-                os.remove(hook)
+                os.symlink(script, hook)
             except Exception, e:
                 errstr = hasattr(e, 'strerror') and e.strerror or e
-                error('%s: Error removing %s: %s' % (gitdir, hook, errstr))
+                error('%s: Error creating %s symlink: %s' % (gitdir, hook,
errstr))
                 return False
-        try:
-            os.symlink(script, hook)
-        except Exception, e:
-            errstr = hasattr(e, 'strerror') and e.strerror or e
-            error('%s: Error creating %s symlink: %s' % (gitdir, hook, errstr))
-            return False
 
     # We ran the gauntlet.
     return True
-- 
2.9.5

[Kevin Fenzi]

On 09/07/2017 07:12 AM, Pierre-Yves Chibon wrote:
>  Good Morning Everyone,
>  ...snip... 
>  What do you think? I know we are in freeze but this shouldn't
> impact directly
>  anything release-related. Worth trying now or should it wait some more? 
I like it and it should hopefully make it easy for me to add grokmirror
hooks. ;)

Can you do it in stg and test and confirm all works ok?

Also, should we renable check perms cron? I don't see that in this
patchset.

I'm somewhat leary of doing this in freeze, but if it looks ok in stg I
suppose it would be ok.

kevin

[Pierre-Yves Chibon]

On Thu, Sep 07, 2017 at 11:59:22AM -0700, Kevin Fenzi wrote:
>  On 09/07/2017 07:12 AM, Pierre-Yves Chibon wrote:
>  > Good Morning Everyone,
>  > ...snip...
>  
>  > What do you think? I know we are in freeze but this shouldn't impact directly
>  > anything release-related. Worth trying now or should it wait some more?
>  
>  I like it and it should hopefully make it easy for me to add grokmirror
>  hooks. ;)
>  
>  Can you do it in stg and test and confirm all works ok? 
I just manually adjusted things in stg, so let's see if it works :)

>  Also, should we renable check perms cron? I don't see that in
> this
>  patchset. 
I manually adjusted it in stg but this is the change to put in ansible:

diff --git a/ roles/distgit/tasks/main.yml b/ roles/distgit/tasks/main.yml
index 42e7097..f6045a9 100644
--- a/ roles/distgit/tasks/main.yml
+++ b/ roles/distgit/tasks/main.yml
@@ -142,7 +142,7 @@
     name="check-update-hooks" cron_file="ansible-check-update-hooks"
     minute=0 hour=0 weekday=3
     user=nobody
-    job="MAILTO=root PATH=/usr/bin:/usr/local/bin git check-perms
--check=update-hook /srv/git/repositories}}"
+    job="MAILTO=root PATH=/usr/bin:/usr/local/bin/git-check-perms
--check=update-hook /srv/git/repositories"
   tags:
   - distgit

diff --git a/ roles/gitolite/check_fedmsg_hooks/tasks/main.yml b/
roles/gitolite/check_fedmsg_hooks/tasks/main.yml
index b0047c9..afa0c61 100644
--- a/ roles/gitolite/check_fedmsg_hooks/tasks/main.yml
+++ b/ roles/gitolite/check_fedmsg_hooks/tasks/main.yml
@@ -9,4 +9,4 @@
     hour=0
     weekday=3
     user=nobody
-    job="MAILTO=root PATH=/usr/bin:/usr/local/bin git check-perms /git
--check=fedmsg-hook"
+    job="MAILTO=root PATH=/usr/bin:/usr/local/bin/git-check-perms
/srv/git/repositories --check=fedmsg-hook"


>  I'm somewhat leary of doing this in freeze, but if it looks ok in
> stg I
>  suppose it would be ok. 
My biggest concern is the lack of fedmsg notifications on new repos.
After I guess it all depends on how long freeze is going to be :s


Pierre

[Pierre-Yves Chibon]

Good Morning Everyone,

After some more testing and debugging in stg, these are the changes I ended up
doing/running in stg.

 [PATCH 1/3] Add pagure's hook to the post-receive hook and point to
 [PATCH 2/3] Fix the check-perms.py script/cron
 [PATCH 3/3] Fix the git-check-perms cron jobs

Worth pushing forward?


Thanks,
Pierre

[PATCH 1/3] Add pagure's hook to the post-receive hook and point to the main hooks

[Pierre-Yves Chibon]

---
 roles/git/hooks/files/post-receive-chained | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/roles/git/hooks/files/post-receive-chained
b/roles/git/hooks/files/post-receive-chained
index 4718f57..e61cb72 100755
--- a/roles/git/hooks/files/post-receive-chained
+++ b/roles/git/hooks/files/post-receive-chained
@@ -4,8 +4,10 @@
 # You need to explicitly add your hook to the following list
 # for it to be invoked.
 pee \
-    $GIT_DIR/hooks/post-receive-chained.d/post-receive-fedmsg \
-    $GIT_DIR/hooks/post-receive-chained.d/post-receive-alternativearch
+    /usr/share/git-core/post-receive-fedmsg \
+    /usr/share/git-core/post-receive-alternativearch \
+    /usr/lib/python2.7/site-packages/pagure/hooks/files/default_hook.py \
+    /usr/lib/python2.7/site-packages/pagure/hooks/files/pagure_hook.py
 
 # We used to send emails directly from the git hook here, but now we send to
 # fedmsg which routes to FMN which routes to packagers and the mailing list.
-- 
2.9.5

[PATCH 2/3] Fix the check-perms.py script/cron

[Pierre-Yves Chibon]

---
 roles/git/checks/files/check-perms.py | 60 +++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/roles/git/checks/files/check-perms.py
b/roles/git/checks/files/check-perms.py
index 7e6501f..469f6c5 100755
--- a/roles/git/checks/files/check-perms.py
+++ b/roles/git/checks/files/check-perms.py
@@ -216,29 +216,34 @@ def set_post_receive_hook_version2(gitdir):
 def set_post_receive_hook_version3(gitdir, fix=False):
     """Configure a git repository to use the fedmsg+gnome-mail
hooks."""
 
-    # Check that the destination is 'okay'
+    # Old folder where we used to place the hooks
     dest_prefix = os.path.join(gitdir, 'hooks',
'post-receive-chained.d')
 
-    if not os.path.exists(dest_prefix):
-        os.mkdir(dest_prefix)
+    # Remove the old hooks
+    hooks = [
+        os.path.join(dest_prefix, 'post-receive-email'),
+        os.path.join(dest_prefix, 'post-receive-fedmsg'),
+        os.path.join(dest_prefix, 'post-receive-alternativearch'),
+    ]
 
-    if not os.path.isdir(dest_prefix):
-        error('%s:  %s is not a directory.' % (gitdir, dest_prefix))
-        return False
+    for hook in hooks:
+        if os.path.exists(hook):
+            if not fix:
+                error('%s should be removed' % hook)
+            else:
+                os.unlink(hook)
+
+    if os.path.exists(dest_prefix):
+        if not fix:
+            error('%s should be removed' % dest_prefix)
+        else:
+            os.rmdir(dest_prefix)
 
     # Symlink mail notification and fedmsg scripts to post-receive hook
     scripts = {
-        '/usr/share/git-core/mail-hooks/gnome-post-receive-email':
-            os.path.join(dest_prefix, 'post-receive-email'),
-        '/usr/share/git-core/post-receive-fedmsg':
-            os.path.join(dest_prefix, 'post-receive-fedmsg'),
-        '/usr/share/git-core/post-receive-alternativearch':
-            os.path.join(dest_prefix, 'post-receive-alternativearch'),
-
         # This one kicks off all the others.
         '/usr/share/git-core/post-receive-chained':
             os.path.join(gitdir, 'hooks', 'post-receive'),
-
     }
 
     for script, hook in scripts.items():
@@ -250,19 +255,28 @@ def set_post_receive_hook_version3(gitdir, fix=False):
                 error('%s: Hook (%s) not installed.' % (gitdir, hook))
                 return False
 
-        if os.path.exists(hook):
+        if not os.path.islink(hook) \
+                or (os.path.islink(hook) and os.path.realpath(hook) != script):
+            if os.path.exists(hook) \
+                    or (os.path.islink(hook) and os.path.realpath(hook) != script):
+                try:
+                    if not fix:
+                        error('%s should be removed' % hook)
+                    else:
+                        os.remove(hook)
+                except Exception, e:
+                    errstr = hasattr(e, 'strerror') and e.strerror or e
+                    error('%s: Error removing %s: %s' % (gitdir, hook, errstr))
+                    return False
             try:
-                os.remove(hook)
+                if not fix:
+                    error('link from %s to %s should be created' % (script,
hook))
+                else:
+                    os.symlink(script, hook)
             except Exception, e:
                 errstr = hasattr(e, 'strerror') and e.strerror or e
-                error('%s: Error removing %s: %s' % (gitdir, hook, errstr))
+                error('%s: Error creating %s symlink: %s' % (gitdir, hook,
errstr))
                 return False
-        try:
-            os.symlink(script, hook)
-        except Exception, e:
-            errstr = hasattr(e, 'strerror') and e.strerror or e
-            error('%s: Error creating %s symlink: %s' % (gitdir, hook, errstr))
-            return False
 
     # We ran the gauntlet.
     return True
-- 
2.9.5

[PATCH 3/3] Fix the git-check-perms cron jobs

[Pierre-Yves Chibon]

---
 roles/distgit/tasks/main.yml                     | 2 +-
 roles/gitolite/check_fedmsg_hooks/tasks/main.yml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/roles/distgit/tasks/main.yml b/roles/distgit/tasks/main.yml
index 42e7097..f6045a9 100644
--- a/roles/distgit/tasks/main.yml
+++ b/roles/distgit/tasks/main.yml
@@ -142,7 +142,7 @@
     name="check-update-hooks" cron_file="ansible-check-update-hooks"
     minute=0 hour=0 weekday=3
     user=nobody
-    job="MAILTO=root PATH=/usr/bin:/usr/local/bin git check-perms
--check=update-hook /srv/git/repositories}}"
+    job="MAILTO=root PATH=/usr/bin:/usr/local/bin/git-check-perms
--check=update-hook /srv/git/repositories"
   tags:
   - distgit
 
diff --git a/roles/gitolite/check_fedmsg_hooks/tasks/main.yml
b/roles/gitolite/check_fedmsg_hooks/tasks/main.yml
index b0047c9..afa0c61 100644
--- a/roles/gitolite/check_fedmsg_hooks/tasks/main.yml
+++ b/roles/gitolite/check_fedmsg_hooks/tasks/main.yml
@@ -9,4 +9,4 @@
     hour=0
     weekday=3
     user=nobody
-    job="MAILTO=root PATH=/usr/bin:/usr/local/bin git check-perms /git
--check=fedmsg-hook"
+    job="MAILTO=root PATH=/usr/bin:/usr/local/bin/git-check-perms
/srv/git/repositories --check=fedmsg-hook"
-- 
2.9.5

[Kevin Fenzi]

On 09/14/2017 02:24 AM, Pierre-Yves Chibon wrote:
>  Good Morning Everyone,
>  
>  After some more testing and debugging in stg, these are the changes I ended up
>  doing/running in stg.
>  
>   [PATCH 1/3] Add pagure's hook to the post-receive hook and point to
>   [PATCH 2/3] Fix the check-perms.py script/cron
>   [PATCH 3/3] Fix the git-check-perms cron jobs
>  
>  Worth pushing forward? 
I think so... +1

kevin

[Michal Novotny]

Hello,

On Thu, Sep 14, 2017 at 11:24 AM, Pierre-Yves Chibon <pingou(a)pingoured.fr&gt;
wrote:

>  Good Morning Everyone,
>
>  After some more testing and debugging in stg, these are the changes I
>  ended up
>  doing/running in stg.
>
>   [PATCH 1/3] Add pagure's hook to the post-receive hook and point to
>   [PATCH 2/3] Fix the check-perms.py script/cron
>   [PATCH 3/3] Fix the git-check-perms cron jobs
>  
Sorry, where can I find those patches? Anyway some of it, could be probably
incorporated into
https://github.com/release-engineering/dist-git/blob/master/scripts/dist-...
.

E.g. grok mirror hook support that Kevin mentioned is already there.

I can adjust the setup script after the changes are in Ansible if needed,
however.


>  Worth pushing forward?
>
>
>  Thanks,
>  Pierre
>  _______________________________________________
>  infrastructure mailing list -- infrastructure(a)lists.fedoraproject.org
>  To unsubscribe send an email to infrastructure-leave@lists.
>  fedoraproject.org
>

[Michal Novotny]

Looking further, this is probably unrelated now as the setup script from
the upstream dist-git is no longer used. Sorry for the fuss.

On Fri, Sep 15, 2017 at 8:13 AM, Michal Novotny <clime(a)redhat.com&gt; wrote:

>  Hello,
>
>  On Thu, Sep 14, 2017 at 11:24 AM, Pierre-Yves Chibon <pingou(a)pingoured.fr&gt;
>  wrote:
>
> > Good Morning Everyone,
> >
> > After some more testing and debugging in stg, these are the changes I
> > ended up
> > doing/running in stg.
> >
> >  [PATCH 1/3] Add pagure's hook to the post-receive hook and point to
> >  [PATCH 2/3] Fix the check-perms.py script/cron
> >  [PATCH 3/3] Fix the git-check-perms cron jobs
> >
>
>  Sorry, where can I find those patches? Anyway some of it, could be probably
>  incorporated into https://github.com/release-engineering/dist-git/blob/
>  master/scripts/dist-git/setup_git_package.
>
>  E.g. grok mirror hook support that Kevin mentioned is already there.
>
>  I can adjust the setup script after the changes are in Ansible if needed,
>  however.
>
>
> >
> > Worth pushing forward?
> >
> >
> > Thanks,
> > Pierre
> > _______________________________________________
> > infrastructure mailing list -- infrastructure(a)lists.fedoraproject.org
> > To unsubscribe send an email to infrastructure-leave(a)lists.fed
> > oraproject.org
> >
>
>

[Pierre-Yves Chibon]

On Fri, Sep 15, 2017 at 08:13:06AM +0200, Michal Novotny wrote:
>     Hello,
>     On Thu, Sep 14, 2017 at 11:24 AM, Pierre-Yves Chibon <pingou(a)pingoured.fr&gt;
>     wrote:
>  
>       Good Morning Everyone,
>  
>       After some more testing and debugging in stg, these are the changes I
>       ended up
>       doing/running in stg.
>  
>       Â [PATCH 1/3] Add pagure's hook to the post-receive hook and point to
>       Â [PATCH 2/3] Fix the check-perms.py script/cron
>       Â [PATCH 3/3] Fix the git-check-perms cron jobs
>  
>     Sorry, where can I find those patches?  
They are present there:
https://lists.fedoraproject.org/archives/list/[e-mail]...
https://lists.fedoraproject.org/archives/list/[e-mail]...
https://lists.fedoraproject.org/archives/list/[e-mail]...

Pierre