Re: [libvirt] [sandbox PATCH v2 04/19] Image: Add download function

On Tue, Aug 04, 2015 at 08:11:10PM +0000, Eren Yagdiran wrote: > Refactor download function from virt-sandbox-image to use > the newly introduced Source abstract class. The docker-specific > download code is moved to a new DockerSource class. > --- > virt-sandbox-image/Makefile.am | 1 + > virt-sandbox-image/sources/DockerSource.py | 227 +++++++++++++++++++++++++++++ > virt-sandbox-image/sources/Source.py | 4 + > virt-sandbox-image/virt-sandbox-image.py | 199 ++++--------------------- > 4 files changed, 259 insertions(+), 172 deletions(-) > create mode 100644 virt-sandbox-image/sources/DockerSource.py > > diff --git a/virt-sandbox-image/Makefile.am b/virt-sandbox-image/Makefile.am > index 5ab4d2e..8188c80 100644 > --- a/virt-sandbox-image/Makefile.am > +++ b/virt-sandbox-image/Makefile.am > @@ -8,6 +8,7 @@ install-data-local: > $(INSTALL) -m 0755 $(srcdir)/virt-sandbox-image.py $(DESTDIR)$(pkgpythondir) > $(INSTALL) -m 0644 $(srcdir)/sources/__init__.py $(DESTDIR)$(pkgpythondir)/sources > $(INSTALL) -m 0644 $(srcdir)/sources/Source.py $(DESTDIR)$(pkgpythondir)/sources > + $(INSTALL) -m 0644 $(srcdir)/sources/DockerSource.py $(DESTDIR)$(pkgpythondir)/sources > > uninstall-local: > rm -f $(DESTDIR)$(pkgpythondir) > diff --git a/virt-sandbox-image/sources/DockerSource.py b/virt-sandbox-image/sources/DockerSource.py > new file mode 100644 > index 0000000..cf81ffe > --- /dev/null > +++ b/virt-sandbox-image/sources/DockerSource.py > @@ -0,0 +1,227 @@ > +''' > +* > +* libvirt-sandbox-config-diskaccess.h: libvirt sandbox configuration > +* > +* Copyright (C) 2015 Universitat Politècnica de Catalunya. > +* > +* This library is free software; you can redistribute it and/or > +* modify it under the terms of the GNU Lesser General Public > +* License as published by the Free Software Foundation; either > +* version 2.1 of the License, or (at your option) any later version. > +* > +* This library is distributed in the hope that it will be useful, > +* but WITHOUT ANY WARRANTY; without even the implied warranty of > +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > +* Lesser General Public License for more details. > +* > +* You should have received a copy of the GNU Lesser General Public > +* License along with this library; if not, write to the Free Software > +* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > +* > +* Author: Eren Yagdiran <erenyagdiran(a)gmail.com&gt; > +* > +''' > +#!/usr/bin/python > + > +from Source import Source > +import urllib2 > +import sys > +import json > +import traceback > +import os > +import subprocess > +import shutil > + > +class DockerSource(Source): > + default_index_server = "index.docker.io" > + default_template_dir = "/var/lib/libvirt/templates" > + default_image_path = "/var/lib/libvirt/templates" > + default_disk_format = "qcow2" These are class level variables > + def __init__(self,server="index.docker.io",destdir="/var/lib/libvirt/templates"): > + self.default_index_server = server > + self.default_template_dir = destdir And these are object level variables with the same name. Generally object level variables should have a leading _ to indicate that they are private. I'd suggest we remove the default directories from this bit of code though. > def download(args): > - info("Downloading %s from %s to %s\n" % (args.name, default_index_server, default_template_dir)) > - download_template(args.name, default_index_server, default_template_dir) > + try: > + dynamic_source_loader(args.source).download_template(name=args.name, > + registry=args.registry, > + username=args.username, > + password=args.password, > + templatedir=args.template_dir) > + except IOError,e: > + print "Source %s cannot be found in given path" %args.source > + except Exception,e: > + print "Download Error %s" % str(e) > > def delete(args): > info("Deleting %s from %s\n" % (args.name, default_template_dir)) > @@ -355,10 +193,27 @@ def requires_name(parser): > parser.add_argument("name", > help=_("name of the template")) > > +def requires_source(parser): > + parser.add_argument("-s","--source", > + default="docker", > + help=_("name of the template")) > + > +def requires_auth_conn(parser): > + parser.add_argument("-r","--registry", > + help=_("Url of the custom registry")) > + parser.add_argument("-u","--username", > + help=_("Username for the custom registry")) > + parser.add_argument("-p","--password", > + help=_("Password for the custom registry")) > + parser.add_argument("-t","--template-dir", > + help=_("Template directory for saving templates")) And register a default here instead. Also, we will want to have a different default directory for people running as non-root, as /var/lib/libvirt/templates won't be accessible for people using qemu:///session