PyChecker.

16 files changed, 162 insertions, 44 deletions

diff --git a/cobbler/action_check.py b/cobbler/action_check.py
index cb02f69..c310fc6 100644
--- a/cobbler/action_check.py
+++ b/cobbler/action_check.py
@@ -1,5 +1,6 @@
 """
-Classes for validating whether asystem is configured for network booting
+Validates whether the system is reasonably well configured for
+serving up content.  This is the code behind 'cobbler check'.
 
 Michael DeHaan <mdehaan@redhat.com>
 """
@@ -14,6 +15,9 @@ from msg import *
 class BootCheck:
 
    def __init__(self,config):
+       """
+       Constructor
+       """
        self.config   = config
        self.settings = config.settings()
 
@@ -35,10 +39,18 @@ class BootCheck:
        return status
 
    def check_name(self,status):
+       """
+       If the server name in the config file is still set to localhost
+       kickstarts run from koan will not have proper kernel line
+       parameters.
+       """
        if self.settings.server == "localhost":
           status.append(m("bad_server"))
 
    def check_httpd(self,status):
+       """
+       Check if Apache is installed.
+       """
        if not os.path.exists(self.settings.httpd_bin):
           status.append(m("no_httpd"))
 
@@ -98,7 +110,9 @@ class BootCheck:
    def check_dhcpd_conf(self,status):
        """
        Check that dhcpd *appears* to be configured for pxe booting.
-       We can't assure file correctness
+       We can't assure file correctness.  Since a cobbler user might
+       have dhcp on another server, it's okay if it's not there and/or
+       not configured correctly according to automated scans.
        """
        if os.path.exists(self.settings.dhcpd_conf):
            match_next = False
diff --git a/cobbler/action_sync.py b/cobbler/action_sync.py
index 7e0d35e..e804586 100644
--- a/cobbler/action_sync.py
+++ b/cobbler/action_sync.py
@@ -1,5 +1,6 @@
 """
-Code to vivify a configuration into a real TFTP/DHCP configuration.
+Builds out a TFTP/cobbler boot tree based on the object tree.
+This is the code behind 'cobbler sync'.
 
 Michael DeHaan <mdehaan@redhat.com>
 """
@@ -24,6 +25,9 @@ Handles conversion of internal state to the tftpboot tree layout
 class BootSync:
 
     def __init__(self,config):
+        """
+        Constructor
+        """
         self.verbose  = True
         self.config   = config
         self.distros  = config.distros()
@@ -124,7 +128,7 @@ class BootSync:
         Similar to what we do for distros, ensure all the kickstarts
         in conf file are valid.   kickstarts are referenced by URL
         (http or ftp), can stay as is.  kickstarts referenced by absolute
-        path will be mirrored over http.
+        path (i.e. are files path) will be mirrored over http.
         """
         # ensure all referenced kickstarts exist
         # these are served by either NFS, Apache, or some ftpd, so we don't need to copy them
@@ -151,7 +155,7 @@ class BootSync:
         configured IP or MAC address.  Also build a parallel 'xeninfo' tree
         for xen-net-install info.
         """
-        print "building trees..."
+        self.sync_log("building trees...")
         # create pxelinux.cfg under tftpboot
         # and file for each MAC or IP (hex encoded 01-XX-XX-XX-XX-XX-XX)
 
diff --git a/cobbler/api.py b/cobbler/api.py
index 4cae53d..37a5934 100644
--- a/cobbler/api.py
+++ b/cobbler/api.py
@@ -5,10 +5,6 @@ see source for cobbler.py, or pydoc, for example usage.
 Michael DeHaan <mdehaan@redhat.com>
 """
 
-import exceptions
-import os
-import traceback
-
 import config
 import utils
 import action_sync
diff --git a/cobbler/cobbler.py b/cobbler/cobbler.py
index 9ea56f1..989b50f 100755
--- a/cobbler/cobbler.py
+++ b/cobbler/cobbler.py
@@ -1,6 +1,7 @@
 """
 Command line interface for cobbler, a network provisioning configuration
-library.  Consult 'man cobbler' for general info.
+library.  Consult 'man cobbler' for general info.  This class serves
+as a good reference on how to drive the API (api.py).
 
 Michael DeHaan <mdehaan@redhat.com>
 """
@@ -137,7 +138,6 @@ class BootCLI:
         commands = {
            '--name'     :  lambda(a) : sys.set_name(a),
            '--profile'  :  lambda(a) : sys.set_profile(a),
-           '--profiles' :  lambda(a) : sys.set_profile(a), # alias
            '--kopts'    :  lambda(a) : sys.set_kernel_options(a)
         }
         on_ok = lambda: self.api.systems().add(sys)
@@ -294,7 +294,7 @@ def main():
 
     # verify syck isn't busted (old syck bindings were)
     if not hasattr(syck,"dump"):
-        raise Exception("needs a more-recent PySyck")
+        raise Exception("needs a more-recent PySyck module")
 
     if os.getuid() != 0:
         # FIXME: don't require root
diff --git a/cobbler/collection.py b/cobbler/collection.py
index 3d44510..027d815 100644
--- a/cobbler/collection.py
+++ b/cobbler/collection.py
@@ -4,6 +4,8 @@ Base class for any serializable list of things...
 Michael DeHaan <mdehaan@redhat.com>
 """
 
+import exceptions
+
 import serializable
 import utils
 import msg
@@ -17,13 +19,23 @@ class Collection(serializable.Serializable):
         self.config = config
         self.clear()
 
-    def factory_produce(self):
+    def factory_produce(self,config,seed_data):
+        """
+        Must override in subclass.  Factory_produce returns an Item object
+        from datastructure seed_data
+        """
         raise exceptions.NotImplementedError
 
     def filename(self):
+        """
+        Must override in subclass.  See Serializable
+        """
         raise exceptions.NotImplementedError
 
     def clear(self):
+        """
+        Forget about objects in the collection.
+        """
         if utils.app_debug:
             print "Collection::clear"
         self.listing = {}
@@ -53,9 +65,9 @@ class Collection(serializable.Serializable):
         if utils.app_debug:
             print "Collection::from_datastruct(%s)" % datastruct
         if datastruct is None:
-            print "DEBUG: from_datastruct -> None, skipping"
+            if utils.app_debug:
+                print "DEBUG: from_datastruct -> None, skipping"
             return
-	print "DEBUG: from_datastruct: %s" % datastruct
         for seed_data in datastruct:
             item = self.factory_produce(self.config,seed_data)
             self.add(item)
diff --git a/cobbler/collection_distros.py b/cobbler/collection_distros.py
index e700727..be024fc 100644
--- a/cobbler/collection_distros.py
+++ b/cobbler/collection_distros.py
@@ -1,19 +1,26 @@
+"""
+A distro represents a network bootable matched set of kernels
+and initrd files
+
+Michael DeHaan <mdehaan@redhat.com
+"""
 
 import utils
 import collection
 import item_distro as distro
-import collection_profiles as profiles
 
-"""
-A distro represents a network bootable matched set of kernels
-and initrd files
-"""
 class Distros(collection.Collection):
 
     def factory_produce(self,config,seed_data):
+        """
+        Return a Distro forged from seed_data
+        """
         return distro.Distro(config).from_datastruct(seed_data)
 
     def filename(self):
+        """
+        Config file for distro serialization
+        """
         return "/var/lib/cobbler/distros"
 
     def remove(self,name):
@@ -21,7 +28,7 @@ class Distros(collection.Collection):
         Remove element named 'name' from the collection
         """
         # first see if any Groups use this distro
-        for k,v in self.config.profiles().listing.items():
+        for v in self.config.profiles():
             if v.distro == name:
                utils.set_error("orphan_files")
                return False
diff --git a/cobbler/collection_systems.py b/cobbler/collection_systems.py
index abcf417..5d201bc 100644
--- a/cobbler/collection_systems.py
+++ b/cobbler/collection_systems.py
@@ -14,9 +14,15 @@ import collection
 class Systems(collection.Collection):
 
     def factory_produce(self,config,seed_data):
+        """
+        Return a system forged from seed_data
+        """
         return system.System(config).from_datastruct(seed_data)
 
     def filename(self):
+        """
+        Return a filename for System serialization
+        """
         return "/var/lib/cobbler/systems"
 
     def remove(self,name):
diff --git a/cobbler/config.py b/cobbler/config.py
index 470b57c..0627abc 100644
--- a/cobbler/config.py
+++ b/cobbler/config.py
@@ -21,8 +21,10 @@ import serializer
 class Config:
 
    def __init__(self):
-       # manage a definitive copy of all data collections with weakrefs
-       # back here so they can understand each other
+       """
+       Constructor.  Manages a definitive copy of all data collections with weakrefs
+       poiting back into the class so they can understand each other's contents
+       """
        self._distros      = distros.Distros(weakref.proxy(self))
        self._profiles     = profiles.Profiles(weakref.proxy(self))
        self._systems      = systems.Systems(weakref.proxy(self))
@@ -36,40 +38,76 @@ class Config:
        self.file_check()
 
    def distros(self):
+       """
+       Return the definitive copy of the Distros collection
+       """
        return self._distros
 
    def profiles(self):
+       """
+       Return the definitive copy of the Profiles collection
+       """
        return self._profiles
 
    def systems(self):
+       """
+       Return the definitive copy of the Systems collection
+       """
        return self._systems
 
    def settings(self):
+       """
+       Return the definitive copy of the application settings
+       """
        return self._settings
 
    def new_distro(self):
+       """
+       Create a new distro object with a backreference to this object
+       """
        return distro.Distro(weakref.proxy(self))
 
    def new_system(self):
+       """
+       Create a new system with a backreference to this object
+       """
        return system.System(weakref.proxy(self))
 
    def new_profile(self):
+       """
+       Create a new profile with a backreference to this object
+       """
        return profile.Profile(weakref.proxy(self))
 
    def clear(self):
+       """
+       Forget about all loaded configuration data
+       """
        for x in self._classes:
           x.clear()
 
    def file_check(self):
+       """
+       Serialize any files that do not yet exist.  This is useful for bringing the
+       app up to a working state on first run or if files are deleted.  See api.py
+       """
        for x in self._classes:
           if not os.path.exists(x.filename()):
               serializer.serialize(x)
 
    def serialize(self):
+       """
+       Save the object hierarchy to disk, using the filenames referenced in each object.
+       """
        for x in self._classes:
           serializer.serialize(x)
 
    def deserialize(self):
+       """
+       Load the object hierachy from disk, using the filenames referenced in each object.
+       """
        for x in self._classes:
           serializer.deserialize(x)
 
+
+
diff --git a/cobbler/item.py b/cobbler/item.py
index c2944e6..b1f5b93 100644
--- a/cobbler/item.py
+++ b/cobbler/item.py
@@ -4,17 +4,12 @@ An Item is a serializable thing that can appear in a Collection
 Michael DeHaan <mdehaan@redhat.com>
 """
 
+import exceptions
+
 import serializable
 
 class Item(serializable.Serializable):
 
-    """
-    constructor must be of format:
-    def __init__(self,seed_data)
-    where seed_data is a hash of argument_name/value pairs
-    see profile.py for example
-    """
-
     def set_name(self,name):
         """
         All objects have names, and with the exception of System
diff --git a/cobbler/item_distro.py b/cobbler/item_distro.py
index 312cd8d..aa2d463 100644
--- a/cobbler/item_distro.py
+++ b/cobbler/item_distro.py
@@ -1,5 +1,6 @@
 """
-A cobbler distribution
+A cobbler distribution.  A distribution is a kernel, and initrd, and potentially
+some kernel options.
 
 Michael DeHaan <mdehaan@redhat.com>
 """
@@ -13,16 +14,25 @@ import os
 class Distro(item.Item):
 
     def __init__(self,config):
+        """
+        Constructor.  Requires a back reference to the Config management object.
+        """
         self.config = config
         self.clear()
 
     def clear(self):
+        """
+        Reset this object.
+        """
         self.name = None
         self.kernel = None
         self.initrd = None
         self.kernel_options = ""
 
     def from_datastruct(self,seed_data):
+        """
+        Modify this object to take on values in seed_data
+        """
         self.name = seed_data['name']
         self.kernel = seed_data['kernel']
         self.initrd = seed_data['initrd']
@@ -64,6 +74,9 @@ class Distro(item.Item):
         return True
 
     def to_datastruct(self):
+        """
+        Return a serializable datastructure representation of this object.
+        """
         return {
            'name': self.name,
            'kernel': self.kernel,
diff --git a/cobbler/item_profile.py b/cobbler/item_profile.py
index f84f241..4709479 100644
--- a/cobbler/item_profile.py
+++ b/cobbler/item_profile.py
@@ -1,5 +1,6 @@
 """
-A Cobbler Profile.
+A Cobbler Profile.  A profile is a reference to a distribution, possibly some kernel
+options, possibly some Xen options, and some kickstart data.
 
 Michael DeHaan <mdehaan@redhat.com>
 """
@@ -11,10 +12,16 @@ from msg import *
 class Profile(item.Item):
 
     def __init__(self,config):
+        """
+        Constructor.  Requires a backreference to Config.
+        """
         self.config = config
         self.clear()
 
     def clear(self):
+        """
+        Reset this object.
+        """
         self.name = None
         self.distro = None # a name, not a reference
         self.kickstart = None
@@ -26,6 +33,9 @@ class Profile(item.Item):
         self.xen_paravirt = True
 
     def from_datastruct(self,seed_data):
+        """
+        Load this object's properties based on seed_data
+        """
         self.name            = seed_data['name']
         self.distro          = seed_data['distro']
         self.kickstart       = seed_data['kickstart']
@@ -82,7 +92,7 @@ class Profile(item.Item):
 	For Xen only.
 	Specifies the size of the Xen image in gigabytes.  xen-net-install
 	may contain some logic to ignore 'illogical' values of this size,
-	though there are no guarantees.  0 tells xen-net-install to just
+	though there are no guarantees.  0 tells koan to just
 	let it pick a semi-reasonable size.  When in doubt, specify the
 	size you want.
 	"""
@@ -147,6 +157,9 @@ class Profile(item.Item):
         return True
 
     def to_datastruct(self):
+        """
+        Return hash representation for the serializer
+        """
         return {
             'name' : self.name,
             'distro' : self.distro,
@@ -160,6 +173,9 @@ class Profile(item.Item):
         }
 
     def printable(self):
+        """
+        A human readable representaton
+        """
         buf = ""
         buf = buf + "profile         : %s\n" % self.name
         buf = buf + "distro          : %s\n" % self.distro
@@ -171,3 +187,4 @@ class Profile(item.Item):
         buf = buf + "xen mac         : %s" % self.xen_mac
         buf = buf + "xen paravirt    : %s" % self.xen_paravirt
         return buf
+
diff --git a/cobbler/item_system.py b/cobbler/item_system.py
index 22ea4b3..0819781 100644
--- a/cobbler/item_system.py
+++ b/cobbler/item_system.py
@@ -31,7 +31,7 @@ class System(item.Item):
         See utils.py
         """
         new_name = utils.find_system_identifier(name)
-        if new_name is None or new_name == False:
+        if not new_name:
             utils.set_error("bad_sys_name")
             return False
         self.name = name  # we check it add time, but store the original value.
diff --git a/cobbler/serializable.py b/cobbler/serializable.py
index 0a23283..a55eae9 100644
--- a/cobbler/serializable.py
+++ b/cobbler/serializable.py
@@ -1,9 +1,12 @@
 """
-Serializable interface, for documentation purposes
+Serializable interface, for documentation purposes.
+Collections and Settings both support this interface.
 
 Michael DeHaan <mdehaan@redhat.com>
 """
 
+import exceptions
+
 class Serializable:
 
    def filename(self):
diff --git a/cobbler/serializer.py b/cobbler/serializer.py
index e2c235a..da4c684 100644
--- a/cobbler/serializer.py
+++ b/cobbler/serializer.py
@@ -1,7 +1,5 @@
 # Michael DeHaan <mdehaan@redhat.com>
 
-import api
-import utils
 import syck  # PySyck 0.61 or greater, not syck-python 0.55
 import msg
 
diff --git a/cobbler/settings.py b/cobbler/settings.py
index bbdf3e9..48226a0 100644
--- a/cobbler/settings.py
+++ b/cobbler/settings.py
@@ -10,12 +10,21 @@ import utils
 class Settings(serializable.Serializable):
 
    def filename(self):
+       """
+       The filename where settings are serialized.
+       """
        return "/var/lib/cobbler/settings"
 
    def __init__(self):
+       """
+       Constructor.
+       """
        self.clear()
 
    def clear(self):
+       """
+       Reset this object to reasonable default values.
+       """
        self._attributes = {
           "httpd_bin"      : "/usr/sbin/httpd",
           "pxelinux"       : "/usr/lib/syslinux/pxelinux.0",
@@ -30,19 +39,24 @@ class Settings(serializable.Serializable):
 
 
    def to_datastruct(self):
+       """
+       Return an easily serializable representation of the config.
+       """
        return self._attributes
 
    def from_datastruct(self,datastruct):
+       """
+       Modify this object to load values in datastruct.
+       """
        if datastruct is None:
-          print "DEBUG: not loading empty structure"
+          print "warning: not loading empty structure for %s" % self.filename()
           return
        self._attributes = datastruct
        return self
 
-   # could use getatr, but I'd rather not.
    def __getattr__(self,name):
        if utils.app_debug:
-           print "Settings::__getattr__(self,%s)" % name       
+           print "Settings::__getattr__(self,%s)" % name
        if self._attributes.has_key(name):
            return self._attributes[name]
        else:
diff --git a/cobbler/utils.py b/cobbler/utils.py
index 90354c6..341bef5 100644
--- a/cobbler/utils.py
+++ b/cobbler/utils.py
@@ -12,17 +12,18 @@ import subprocess
 
 import msg
 
-app_debug =  True # essentially an app level global, but the only one
+app_debug = False # essentially an app level global, but the only one
 
 _re_kernel = re.compile(r'vmlinuz-(\d+)\.(\d+)\.(\d+)-(.*)')
 _re_initrd = re.compile(r'initrd-(\d+)\.(\d+)\.(\d+)-(.*).img')
-_last_error = ""
+utils_last_error = ""
 
 def last_error():
-    return _last_error
+    return utils_last_error
 
 def set_error(strmsg):
-    _last_error = msg.m(strmsg)
+    global utils_last_error
+    utils_last_error = msg.m(strmsg)
 
 def get_host_ip(ip):
     handle = subprocess.Popen("/usr/bin/gethostip %s" % ip, shell=True, stdout=subprocess.PIPE)