Misc cobbler cleanup. Switched to the new yaml parser (it's uglier,

need to do something about that).  Added some tests.  Modified test
code so it never clobbers a working install.

6 files changed, 169 insertions, 138 deletions

diff --git a/cobbler/api.py b/cobbler/api.py
index 429fa33..953dd2e 100644
--- a/cobbler/api.py
+++ b/cobbler/api.py
@@ -1,6 +1,9 @@
-# friendly OO python API module for BootConf 
-#
-# Michael DeHaan <mdehaan@redhat.com>
+"""
+python API module for BootConf 
+see source for bootconf.py for a good API reference
+
+Michael DeHaan <mdehaan@redhat.com>
+"""
 
 import exceptions
 import os
@@ -68,50 +71,58 @@ class BootAPI:
 
     def new_system(self):
        """
-       Create a blank, unconfigured system
+       Return a blank, unconfigured system, unattached to a collection
        """
        return System(self,None)
 
 
     def new_distro(self):
        """
-       Create a blank, unconfigured distro
+       Create a blank, unconfigured distro, unattached to a collection.
        """
        return Distro(self,None)
 
 
     def new_profile(self):
        """
-       Create a blank, unconfigured profile
+       Create a blank, unconfigured profile, unattached to a collection
        """
        return Profile(self,None)
 
 
     def check(self):
        """
-       See if all preqs for network booting are operational
+       See if all preqs for network booting are valid.  This returns
+       a list of strings containing instructions on things to correct.
+       An empty list means there is nothing to correct, but that still
+       doesn't mean there are configuration errors.  This is mainly useful
+       for human admins, who may, for instance, forget to properly set up
+       their TFTP servers for PXE, etc.
        """
        return check.BootCheck(self).run()
 
 
     def sync(self,dry_run=True):
        """
-       Update the system with what is specified in the config file
+       Take the values currently written to the configuration files in
+       /etc, and /var, and build out the information tree found in 
+       /tftpboot.  Any operations done in the API that have not been
+       saved with serialize() will NOT be synchronized with this command.
        """ 
        self.config.deserialize();
        configurator = sync.BootSync(self)
-       configurator.sync(dry_run)
+       return configurator.sync(dry_run)
 
 
     def serialize(self):
        """
-       Save the config file
+       Save the config file(s) to disk.
        """
        self.config.serialize() 
     
     def deserialize(self):
        """
-       Make the API's internal state reflect that of the config file
+       Load the current configuration from config file(s)
        """
        self.config.deserialize()
 
@@ -125,7 +136,8 @@ class Collection:
 
     def find(self,name):
         """
-        Return anything named 'name' in the collection, else return None
+        Return anything named 'name' in the collection, else return None if
+        no objects can be found.
         """
         if name in self.listing.keys():
             return self.listing[name]
@@ -134,14 +146,18 @@ class Collection:
 
     def to_datastruct(self):
         """
-        Return datastructure representation (to feed to serializer)
+        Return datastructure representation of this collection suitable
+        for feeding to a serializer (such as YAML)
         """
         return [x.to_datastruct() for x in self.listing.values()]
     
      
     def add(self,ref):
         """
-        Add an object to the collection, if it's valid
+        Add an object to the collection, if it's valid.  Returns True
+        if the object was added to the collection.  Returns False if the
+        object specified by ref deems itself invalid (and therefore
+        won't be added to the collection).
         """
         if ref is None or not ref.is_valid(): 
             if self.api.last_error is None or self.api.last_error == "":
@@ -151,23 +167,26 @@ class Collection:
         return True
 
 
-    def __str__(self):
+    def printable(self):
         """
-        Printable representation
+        Creates a printable representation of the collection suitable
+        for reading by humans or parsing from scripts.  Actually scripts
+        would be better off reading the YAML in the config files directly.
         """
         buf = ""
-        values = map(lambda(a): str(a), sorted(self.listing.values()))
+        values = map(lambda(a): a.printable(), sorted(self.listing.values()))
         if len(values) > 0: 
            return "\n\n".join(values)
         else:
            return m("empty_list")
 
-    def contents(self):
-        """
-	Access the raw contents of the collection.  Classes shouldn't
-	be doing this (preferably) and should use the __iter__ interface
-	"""
-        return self.listing.values()
+    #def contents(self):
+    #    """
+    #	Access the raw contents of the collection.  Classes shouldn't
+    #	be doing this (preferably) and should use the __iter__ interface.
+    #    Deprecrated.
+    #	 """
+    #    return self.listing.values()
 
     def __iter__(self):
         """
@@ -375,7 +394,7 @@ class Distro(Item):
            'kernel_options' : self.kernel_options
         }
 
-    def __str__(self):
+    def printable(self):
         """
 	Human-readable representation.
 	"""
@@ -417,6 +436,8 @@ class Profile(Item):
            self.kickstart       = seed_data['kickstart'] 
            self.kernel_options  = seed_data['kernel_options']
            self.xen_name        = seed_data['xen_name']
+           if not self.xen_name or self.xen_name == '':
+              self.xen_name = self.name
            self.xen_ram         = seed_data['xen_ram']
            self.xen_file_size   = seed_data['xen_file_size']
            self.xen_mac         = seed_data['xen_mac']
@@ -460,19 +481,6 @@ class Profile(Item):
         self.xen_name = str
         return True
 
-    def set_xen_file_path(self,str):
-        """
-	For Xen only.
-	Specifies that Xen filenames be stored in path specified by 'str'.
-	Paths must be absolute.  xen-net-install will ignore this suggestion
-	if it cannot write to the given location.
-	""" 
-        # path must look absolute
-        if len(str) < 1 or str[0] != "/":
-            return False
-        self.xen_file_path = str
-        return True
-
     def set_xen_file_size(self,num):
         """
 	For Xen only.
@@ -555,14 +563,13 @@ class Profile(Item):
             'xen_paravirt'    : self.xen_paravirt
         }
 
-    def __str__(self):
+    def printable(self):
         buf = ""
         buf = buf + "profile         : %s\n" % self.name
         buf = buf + "distro          : %s\n" % self.distro
         buf = buf + "kickstart       : %s\n" % self.kickstart
         buf = buf + "kernel opts     : %s" % self.kernel_options
-        buf = buf + "xen name prefix : %s" % self.xen_name
-        buf = buf + "xen file path   : %s" % self.xen_file_path
+        buf = buf + "xen name        : %s" % self.xen_name
         buf = buf + "xen file size   : %s" % self.xen_file_size
         buf = buf + "xen ram         : %s" % self.xen_ram
         buf = buf + "xen mac         : %s" % self.xen_mac
@@ -625,7 +632,7 @@ class System(Item):
            'kernel_options' : self.kernel_options
         }
 
-    def __str__(self):
+    def printable(self):
         buf = ""
         buf = buf + "system       : %s\n" % self.name
         buf = buf + "profile      : %s\n" % self.profile
diff --git a/cobbler/check.py b/cobbler/check.py
index 15f52ae..aa5864e 100644
--- a/cobbler/check.py
+++ b/cobbler/check.py
@@ -1,9 +1,7 @@
-# Validates a system is configured for network booting
+# Classes for validating whether asystem is configured for network booting
 # 
 # Michael DeHaan <mdehaan@redhat.com>
 
-# FUTURE: Check to see what's running
-
 import os
 import sys
 import re
@@ -20,7 +18,8 @@ class BootCheck:
    def run(self):
        """
        Returns None if there are no errors, otherwise returns a list 
-       of things to correct prior to running $0 'for real'.
+       of things to correct prior to running application 'for real'.
+       (The CLI usage is "cobbler check" before "cobbler sync")
        """
        status = []
        self.check_name(status)
diff --git a/cobbler/cobbler.py b/cobbler/cobbler.py
index 2ff030b..30baaff 100755
--- a/cobbler/cobbler.py
+++ b/cobbler/cobbler.py
@@ -2,8 +2,8 @@
 # Michael DeHaan <mdehaan@redhat.com>
 
 """
-Command line interface for BootConf, a network boot configuration
-library
+Command line interface for cobbler, a network provisioning configuration
+library.  Consult 'man cobbler' for general info.
 """
 
 import os
@@ -76,28 +76,28 @@ class BootCLI:
 
     def system_list(self,args):
         """
-        Print out the list of systems:  '$0 system list'
+        Print out the list of systems:  'cobbler system list'
         """
-        print str(self.api.get_systems())
-
+        print self.api.get_systems().printable()
+        return True
 
     def profile_list(self,args):
         """
-        Print out the list of profiles: '$0 profile list'
+        Print out the list of profiles: 'cobbler profile list'
         """
-        print str(self.api.get_profiles())
-    
+        print self.api.get_profiles().printable()
+        return True
 
     def distro_list(self,args):
         """
-        Print out the list of distros: '$0 distro list'
+        Print out the list of distros: 'cobbler distro list'
         """
-        print str(self.api.get_distros())
-
+        print self.api.get_distros().printable()
+        return True
 
     def system_remove(self,args):
         """
-        Delete a system:  '$0 system remove --name=foo'
+        Delete a system:  'cobbler system remove --name=foo'
         """
         commands = {
            '--name'       : lambda(a):  self.api.get_systems().remove(a) 
@@ -108,7 +108,7 @@ class BootCLI:
 
     def profile_remove(self,args):
         """
-        Delete a profile:   '$0 profile remove --name=foo'
+        Delete a profile:   'cobbler profile remove --name=foo'
         """
         commands = {
            '--name'       : lambda(a):  self.api.get_profiles().remove(a)
@@ -119,7 +119,7 @@ class BootCLI:
 
     def distro_remove(self,args):
         """
-        Delete a distro:  '$0 distro remove --name='foo'
+        Delete a distro:  'cobbler distro remove --name='foo'
         """
         commands = {
            '--name'     : lambda(a):  self.api.get_distros().remove(a)
@@ -130,7 +130,7 @@ class BootCLI:
 
     def system_edit(self,args):
         """
-        Create/Edit a system:  '$0 system edit --name='foo' ...
+        Create/Edit a system:  'cobbler system edit --name='foo' ...
         """
         sys = self.api.new_system()
         commands = {
@@ -145,7 +145,7 @@ class BootCLI:
 
     def profile_edit(self,args):
         """
-        Create/Edit a profile:  '$0 profile edit --name='foo' ...
+        Create/Edit a profile:  'cobbler profile edit --name='foo' ...
         """
         profile = self.api.new_profile()
         commands = {
@@ -155,10 +155,11 @@ class BootCLI:
             '--kopts'           :  lambda(a) : profile.set_kernel_options(a),
             '--xen-name'        :  lambda(a) : profile.set_xen_name(a),
             '--xen-file-size'   :  lambda(a) : profile.set_xen_file_size(a),
-            '--xen-ram'         :  lambda(a) : profile.set_xen_ram(a),
-            '--xen-mac'         :  lambda(a) : profile.set_xen_mac(a),
-            '--xen-paravirt'    :  lambda(a) : profile.set_xen_paravirt(a),
-            # FIXME: more Xen opts that xen-guest-install needs
+            '--xen-ram'         :  lambda(a) : profile.set_xen_ram(a)
+        # the following options are most likely not useful for profiles (yet)
+        # primarily due to not being implemented in koan.  
+        #    '--xen-mac'         :  lambda(a) : profile.set_xen_mac(a),
+        #    '--xen-paravirt'    :  lambda(a) : profile.set_xen_paravirt(a),
         }
         on_ok = lambda: self.api.get_profiles().add(profile)
         return self.apply_args(args,commands,on_ok,True)
@@ -166,7 +167,7 @@ class BootCLI:
 
     def distro_edit(self,args):
         """
-        Create/Edit a distro:  '$0 distro edit --name='foo' ...
+        Create/Edit a distro:  'cobbler distro edit --name='foo' ...
         """
         distro = self.api.new_distro()
         commands = {
@@ -181,7 +182,7 @@ class BootCLI:
 
     def apply_args(self,args,input_routines,on_ok,serialize):
         """
-        Instead of getopt...
+        Custom CLI handling, instead of getopt/optparse
         Parses arguments of the form --foo=bar, see profile_edit for example
         """
         if len(args) == 0:
@@ -189,18 +190,26 @@ class BootCLI:
             return False
         for x in args:
             try:
+                # all arguments must be of the form --key=value
                 key, value = x.split("=",1)
                 value = value.replace('"','').replace("'",'')
             except:
                 print m("bad_arg") % x
                 return False
             if key in input_routines:
+                # --argument is recognized, so run the loader
+                # attached to it in the dispatch table
                 if not input_routines[key](value):
+                   # loader does not like passed value
                    print m("reject_arg") % key
                    return False
             else:
+                # --argument is not recognized
                 print m("weird_arg") % key
                 return False
+        # success thus far, so run the success routine for the set of
+        # arguments.  Configuration will only be written to file if the
+        # final routine succeeds.
         rc = on_ok()
         if rc and serialize:
             self.api.serialize()
@@ -216,6 +225,9 @@ class BootCLI:
             print m("help")
             return False
         if args[0] in commands:
+            # if the subargument is in the dispatch table, run
+            # the selected command routine with the rest of the 
+            # arguments
             rc = commands[args[0]](args[1:])
             if not rc:
                return False
@@ -227,7 +239,7 @@ class BootCLI:
 
     def sync(self, args):
         """
-        Sync the config file with the system config: '$0 sync [--dryrun]'
+        Sync the config file with the system config: 'cobbler sync [--dryrun]'
         """
         status = None 
         if args is not None and "--dryrun" in args:
@@ -239,7 +251,7 @@ class BootCLI:
 
     def check(self,args):
         """
-        Check system for network boot decency/prereqs: '$0 check'
+        Check system for network boot decency/prereqs: 'cobbler check'
         """
         status = self.api.check()
         if status is None:
@@ -256,26 +268,35 @@ class BootCLI:
 
     def distro(self,args):
         """
-        Handles any of the '$0 distro' subcommands
+        Handles any of the 'cobbler distro' subcommands
         """
         return self.curry_args(args, self.commands['distro']) 
 
 
     def profile(self,args):
         """
-        Handles any of the '$0 profile' subcommands
+        Handles any of the 'cobbler profile' subcommands
         """
         return self.curry_args(args, self.commands['profile'])
 
 
     def system(self,args):
         """
-        Handles any of the '$0 system' subcommands
+        Handles any of the 'cobbler system' subcommands
         """
         return self.curry_args(args, self.commands['system'])
 
 def main():
-    if os.getuid() != 0: # FIXME
+    """
+    CLI entry point
+    """
+    if os.getuid() != 0:
+       # while it's true that we don't technically need root, we do need
+       # permissions on a relatively long list of files that ordinarily 
+       # only root has access to, and we don't know specifically what
+       # files are where (other distributions in play, etc).  It's 
+       # fairly safe to assume root is required.  This might be patched
+       # later.
        print m("need_root")
        sys.exit(1)
     if BootCLI(sys.argv).run():
diff --git a/cobbler/config.py b/cobbler/config.py
index 6a1024e..8c8991a 100644
--- a/cobbler/config.py
+++ b/cobbler/config.py
@@ -8,42 +8,46 @@ import util
 from msg import *
 
 import os
-import yaml
-#import syck -- we *want* to use syck, but the FC syck currently does not 
-#            -- contain the dump function, i.e. not gonna work
+import yaml  # python yaml 3000 from pyyaml.org, soon to be in extras
 import traceback
 
+global_settings_file = "/etc/cobbler.conf"
+global_state_file = "/var/lib/cobbler/cobbler.conf"
+
 class BootConfig:
 
-    """
-    Constructor.  This class maintains both the logical
-    configuration for Boot and the file representation thereof.
-    Creating the config object only loads the default values,
-    users of this class need to call deserialize() to load config
-    file values.
-    """
     def __init__(self,api):
+        """
+        Constructor.  This class maintains both the logical
+        configuration for Cobbler and the file representation thereof.
+        Creating the config object only loads the default values,
+        users of this class need to call deserialize() to load config
+        file values.  See cobbler.py for how the CLI does it.
+        """
         self.api = api
-        self.settings_file    = "/etc/cobbler.conf"
-        self.state_file       = "/var/lib/cobbler/cobbler.conf"
+        self.settings_file    = global_settings_file
+        self.state_file       = global_state_file
         self.set_defaults()
         self.clear()
 
     def files_exist(self):
+        """
+        Returns whether the config files exist.
+        """
         return os.path.exists(self.settings_file) and os.path.exists(self.state_file)
 
-    """
-    Establish an empty list of profiles distros, and systems.
-    """
     def clear(self):
+        """
+        Establish an empty list of profiles distros, and systems.
+        """
         self.profiles       = api.Profiles(self.api,None)
         self.distros        = api.Distros(self.api,None)
         self.systems        = api.Systems(self.api,None)
 
-    """
-    Set some reasonable defaults in case no values are available
-    """
     def set_defaults(self):
+        """
+        Set some reasonable defaults in case no values are available
+        """
         self.server         = "localhost"
         self.tftpboot       = "/tftpboot"
         self.dhcpd_conf     = "/etc/dhcpd.conf"
@@ -54,30 +58,30 @@ class BootConfig:
         self.httpd_bin      = "/usr/sbin/httpd"
         self.kernel_options = "append devfs=nomount ramdisk_size=16438 lang= vga=788 ksdevice=eth0" #initrd and ks added programmatically
 
-    """
-    Access the current profiles list
-    """
     def get_profiles(self):
+        """
+        Access the current profiles list
+        """
         return self.profiles
 
-    """
-    Access the current distros list
-    """
     def get_distros(self):
+        """
+        Access the current distros list
+        """
         return self.distros
 
-    """
-    Access the current systems list
-    """
     def get_systems(self):
+        """
+        Access the current systems list
+        """
         return self.systems
 
-    """
-    Save all global config options in hash form (for serialization)
-    """
     def config_to_hash(self):
+        """
+        Save all global config options in hash form (for serialization)
+        """
         data = {}
-        data['server']         = self.server
+        data["server"]         = self.server
         data['tftpboot']       = self.tftpboot
         data['dhcpd_conf']     = self.dhcpd_conf
         data['tftpd_conf']     = self.tftpd_conf
@@ -88,10 +92,10 @@ class BootConfig:
         data['kernel_options'] = self.kernel_options
         return data
     
-    """
-    Load all global config options from hash form (for deserialization)
-    """
     def config_from_hash(self,hash):
+        """
+        Load all global config options from hash form (for deserialization)
+        """
         try:
             self.server          = hash['server']
             self.tftpboot        = hash['tftpboot']
@@ -105,11 +109,12 @@ class BootConfig:
         except:
             print "WARNING: config file error: %s" % (self.settings_file)
             self.set_defaults()
-    """
-    Convert all items cobbler knows about to a nested hash.
-    There are seperate hashes for the /etc and /var portions.
-    """
+    
     def to_hash(self,is_etc):
+        """
+        Convert all items cobbler knows about to a nested hash.
+        There are seperate hashes for the /etc and /var portions.
+        """
         world = {} 
         if is_etc:
             world['config']      = self.config_to_hash()
@@ -121,12 +126,11 @@ class BootConfig:
         return world  
 
 
-    """
-    Convert a hash representation of a cobbler to 'reality'
-    There are seperate hashes for the /etc and /var portions.
-    """
     def from_hash(self,hash,is_etc):
-        #print "DEBUG: %s" % hash
+        """
+        Convert a hash representation of a cobbler to 'reality'
+        There are seperate hashes for the /etc and /var portions.
+        """
         if is_etc:
             self.config_from_hash(hash['config'])
         else:
@@ -137,12 +141,12 @@ class BootConfig:
     # ------------------------------------------------------
     # we don't care about file formats until below this line
 
-    """
-    Save everything to the config file.
-    This goes through an intermediate data format so we
-    could use YAML later if we wanted.
-    """
     def serialize(self):
+        """
+        Save everything to the config file.
+        This goes through an intermediate data format so we
+        could use YAML later if we wanted.
+        """
 
         settings = None
         state = None
@@ -160,32 +164,31 @@ class BootConfig:
         # ------
         # dump internal state (distros, profiles, systems...)
         if not os.path.isdir(os.path.dirname(self.state_file)):
-            os.mkdir(os.path.dirname(self.state_file))
+            os.makedirs(os.path.dirname(self.state_file))
         try:
             state = open(self.state_file,"w+")
         except:
             self.api.last_error = m("cant_create: %s" % self.state_file)
+            return False
         data = self.to_hash(False)
         state.write(yaml.dump(data))
 
         # all good
         return True
 
-    """
-    Load everything from the config file.
-    This goes through an intermediate data structure format so we
-    could use YAML later if we wanted.
-    """
     def deserialize(self):
-        #print "DEBUG: deserialize"
+        """
+        Load everything from the config file.
+        This goes through an intermediate data structure format so we
+        could use YAML later if we wanted.
+        """
 
         # -----
         # load global config (pathing, urls, etc)...
         try:
-            settings = yaml.loadFile(self.settings_file)
-            raw_data = settings.next()
-            if raw_data is not None:
-                self.from_hash(raw_data,True)
+            settings = yaml.load(open(self.settings_file,"r").read())
+            if settings is not None:
+                return self.from_hash(settings,True)
             else:
                 print "WARNING: no %s data?" % self.settings_file
         except:
@@ -195,10 +198,9 @@ class BootConfig:
         # -----
         # load internal state(distros, systems, profiles...)
         try:
-            state = yaml.loadFile(self.state_file)
-            raw_data = state.next()
-            if raw_data is not None:
-                self.from_hash(raw_data,False)
+            state = yaml.load(open(self.state_file,"r").read())
+            if state is not None:
+                return self.from_hash(state,False)
             else:
                 print "WARNING: no %s data?" % self.state_file
         except:
diff --git a/cobbler/sync.py b/cobbler/sync.py
index 3adfbcf..a31e262 100644
--- a/cobbler/sync.py
+++ b/cobbler/sync.py
@@ -95,7 +95,7 @@ class BootSync:
         """
         # copy is a 4-letter word but tftpboot runs chroot, thus it's required.
         distros = os.path.join(self.api.config.tftpboot, "images")
-        for d in self.api.get_distros().contents():
+        for d in self.api.get_distros():
             distro_dir = os.path.join(distros,d.name)
             self.mkdir(distro_dir)
             kernel = self.api.utils.find_kernel(d.kernel) # full path
@@ -123,7 +123,7 @@ class BootSync:
         # these are served by either NFS, Apache, or some ftpd, so we don't need to copy them
         # it's up to the user to make sure they are nicely served by their URLs
 
-        for g in self.api.get_profiles().contents():
+        for g in self.api.get_profiles():
            self.sync_log("mirroring any local kickstarts: %s" % g.name)
            kickstart_path = self.api.utils.find_kickstart(g.kickstart)
            if kickstart_path and os.path.exists(kickstart_path):
diff --git a/cobbler/util.py b/cobbler/util.py
index f7dd473..e8cdf3f 100644
--- a/cobbler/util.py
+++ b/cobbler/util.py
@@ -148,6 +148,8 @@ class BootUtil:
        x = url.lower()
        for y in ["http://","nfs://","ftp://","/"]:
           if x.startswith(y):
+              if x.startswith("/") and not os.path.isfile(x):
+                  return None
               return url
        return None