Freeze break request: pkgdb_sync should only remove master from todo if it was requested

[Patrick Uiterwijk]

Hi,

So another freeze break request, +1s appreciated.
This change is the only thing needed to make non-master-repos an option.
Currently, the code always removes master from the requested branches, and crashes if it
wasn't there.
I'm just adding a check to only remove it (and announce the package) if it was
requested.


commit d230b5701575879e757f2c4aeb8eed62c319d9e6
Author: Patrick Uiterwijk <puiterwijk(a)redhat.com&gt;
Date:   Wed Feb 25 11:34:07 2015 +0000

    Only remove master from request if it was requested
    
    Signed-off-by: Patrick Uiterwijk <puiterwijk(a)redhat.com&gt;

diff --git a/roles/distgit/templates/pkgdb_sync_git_branches.py
b/roles/distgit/templates/pkgdb_sync_git_branches.py
index c931d26..ced139b 100644
--- a/roles/distgit/templates/pkgdb_sync_git_branches.py
+++ b/roles/distgit/templates/pkgdb_sync_git_branches.py
@@ -189,16 +189,17 @@ def branch_package(pkgname, branches):
     if not os.path.exists(
             os.path.join(GIT_FOLDER, '%s.git' % pkgname)):
         _invoke(SETUP_PACKAGE, [pkgname])
-        branches.remove('master')  # SETUP_PACKAGE creates master
-        fedmsg.publish(
-            topic='branch',
-            modname='git',
-            msg=dict(
-                agent='pkgdb',
-                name=pkgname,
-                branch='master',
-            ),
-        )
+        if 'master' in branches:
+            branches.remove('master')  # SETUP_PACKAGE creates master
+            fedmsg.publish(
+                topic='branch',
+                modname='git',
+                msg=dict(
+                    agent='pkgdb',
+                    name=pkgname,
+                    branch='master',
+                ),
+             )
 
     # Create all the required branches for the package
     # Use the translated branch name until pkgdb falls inline



With kind regards,
Patrick Uiterwijk
Fedora Infra

[Pierre-Yves Chibon]

On Wed, Feb 25, 2015 at 06:36:27AM -0500, Patrick Uiterwijk wrote:
>  Hi,
>  
>  So another freeze break request, +1s appreciated.
>  This change is the only thing needed to make non-master-repos an option.
>  Currently, the code always removes master from the requested branches, and crashes if it
> wasn't there.
>  I'm just adding a check to only remove it (and announce the package) if it was
> requested.
>  
>  
>  commit d230b5701575879e757f2c4aeb8eed62c319d9e6
>  Author: Patrick Uiterwijk <puiterwijk(a)redhat.com&gt;
>  Date:   Wed Feb 25 11:34:07 2015 +0000
>  
>      Only remove master from request if it was requested
>      
>      Signed-off-by: Patrick Uiterwijk <puiterwijk(a)redhat.com&gt;
>  
>  diff --git a/roles/distgit/templates/pkgdb_sync_git_branches.py
> b/roles/distgit/templates/pkgdb_sync_git_branches.py
>  index c931d26..ced139b 100644
>  --- a/roles/distgit/templates/pkgdb_sync_git_branches.py
>  +++ b/roles/distgit/templates/pkgdb_sync_git_branches.py
>  @@ -189,16 +189,17 @@ def branch_package(pkgname, branches):
>       if not os.path.exists(
>               os.path.join(GIT_FOLDER, '%s.git' % pkgname)):
>           _invoke(SETUP_PACKAGE, [pkgname])
>  -        branches.remove('master')  # SETUP_PACKAGE creates master
>  -        fedmsg.publish(
>  -            topic='branch',
>  -            modname='git',
>  -            msg=dict(
>  -                agent='pkgdb',
>  -                name=pkgname,
>  -                branch='master',
>  -            ),
>  -        )
>  +        if 'master' in branches:
>  +            branches.remove('master')  # SETUP_PACKAGE creates master
>  +            fedmsg.publish(
>  +                topic='branch',
>  +                modname='git',
>  +                msg=dict(
>  +                    agent='pkgdb',
>  +                    name=pkgname,
>  +                    branch='master',
>  +                ),
>  +             ) 
Could you expand a little about this fedmsg message?
We don't create the branch (created by SETUP_PACKAGE) but we still send a
message that we did?


Pierre

[Patrick Uiterwijk]

----- Original Message -----
>  On Wed, Feb 25, 2015 at 06:36:27AM -0500, Patrick Uiterwijk wrote:
>  Could you expand a little about this fedmsg message?
>  We don't create the branch (created by SETUP_PACKAGE) but we still send a
>  message that we did?
>   
That's because the SETUP_PACKAGE script doesn't fire off fedmsg messages.
As far as I know, there's no fedmsg message for new-repo.

>  
>  Pierre
>  _______________________________________________

[Pierre-Yves Chibon]

On Wed, Feb 25, 2015 at 08:50:25AM -0500, Patrick Uiterwijk wrote:
>  
>  ----- Original Message -----
>  > On Wed, Feb 25, 2015 at 06:36:27AM -0500, Patrick Uiterwijk wrote:
>  
>  > Could you expand a little about this fedmsg message?
>  > We don't create the branch (created by SETUP_PACKAGE) but we still send a
>  > message that we did?
>  > 
>  
>  That's because the SETUP_PACKAGE script doesn't fire off fedmsg messages.
>  As far as I know, there's no fedmsg message for new-repo. 
But we could have it fire that same message about the master branch being added,
no?
(I'm asking if we could move that same piece of code to the place where the
master branch is being created)

Pierre

[Patrick Uiterwijk]

----- Original Message -----
>  On Wed, Feb 25, 2015 at 08:50:25AM -0500, Patrick Uiterwijk wrote:
>  But we could have it fire that same message about the master branch
> being
>  added,
>  no?
>  (I'm asking if we could move that same piece of code to the place where the
>  master branch is being created) 
If we would do that, we would need to also add fedmsg initialization to the
setup_git_package script.
While this probably can be done, I think it's most clear to have the python script
(pkgdb_sync) initialize it just once and sent the messages there.
Otherwise we'd likely end up with a bunch of json etc in the scripts.

>  
>  Pierre 
Patrick

[Pierre-Yves Chibon]

On Wed, Feb 25, 2015 at 08:56:40AM -0500, Patrick Uiterwijk wrote:
>  ----- Original Message -----
>  > On Wed, Feb 25, 2015 at 08:50:25AM -0500, Patrick Uiterwijk wrote:
>  
>  > But we could have it fire that same message about the master branch being
>  > added,
>  > no?
>  > (I'm asking if we could move that same piece of code to the place where the
>  > master branch is being created)
>  
>  If we would do that, we would need to also add fedmsg initialization to the
> setup_git_package script.
>  While this probably can be done, I think it's most clear to have the python script
> (pkgdb_sync) initialize it just once and sent the messages there.
>  Otherwise we'd likely end up with a bunch of json etc in the scripts. 
It's shell script, so we could just relay on calling fedmsg-logger, the arguments
are just the agent, the name of the package and the name of the branch (ie
master), 2 of the 3 are hard-coded and we know the third one (the package name).

Might not be that much json at the end :)

Maybe something along the lines of:

fedmsg-logger --message='{"agent": "pkgdb", "name":
"$PACKAGE", "branch": "master"}' \
  --json-input --modname=git --topic=branch

I guess we may want to ask Ralph for some input here

But then again, that's something for after the freeze :)


Pierre

[Pierre-Yves Chibon]

On Wed, Feb 25, 2015 at 01:48:12PM +0100, Pierre-Yves Chibon wrote:
>  On Wed, Feb 25, 2015 at 06:36:27AM -0500, Patrick Uiterwijk wrote:
>  > Hi,
>  > 
>  > So another freeze break request, +1s appreciated.
>  > This change is the only thing needed to make non-master-repos an option.
>  > Currently, the code always removes master from the requested branches, and crashes
> if it wasn't there.
>  > I'm just adding a check to only remove it (and announce the package) if it was
> requested.
>  > 
>  > 
>  > commit d230b5701575879e757f2c4aeb8eed62c319d9e6
>  > Author: Patrick Uiterwijk <puiterwijk(a)redhat.com&gt;
>  > Date:   Wed Feb 25 11:34:07 2015 +0000
>  > 
>  >     Only remove master from request if it was requested
>  >     
>  >     Signed-off-by: Patrick Uiterwijk <puiterwijk(a)redhat.com&gt;
>  > 
>  > diff --git a/roles/distgit/templates/pkgdb_sync_git_branches.py
> b/roles/distgit/templates/pkgdb_sync_git_branches.py
>  > index c931d26..ced139b 100644
>  > --- a/roles/distgit/templates/pkgdb_sync_git_branches.py
>  > +++ b/roles/distgit/templates/pkgdb_sync_git_branches.py
>  > @@ -189,16 +189,17 @@ def branch_package(pkgname, branches):
>  >      if not os.path.exists(
>  >              os.path.join(GIT_FOLDER, '%s.git' % pkgname)):
>  >          _invoke(SETUP_PACKAGE, [pkgname])
>  > -        branches.remove('master')  # SETUP_PACKAGE creates master
>  > -        fedmsg.publish(
>  > -            topic='branch',
>  > -            modname='git',
>  > -            msg=dict(
>  > -                agent='pkgdb',
>  > -                name=pkgname,
>  > -                branch='master',
>  > -            ),
>  > -        )
>  > +        if 'master' in branches:
>  > +            branches.remove('master')  # SETUP_PACKAGE creates master
>  > +            fedmsg.publish(
>  > +                topic='branch',
>  > +                modname='git',
>  > +                msg=dict(
>  > +                    agent='pkgdb',
>  > +                    name=pkgname,
>  > +                    branch='master',
>  > +                ),
>  > +             ) 
+1 for the change as it fixes the 'bug' we saw and doesn't change from the
current situation.
But sending the message at a different place than where the action is done
doesn't sound right for me, maybe something for later.

Pierre

[Kevin Fenzi]

On Wed, 25 Feb 2015 06:36:27 -0500 (EST)
Patrick Uiterwijk <puiterwijk(a)redhat.com&gt; wrote:

>  Hi,
>  
>  So another freeze break request, +1s appreciated.
>  This change is the only thing needed to make non-master-repos an
>  option.  
I'm pretty sure we don't want to decide to change this option during
freeze. ;) 

>  Currently, the code always removes master from the requested
>  branches, and crashes if it wasn't there. I'm just adding a check to
>  only remove it (and announce the package) if it was requested. 
I guess +1 to this anyhow, as it could fix a error condition. 

kevin
--
>  
>  commit d230b5701575879e757f2c4aeb8eed62c319d9e6
>  Author: Patrick Uiterwijk <puiterwijk(a)redhat.com&gt;
>  Date:   Wed Feb 25 11:34:07 2015 +0000
>  
>      Only remove master from request if it was requested
>      
>      Signed-off-by: Patrick Uiterwijk <puiterwijk(a)redhat.com&gt;
>  
>  diff --git a/roles/distgit/templates/pkgdb_sync_git_branches.py
>  b/roles/distgit/templates/pkgdb_sync_git_branches.py index
>  c931d26..ced139b 100644 ---
>  a/roles/distgit/templates/pkgdb_sync_git_branches.py +++
>  b/roles/distgit/templates/pkgdb_sync_git_branches.py @@ -189,16
>  +189,17 @@ def branch_package(pkgname, branches): if not
>  os.path.exists( os.path.join(GIT_FOLDER, '%s.git' % pkgname)):
>           _invoke(SETUP_PACKAGE, [pkgname])
>  -        branches.remove('master')  # SETUP_PACKAGE creates master
>  -        fedmsg.publish(
>  -            topic='branch',
>  -            modname='git',
>  -            msg=dict(
>  -                agent='pkgdb',
>  -                name=pkgname,
>  -                branch='master',
>  -            ),
>  -        )
>  +        if 'master' in branches:
>  +            branches.remove('master')  # SETUP_PACKAGE creates master
>  +            fedmsg.publish(
>  +                topic='branch',
>  +                modname='git',
>  +                msg=dict(
>  +                    agent='pkgdb',
>  +                    name=pkgname,
>  +                    branch='master',
>  +                ),
>  +             )
>   
>       # Create all the required branches for the package
>       # Use the translated branch name until pkgdb falls inline
>  
>  
>  
>  With kind regards,
>  Patrick Uiterwijk
>  Fedora Infra
>  
>  _______________________________________________
>  infrastructure mailing list
>  infrastructure(a)lists.fedoraproject.org
>  https://admin.fedoraproject.org/mailman/listinfo/infrastructure

[Patrick Uiterwijk]

Thanks.
And I personally think we should not do this, but was told we should consider the option.

With kind regards,
Patrick Uiterwijk
Fedora Infra

----- Original Message -----
>  On Wed, 25 Feb 2015 06:36:27 -0500 (EST)
>  Patrick Uiterwijk <puiterwijk(a)redhat.com&gt; wrote:
>  
>  > Hi,
>  > 
>  > So another freeze break request, +1s appreciated.
>  > This change is the only thing needed to make non-master-repos an
>  > option.
>  
>  I'm pretty sure we don't want to decide to change this option during
>  freeze. ;)
>  
>  > Currently, the code always removes master from the requested
>  > branches, and crashes if it wasn't there. I'm just adding a check to
>  > only remove it (and announce the package) if it was requested.
>  
>  I guess +1 to this anyhow, as it could fix a error condition.
>  
>  kevin
>  --
>  > 
>  > commit d230b5701575879e757f2c4aeb8eed62c319d9e6
>  > Author: Patrick Uiterwijk <puiterwijk(a)redhat.com&gt;
>  > Date:   Wed Feb 25 11:34:07 2015 +0000
>  > 
>  >     Only remove master from request if it was requested
>  >     
>  >     Signed-off-by: Patrick Uiterwijk <puiterwijk(a)redhat.com&gt;
>  > 
>  > diff --git a/roles/distgit/templates/pkgdb_sync_git_branches.py
>  > b/roles/distgit/templates/pkgdb_sync_git_branches.py index
>  > c931d26..ced139b 100644 ---
>  > a/roles/distgit/templates/pkgdb_sync_git_branches.py +++
>  > b/roles/distgit/templates/pkgdb_sync_git_branches.py @@ -189,16
>  > +189,17 @@ def branch_package(pkgname, branches): if not
>  > os.path.exists( os.path.join(GIT_FOLDER, '%s.git' % pkgname)):
>  >          _invoke(SETUP_PACKAGE, [pkgname])
>  > -        branches.remove('master')  # SETUP_PACKAGE creates master
>  > -        fedmsg.publish(
>  > -            topic='branch',
>  > -            modname='git',
>  > -            msg=dict(
>  > -                agent='pkgdb',
>  > -                name=pkgname,
>  > -                branch='master',
>  > -            ),
>  > -        )
>  > +        if 'master' in branches:
>  > +            branches.remove('master')  # SETUP_PACKAGE creates master
>  > +            fedmsg.publish(
>  > +                topic='branch',
>  > +                modname='git',
>  > +                msg=dict(
>  > +                    agent='pkgdb',
>  > +                    name=pkgname,
>  > +                    branch='master',
>  > +                ),
>  > +             )
>  >  
>  >      # Create all the required branches for the package
>  >      # Use the translated branch name until pkgdb falls inline
>  > 
>  > 
>  > 
>  > With kind regards,
>  > Patrick Uiterwijk
>  > Fedora Infra
>  > 
>  > _______________________________________________
>  > infrastructure mailing list
>  > infrastructure(a)lists.fedoraproject.org
>  > https://admin.fedoraproject.org/mailman/listinfo/infrastructure
>  
>  _______________________________________________
>  infrastructure mailing list
>  infrastructure(a)lists.fedoraproject.org
>  https://admin.fedoraproject.org/mailman/listinfo/infrastructure