On Fri, 01 Mar 2013 16:00:18 +0100 Petr Schindler pschindl@redhat.com wrote:

Hi,

when I was going through the code of FBS I found some quite unclear parts. You can find attached my small patch with changes that, I think, make some things clearer. Please review it, I hope that at least some of those changes will make it to the code.

This probably should have gone to qa-devel@ since autoqa-devel@ is going to be going away before long.

It looks good for the most part, I'm not sure about why the changes to image_builder/imagebuilder.py are needed or how they improve the code. Is there some reason that not using instance variables is better outside of individual stylistic preference?

Tim

-------PATCH-------

diff --git a/client/imagebuild.py b/client/imagebuild.py index 78a29d4..b76e266 100755 --- a/client/imagebuild.py +++ b/client/imagebuild.py @@ -115,7 +115,7 @@ def main(): # when this script has distributed mode, this will go away opts.__dict__['email']='foo@bar.com'

• if None in [option_value for option_value in

opts.__dict__.values()]:

• if None in opts.__dict__.values(): print 'You must atleast specify the image type and the

configuration file' parser.print_help() sys.exit() diff --git a/image_builder/imagebuilder.py b/image_builder/imagebuilder.py index 205b81c..0116edf 100644 --- a/image_builder/imagebuilder.py +++ b/image_builder/imagebuilder.py @@ -177,25 +177,25 @@ class ImageBuilder: # boot if self.iso_type == 'boot': self.logger.info('Starting the Image Build Process')

•        self.imgloc = worker.build_bootiso()
  

•        imgloc = worker.build_bootiso()
  
     # DVD
     if self.iso_type == 'dvd':
         self.logger.info('Starting the Image Build Process')
  

•        self.imgloc = worker.build_dvd(self.kickstart)
  

•        imgloc = worker.build_dvd(self.kickstart)
  
     #Live image
     if self.iso_type == 'live':
         self.logger.info('Starting the Image Build Process')
  

•        self.imgloc = worker.build_live(self.kickstart)
  

•        imgloc = worker.build_live(self.kickstart)
  
     self.logger.info('Image building process complete')
  
     #transfer image(s) and logs
  

•    if self.imgloc:
  

•    if imgloc:
         self.logger.info('Image successfully created.
  

Transferring to staging.')

•        t = Transfer(self.buildconfig, self.imgloc, self.logfile)
  

•        t = Transfer(self.buildconfig, imgloc, self.logfile)
         status = t.transfer()
  
         if status == 0:
  

@@ -206,14 +206,14 @@ class ImageBuilder:

         if status == -1:
             self.logger.info('Error in transfering

image(s)/logs')

•            dirname, temp =
  

os.path.split(os.path.abspath(self.imgloc[0]))

•            dirname, temp =
  

os.path.split(os.path.abspath(imgloc[0])) self.logger.info('Image(s) available in {0:s} on {1:s}'.format(dirname,os.uname()[1])) #build completion notification email self.notify_email_final() return -1 else: self.logger.info('Error creating image. Transferring Logs.')

•        t = Transfer(self.buildconfig, self.imgloc, self.logfile)
  

•        t = Transfer(self.buildconfig, imgloc, self.logfile)
         status = t.transfer()
  
         if status == 0:
  

diff --git a/image_builder/util.py b/image_builder/util.py index 2fe14cd..3ba9c8e 100644 --- a/image_builder/util.py +++ b/image_builder/util.py @@ -151,8 +151,8 @@ class Utilities: if ks_fname.startswith(('http', 'https')): # get the 'raw' file URL if its from the git repo if 'spin-kickstarts.git' in ks_fname:

•            ks_fname=ks_fname.split('tree')[0] + 'plain' +
  

ks_fname.split('tree')[1]

•            ks_fname = ks_fname.replace('tree', 'plain', 1)
  

•            
         # download and then JSON dump
         import urllib2
         try:
  

diff --git a/image_builder/worker.py b/image_builder/worker.py index a31e594..277f259 100644 --- a/image_builder/worker.py +++ b/image_builder/worker.py @@ -122,13 +122,11 @@ class Worker(): def prep_siderepo(self, workdir, packages, arch): """ prepare a side repository given extra packages """

•    arch = [arch]
  

•    arch = [arch, 'noarch']
  
     # DVD needs the SRPMs. Others don't.
     if self.buildconfig['default']['type'] == 'dvd':
  

•        arch.extend(['noarch','src'])
  

•    else:
  

•        arch.append('noarch')
  

•        arch.append('src')
  
     repodir = '{0:s}/siderepo'.format(workdir)
     repo_create = RepoCreate(repodir, arch)