[PATCH] distgit: Upload files to both the new and old path

[Mathieu Bridon]

On Thu, 2015-05-28 at 14:17 -0500, Dennis Gilmore wrote:
> On Thursday, May 28, 2015 05:52:53 PM Pierre-Yves Chibon wrote:
> > On Thu, May 28, 2015 at 10:36:54AM -0500, Dennis Gilmore wrote:
> > > On Thursday, May 28, 2015 02:05:44 PM Mathieu Bridon wrote:
> > > > From: Mathieu Bridon <bochecha at daitauha.fr>
> > > > 
> > > > Currently, the CGI script is set to upload files:
> > > > - to the old path if the upload uses md5
> > > > - to the new path if the upload uses sha512
> > > > 
> > > > The old path is as follows:
> > > >     /%(srpmname)s/%(filename)s/%(hash)s/%(filename)s
> > > > 
> > > > The new path is:
> > > >     /%(srpmname)s/%(filename)s/%(hashtype)s/%(hash)s/%(filename)s
> > > > 
> > > > This was meant to ensure compatibility with current fedpkg which
> > > > always downloads from the old path, but will eventually download from
> > > > the new path when we move to sha512.
> > > > 
> > > > However, working more on this, I now think it would make for a smoother
> > > > transition if we instead always stored the files at the new path, but
> > > > just hardlinked to the old path if the upload is using md5.
> > > > 
> > > > This is what this patch achieves.
> > > > 
> > > > With this deployed in production, fedpkg could be patched to try
> > > > downloading from the new path, and fallback to the old one if necessary,
> > > > which decouples the migration to the new path from the migration to the
> > > > new hash.
> > > > ---
> > > > 
> > > >  roles/distgit/files/dist-git-upload.cgi | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/roles/distgit/files/dist-git-upload.cgi
> > > > b/roles/distgit/files/dist-git-upload.cgi index b4fda74..38c40db 100644
> > > > --- a/roles/distgit/files/dist-git-upload.cgi
> > > > +++ b/roles/distgit/files/dist-git-upload.cgi
> > > > 
> > > > @@ -112,11 +112,6 @@ def main():
> > > >      hash_dir = os.path.join(module_dir, filename, hash_type, checksum)
> > > >      msgpath = os.path.join(name, module_dir, filename, hash_type,
> > > >      checksum,
> > > > 
> > > > filename)
> > > > 
> > > > -    if hash_type == "md5":
> > > > -        # Preserve compatibility with the current folder hierarchy for
> > > > md5
> > > > -        hash_dir = os.path.join(module_dir, filename, checksum)
> > > > -        msgpath = os.path.join(name, module_dir, filename, checksum,
> > > > filename) -
> > > > 
> > > >      unwanted_prefix = '/srv/cache/lookaside/pkgs/'
> > > >      
> > > >      if msgpath.startswith(unwanted_prefix):
> > > >          msgpath = msgpath[len(unwanted_prefix):]
> > > > 
> > > > @@ -180,6 +175,11 @@ def main():
> > > >      print >> sys.stderr, '[username=%s] Stored %s (%d bytes)' %
> > > >      (username,
> > > > 
> > > > dest_file, filesize) print 'File %s size %d %s %s stored OK' %
> > > > (filename,
> > > > filesize, hash_type.upper(), checksum)
> > > > 
> > > > +    # Add the file to the old path, where fedpkg is currently looking
> > > > for
> > > > it +    if hash_type == "md5":
> > > > +        old_path = os.path.join(module_dir, filename, checksum,
> > > > filename)
> > > > +        os.link(dest_file, old_path)
> > > > +
> > > > 
> > > >      # Emit a fedmsg message.  Load the config to talk to the
> > > >      fedmsg-relay.
> > > >      
> > > >      try:
> > > >          config = fedmsg.config.load_config([], None)
> > > 
> > > The idea is fine, but it is a bit hard to tell from this patch what
> > > exactly is going on as there is no context.
> > 
> > You mean in the code? I find the commit message to be pretty explanatory.
> > What are you looking for?
> 
> Yeah in the code.

Overall, the whole of the code of this CGI script is hard to read and
understand.

What I'm doing in this patch is quite simple, though, and explained in
the commit message. (and that's what commit messages are for, after all)

I'd be happy to replace the CGI script altogether by something else (how
about replacing distgit by pagure, and just storing the tarballs inside
pagure somewhere?), but that's a very different discussion. :)

Right now, all I want is make progress on the move away from MD5, and
this patch helps with that.

Given that I received two +1, and that you agreed to the general idea,
I've pushed it, and Pierre-Yves is helping me run the playbook to test
it in staging.


-- 
Mathieu