Source fully documented

6 files changed, 439 insertions, 117 deletions

diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/Apt.py b/src/lib/Bcfg2/Server/Plugins/Packages/Apt.py
index 1c48fb40b..30be4b089 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/Apt.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/Apt.py
@@ -24,7 +24,8 @@ class AptSource(Source):
     basegroups = ['apt', 'debian', 'ubuntu', 'nexenta']
     ptype = 'deb'
 
-    def get_urls(self):
+    @property
+    def urls(self):
         if not self.rawurl:
             rv = []
             for part in self.components:
@@ -34,7 +35,6 @@ class AptSource(Source):
             return rv
         else:
             return ["%sPackages.gz" % self.rawurl]
-    urls = property(get_urls)
 
     def read_files(self):
         bdeps = dict()
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/Collection.py b/src/lib/Bcfg2/Server/Plugins/Packages/Collection.py
index 033eb2fc8..5074cc389 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/Collection.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/Collection.py
@@ -15,19 +15,27 @@ Overriding Methods
 As noted above, the ``_Collection`` object is written expressly so
 that you can subclass it and override no methods or attributes, and it
 will work by deferring all calls to the Source objects it contains.
-If you do choose to override methods, there are two approaches:
+There are thus three approaches to writing a ``_Collection`` subclass:
+
+#. Keep the superclass almost entirely intact and defer to the
+   ``Source`` objects inside it. For an example of this kind of
+   ``_Collection`` object, see
+   :mod:`Bcfg2.Server.Plugins.Packages.Apt`.
 
 #. Keep :func:`_Collection.complete` intact, and override the methods
    it calls: :func:`_Collection.is_package`,
    :func:`_Collection.is_virtual_package`,
    :func:`_Collection.get_deps`, :func:`_Collection.get_provides`,
    :func:`_Collection.get_vpkgs`, and :func:`_Collection.setup_data`.
+   There are no examples of this kind of ``_Collection`` subclass yet.
 
 #. Provide your own implementation of :func:`_Collection.complete`, in
    which case you do not have to override the above methods.  You may
    want to override :func:`_Collection.packages_from_entry`,
    :func:`_Collection.packages_to_entry`, and
-   :func:`_Collection.get_new_packages`.
+   :func:`_Collection.get_new_packages`.  For an example of this kind
+   of ``_Collection`` object, see
+   :mod:`Bcfg2.Server.Plugins.Packages.yum`.
 
 In either case, you may want to override
 :func:`_Collection.get_groups`, :func:`_Collection.get_group`,
@@ -70,6 +78,7 @@ import logging
 import lxml.etree
 import Bcfg2.Server.Plugin
 from Bcfg2.Compat import any, md5
+from Bcfg2.Server.Plugins.Packages.Source import Source
 
 LOGGER = logging.getLogger(__name__)
 
@@ -201,17 +210,15 @@ class _Collection(list, Bcfg2.Server.Plugin.Debuggable):
         return "\n".join(srcs)
 
     def get_relevant_groups(self):
-        """ Get all groups that might be relevant to determining which
-        sources apply to this collection's client.
-
-        The base implementation simply aggregates the results of
-        :func:`Bcfg2.Server.Plugins.Packages.Source.Source.get_relevant_groups`.
-
-        :return: list of strings - group names"""
         groups = []
         for source in self:
             groups.extend(source.get_relevant_groups(self.metadata))
         return sorted(list(set(groups)))
+    get_relevant_groups.__doc__ = Source.get_relevant_groups.__doc__ + """
+
+        The base implementation simply aggregates the results of
+        :func:`Bcfg2.Server.Plugins.Packages.Source.Source.get_relevant_groups`.
+        """
 
     @property
     def basegroups(self):
@@ -228,11 +235,12 @@ class _Collection(list, Bcfg2.Server.Plugin.Debuggable):
 
     @property
     def cachefiles(self):
-        """ Geta list of the full path to all cachefiles used by this
+        """ A list of the full path to all cachefiles used by this
         collection.
 
-        The base implementation simply aggregates the results of
-        :attr:`Bcfg2.Server.Plugins.Packages.Source.Source.cachefiles`."""
+        The base implementation simply aggregates
+        :attr:`Bcfg2.Server.Plugins.Packages.Source.Source.cachefile`
+        attributes."""
         cachefiles = set([self.cachepath])
         for source in self:
             cachefiles.add(source.cachefile)
@@ -245,7 +253,7 @@ class _Collection(list, Bcfg2.Server.Plugin.Debuggable):
         once faster than serially.
 
         The base implementation simply aggregates the results of
-        :func:`Bcfg2.Server.Plugins.Packages.Source.Source.get_groups`.
+        :func:`Bcfg2.Server.Plugins.Packages.Source.Source.get_group`.
 
         :param grouplist: The list of groups to query
         :type grouplist: list of strings - group names
@@ -333,8 +341,9 @@ class _Collection(list, Bcfg2.Server.Plugin.Debuggable):
     def get_essential(self):
         """ Get a list of packages that are essential to the repository.
 
-        The base implementation simply aggregates the results of
-        :func:`Bcfg2.Server.Plugins.Packages.Source.Source.get_essential`.
+        The base implementation simply aggregates
+        :attr:`Bcfg2.Server.Plugins.Packages.Source.Source.essentialpkgs`
+        attributes
 
         :returns: list of strings, but see :ref:`pkg-objects`
         """
@@ -382,6 +391,9 @@ class _Collection(list, Bcfg2.Server.Plugin.Debuggable):
         the list of unknown packages but should not be presented to
         the user.  E.g., packages that you expect to be unknown.
 
+        The base implementation filters out packages that are expected
+        to be unknown by any source in this collection.
+
         :param unknown: A set of unknown packages.  The set should be
                         modified in place.
         :type unknown: set of strings, but see :ref:`pkg-objects`
@@ -395,8 +407,9 @@ class _Collection(list, Bcfg2.Server.Plugin.Debuggable):
         the magic groups for any of the sources contained in this
         Collection.
 
-        The base implementation simply aggregates the results of
-        :func:`Bcfg2.Server.Plugins.Packages.Source.Source.magic_groups_match`.
+        The base implementation returns True if any source
+        :func:`Bcfg2.Server.Plugins.Packages.Source.Source.magic_groups_match`
+        returns True.
 
         :returns: bool
         """
@@ -419,8 +432,9 @@ class _Collection(list, Bcfg2.Server.Plugin.Debuggable):
         :func:`Bcfg2.Server.Plugins.Packages.Packages.get_additional_data`
         (and thence to client metadata objects).
 
-        The base implementation simply aggregates the results of
-        :func:`Bcfg2.Server.Plugins.Packages.Source.Source.get_additional_data`
+        The base implementation simply aggregates
+        :attr:`Bcfg2.Server.Plugins.Packages.Source.Source.url_map`
+        attributes.
 
         :returns: list of additional Connector data
         """
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/Pac.py b/src/lib/Bcfg2/Server/Plugins/Packages/Pac.py
index 2418e6f2b..15fb62431 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/Pac.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/Pac.py
@@ -11,7 +11,8 @@ class PacSource(Source):
     basegroups = ['arch', 'parabola']
     ptype = 'pacman'
 
-    def get_urls(self):
+    @property
+    def urls(self):
         if not self.rawurl:
             rv = []
             for part in self.components:
@@ -21,7 +22,6 @@ class PacSource(Source):
             return rv
         else:
             raise Exception("PacSource : RAWUrl not supported (yet)")
-    urls = property(get_urls)
 
     def read_files(self):
         bdeps = dict()
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/Source.py b/src/lib/Bcfg2/Server/Plugins/Packages/Source.py
index b57d1b0cc..d73a942c4 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/Source.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/Source.py
@@ -25,10 +25,9 @@ If you are using the stock (or a near-stock)
 then you will need to implement the following methods and attributes
 in your ``Source`` subclass:
 
-* :func:`Source.get_urls`
+* :func:`Source.urls`
 * :func:`Source.read_files`
 * :attr:`Source.basegroups`
-* :attr:`Source.ptype`
 
 Additionally, you may want to consider overriding the following
 methods and attributes:
@@ -39,9 +38,13 @@ methods and attributes:
 * :attr:`Source.load_state`
 * :attr:`Source.save_state`
 
+For an example of this kind of ``Source`` object, see
+:mod:`Bcfg2.Server.Plugins.Packages.Apt`.
+
 If you are overriding the ``_Collection`` object in more depth, then
 you have more leeway in what you might want to override or implement
-in your ``Source`` subclass.
+in your ``Source`` subclass.  For an example of this kind of
+``Source`` object, see :mod:`Bcfg2.Server.Plugins.Packages.Yum`.
 """
 
 import os
@@ -79,32 +82,94 @@ class SourceInitError(Exception):
     pass
 
 
+#: A regular expression used to determine the base name of a repo from
+#: its URL.  This is used when generating repo configs and by
+#: :func:`Source.get_repo_name`.  It handles `Pulp
+#: <http://www.pulpproject.org/>`_ and `mrepo
+#: <http://dag.wieers.com/home-made/mrepo/>`_ repositories specially,
+#: and otherwise grabs the last component of the URL (as delimited by
+#: slashes).
+REPO_RE = re.compile(r'(?:pulp/repos/|/RPMS\.|/)([^/]+)/?$')
+
+
 class Source(Bcfg2.Server.Plugin.Debuggable):
-    mrepo_re = re.compile(r'/RPMS\.([^/]+)')
-    pulprepo_re = re.compile(r'pulp/repos/([^/]+)')
-    genericrepo_re = re.compile('https?://.*?/([^/]+)/?$')
+    """ ``Source`` objects represent a single <Source> tag in
+    ``sources.xml``.  Note that a single Source tag can itself
+    describe multiple repositories (if it uses the "url" attribute
+    instead of "rawurl"), and so can the ``Source`` object.
+
+    Note that a number of the attributes of this object may be more or
+    less specific to one backend (e.g., :attr:`essentialpkgs`,
+    :attr:`recommended`, :attr:`gpgkeys`, but they are included in the
+    superclass to make the parsing of sources from XML more
+    consistent, and to make it trivial for other backends to support
+    those features.
+    """
+
+    #: The list of
+    #: :ref:`server-plugins-generators-packages-magic-groups` that
+    #: make sources of this type available to clients.
     basegroups = []
+
+    #: A predicate that is used by :func:`filter_unknown` to filter
+    #: packages from the results of
+    #: :func:`Bcfg2.Server.Plugins.Packages.Collection._Collection.complete`
+    #: that should not be shown to the end user (i.e., that are not
+    #: truly unknown, but are rather packaging system artifacts).  By
+    #: default, excludes any package whose name starts with "choice"
     unknown_filter = lambda p: p.startswith("choice")
 
     def __init__(self, basepath, xsource, setup):
+        """
+        :param basepath: The base filesystem path under which cache
+                         data for this source should be stored
+        :type basepath: string
+        :param xsource: The XML tag that describes this source
+        :type source: lxml.etree._Element
+        :param setup: A Bcfg2 options dict
+        :type setup: dict
+        :raises: :class:`Bcfg2.Server.Plugins.Packages.Source.SourceInitError`
+        """
         Bcfg2.Server.Plugin.Debuggable.__init__(self)
+
+        #: The base filesystem path under which cache data for this
+        #: source should be stored
         self.basepath = basepath
+
+        #: The XML tag that describes this source
         self.xsource = xsource
+
+        #: A Bcfg2 options dict
         self.setup = setup
-        self.essentialpkgs = set()
-        self.pkgnames = set()
 
-        try:
-            self.version = xsource.find('Version').text
-        except AttributeError:
-            self.version = None
+        #: A set of package names that are deemed "essential" by this
+        #: source
+        self.essentialpkgs = set()
 
+        #: A list of the text of all 'Component' attributes of this
+        #: source from XML
         self.components = [item.text for item in xsource.findall('Component')]
+
+        #: A list of the arches supported by this source
         self.arches = [item.text for item in xsource.findall('Arch')]
+
+        #: A list of the the names of packages that are blacklisted
+        #: from this source
         self.blacklist = [item.text for item in xsource.findall('Blacklist')]
+
+        #: A list of the the names of packages that are whitelisted in
+        #: this source
         self.whitelist = [item.text for item in xsource.findall('Whitelist')]
 
+        #: A dict of repository options that will be included in the
+        #: configuration generated on the server side (if such is
+        #: applicable; most backends do not generate any sort of
+        #: repository configuration on the Bcfg2 server)
         self.server_options = dict()
+
+        #: A dict of repository options that will be included in the
+        #: configuration generated for the client (if that is
+        #: supported by the backend)
         self.client_options = dict()
         opts = xsource.findall("Options")
         for el in opts:
@@ -116,24 +181,45 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
             if el.get("serveronly", "false").lower() == "false":
                 self.client_options.update(repoopts)
 
+        #: A list of URLs to GPG keys that apply to this source
         self.gpgkeys = [el.text for el in xsource.findall("GPGKey")]
 
+        #: Whether or not to include essential packages from this source
         self.essential = xsource.get('essential', 'true').lower() == 'true'
+
+        #: Whether or not to include recommended packages from this source
         self.recommended = xsource.get('recommended',
                                        'false').lower() == 'true'
 
+        #: The "rawurl" attribute from :attr:`xsource`, if applicable.
+        #: A trailing slash is automatically appended to this if there
+        #: wasn't one already present.
         self.rawurl = xsource.get('rawurl', '')
         if self.rawurl and not self.rawurl.endswith("/"):
             self.rawurl += "/"
+
+        #: The "url" attribute from :attr:`xsource`, if applicable.  A
+        #: trailing slash is automatically appended to this if there
+        #: wasn't one already present.
         self.url = xsource.get('url', '')
         if self.url and not self.url.endswith("/"):
             self.url += "/"
+
+        #: The "version" attribute from :attr:`xsource`
         self.version = xsource.get('version', '')
 
-        # build the set of conditions to see if this source applies to
-        # a given set of metadata
+        #: A list of predicates that are used to determine if this
+        #: source applies to a given
+        #: :class:`Bcfg2.Server.Plugins.Metadata.ClientMetadata`
+        #: object.
         self.conditions = []
-        self.groups = []  # provided for some limited backwards compat
+        #: Formerly, :ref:`server-plugins-generators-packages` only
+        #: supported applying package sources to groups; that is, they
+        #: could not be assigned by more complicated logic like
+        #: per-client repositories and group or client negation.  This
+        #: attribute attempts to provide for some limited backwards
+        #: compat with older code that relies on this.
+        self.groups = []
         for el in xsource.iterancestors():
             if el.tag == "Group":
                 if el.get("negate", "false").lower() == "true":
@@ -151,15 +237,44 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
                     self.conditions.append(lambda m, el=el:
                                            el.get("name") == m.hostname)
 
+        #: A set of all package names in this source.  This will not
+        #: necessarily be populated, particularly by backends that
+        #: reimplement large portions of
+        #: :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
+        self.pkgnames = set()
+
+        #: A dict of ``<package name>`` -> ``<list of dependencies>``.
+        #: This will not necessarily be populated, particularly by
+        #: backends that reimplement large portions of
+        #: :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
         self.deps = dict()
+
+        #: A dict of ``<package name>`` -> ``<list of provided
+        #: symbols>``.  This will not necessarily be populated,
+        #: particularly by backends that reimplement large portions of
+        #: :class:`Bcfg2.Server.Plugins.Packages.Collection._Collection`
         self.provides = dict()
 
+        #: The file (or directory) used for this source's cache data
         self.cachefile = os.path.join(self.basepath,
                                       "cache-%s" % self.cachekey)
         if not self.rawurl:
-            self.baseurl = self.url + "%(version)s/%(component)s/%(arch)s/"
+            baseurl = self.url + "%(version)s/%(component)s/%(arch)s/"
         else:
-            self.baseurl = self.rawurl
+            baseurl = self.rawurl
+
+        #: A list of dicts, each of which describes the URL to one
+        #: repository contained in this source.  Each dict contains
+        #: the following keys:
+        #:
+        #: * ``version``: The version of the repo (``None`` for
+        #:   ``rawurl`` repos)
+        #: * ``component``: The component use to form this URL
+        #:   (``None`` for ``rawurl`` repos)
+        #: * ``arch``: The architecture of this repo
+        #: * ``baseurl``: Either the ``rawurl`` attribute, or the
+        #:   format string built from the ``url`` attribute
+        #: * ``url``: The actual URL to the repository
         self.url_map = []
         for arch in self.arches:
             if self.url:
@@ -175,87 +290,166 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
                     setting['baseurl'] = self.url
                 else:
                     setting['baseurl'] = self.rawurl
-                setting['url'] = self.baseurl % setting
+                setting['url'] = baseurl % setting
             self.url_map.extend(usettings)
 
     @property
     def cachekey(self):
+        """ A unique key for this source that will be used to generate
+        :attr:`cachefile` and other cache paths """
         return md5(cPickle.dumps([self.version, self.components, self.url,
                                   self.rawurl, self.arches])).hexdigest()
 
     def get_relevant_groups(self, metadata):
+        """ Get all groups that might be relevant to determining which
+        sources apply to this collection's client.
+
+        :return: list of strings - group names"""
         return sorted(list(set([g for g in metadata.groups
                                 if (g in self.basegroups or
                                     g in self.groups or
                                     g in self.arches)])))
 
     def load_state(self):
+        """ Load saved state from :attr:`cachefile`.  If caching and
+        state is handled by the package library, then this function
+        does not need to be implemented.
+
+        :raises: OSError - If the saved data cannot be read
+        :raises: cPickle.UnpicklingError - If the saved data is corrupt """
         data = open(self.cachefile)
         (self.pkgnames, self.deps, self.provides,
          self.essentialpkgs) = cPickle.load(data)
 
     def save_state(self):
+        """ Save state to :attr:`cachefile`.  If caching and
+        state is handled by the package library, then this function
+        does not need to be implemented. """
         cache = open(self.cachefile, 'wb')
         cPickle.dump((self.pkgnames, self.deps, self.provides,
                       self.essentialpkgs), cache, 2)
         cache.close()
 
     def setup_data(self, force_update=False):
-        should_read = True
-        should_download = False
-        if os.path.exists(self.cachefile):
-            try:
-                self.load_state()
-                should_read = False
-            except:
-                self.logger.error("Packages: Cachefile %s load failed; "
-                                  "falling back to file read" % self.cachefile)
-        if should_read:
-            try:
-                self.read_files()
-            except:
-                self.logger.error("Packages: File read failed; "
-                                  "falling back to file download")
-                should_download = True
-
-        if should_download or force_update:
+        """ Perform all data fetching and setup tasks.  For most
+        backends, this involves downloading all metadata from the
+        repository, parsing it, and caching the parsed data locally.
+        The order of operations is:
+
+        #. Call :func:`load_state` to try to load data from the local
+           cache.
+        #. If that fails, call :func:`read_files` to read and parse
+           the locally downloaded metadata files.
+        #. If that fails, call :func:`update` to fetch the metadata,
+           then :func:`read_files` to parse it.
+
+        Obviously with a backend that leverages repo access libraries
+        to avoid downloading all metadata, many of the functions
+        called by ``setup_data`` can be no-ops (or nearly so).
+
+        :param force_update: Ignore all locally cached and downloaded
+                             data and fetch the metadata anew from the
+                             upstream repository.
+        :type force_update: bool
+        """
+        if not force_update:
+            if os.path.exists(self.cachefile):
+                try:
+                    self.load_state()
+                except:
+                    err = sys.exc_info()[1]
+                    self.logger.error("Packages: Cachefile %s load failed: %s" %
+                                      (self.cachefile, err))
+                    self.logger.error("Falling back to file read")
+
+                    try:
+                        self.read_files()
+                    except:
+                        err = sys.exc_info()[1]
+                        self.logger.error("Packages: File read failed: %s" %
+                                          err)
+                        self.logger.error("Falling back to file download")
+                        force_update = True
+
+        if force_update:
             try:
                 self.update()
                 self.read_files()
             except:
-                self.logger.error("Packages: Failed to load data for Source "
-                                  "of %s. Some Packages will be missing." %
-                                  self.urls)
+                err = sys.exc_info()[1]
+                self.logger.error("Packages: Failed to load data for %s: %s" %
+                                  (self, err))
+                self.logger.error("Some Packages will be missing")
 
     def get_repo_name(self, url_map):
-        # try to find a sensible name for a repo
+        """ Try to find a sensible name for a repository. Since
+        ``sources.xml`` doesn't provide for repository names, we have
+        to try to guess at the names when generating config files or
+        doing other operations that require repository names.  This
+        function tries several approaches:
+
+        #. First, if the map contains a ``component`` key, use that as
+           the name.
+
+        #. If not, then try to match the repository URL against
+           :attr:`Bcfg2.Server.Plugins.Packages.Source.REPO_RE`.  If
+           that succeeds, use the first matched group; additionally,
+           if the Source tag that describes this repo is contained in
+           a ``<Group>`` tag, prepend that to the name.
+
+        #. If :attr:`Bcfg2.Server.Plugins.Packages.Source.REPO_RE`
+           does not match the repository, and the Source tag that
+           describes this repo is contained in a ``<Group>`` tag, use
+           the name of the group.
+
+        #. Failing that, use the full URL to this repository, with the
+           protocol and trailing slash stripped off if possible.
+
+        Once that is done, all characters disallowed in yum source
+        names are replaced by dashes.  See below for the exact regex.
+        The yum rules are used here because they are so restrictive.
+
+        ``get_repo_name`` is **not** guaranteed to return a unique
+        name.  If you require a unique name, then you will need to
+        generate all repo names and make them unique through the
+        approach of your choice, e.g., appending numbers to non-unique
+        repository names.  See
+        :func:`Bcfg2.Server.Plugins.Packages.Yum.Source.get_repo_name`
+        for an example.
+
+        :param url_map: A single :attr:`url_map` dict, i.e., any
+                        single element of :attr:`url_map`.
+        :type url_map: dict
+        :returns: string - the name of the repository.
+        """
         if url_map['component']:
             rname = url_map['component']
         else:
-            name = None
-            for repo_re in (self.mrepo_re,
-                            self.pulprepo_re,
-                            self.genericrepo_re):
-                match = repo_re.search(url_map['url'])
-                if match:
-                    name = match.group(1)
-                    break
-            if name and self.groups:
-                rname = "%s-%s" % (self.groups[0], name)
+            match = REPO_RE.search(url_map['url'])
+            if match:
+                rname = match.group(1)
+                if self.groups:
+                    rname = "%s-%s" % (self.groups[0], rname)
             elif self.groups:
                 rname = self.groups[0]
             else:
-                # a global source with no reasonable name.  just use
-                # the full url and let the regex below make it even
-                # uglier.
-                rname = url_map['url']
+                # a global source with no reasonable name.  Try to
+                # strip off the protocol and trailing slash.
+                match = re.search(r'^[A-z]://(.*?)/?', url_map['url'])
+                if match:
+                    rname = match.group(1)
+                else:
+                    # what kind of crazy url is this?  I give up!
+                    # just use the full url and let the regex below
+                    # make it even uglier.
+                    rname = url_map['url']
         # see yum/__init__.py in the yum source, lines 441-449, for
         # the source of this regex.  yum doesn't like anything but
         # string.ascii_letters, string.digits, and [-_.:].  There
         # doesn't seem to be a reason for this, because yum.
         return re.sub(r'[^A-Za-z0-9-_.:]', '-', rname)
 
-    def __str__(self):
+    def __repr__(self):
         if self.rawurl:
             return "%s at %s" % (self.__class__.__name__, self.rawurl)
         elif self.url:
@@ -263,18 +457,23 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
         else:
             return self.__class__.__name__
 
-    def __repr__(self):
-        return str(self)
-
-    def get_urls(self):
+    @property
+    def urls(self):
+        """ A list of URLs to the base metadata file for each
+        repository described by this source. """
         return []
-    urls = property(get_urls)
 
-    def get_files(self):
+    @property
+    def files(self):
+        """ A list of files stored in the local cache by this backend.
+        """
         return [self.escape_url(url) for url in self.urls]
-    files = property(get_files)
 
     def get_vpkgs(self, metadata):
+        """ Get a list of all virtual packages provided by all sources.
+
+        :returns: list of strings
+        """
         agroups = ['global'] + [a for a in self.arches
                                 if a in metadata.groups]
         vdict = dict()
@@ -291,18 +490,52 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
         return vdict
 
     def is_virtual_package(self, metadata, package):
-        """ called to determine if a package is a virtual package.
-        this is only invoked if the package is not listed in the dict
-        returned by get_vpkgs """
+        """ Return True if a name is a virtual package (i.e., is a
+        symbol provided by a real package), False otherwise.
+
+        :param package: The name of the symbol, but see :ref:`pkg-objects`
+        :type package: string
+        :returns: bool
+        """
         return False
 
     def escape_url(self, url):
+        """ Given a URL to a repository metadata file, return the full
+        path to a file suitable for storing that file locally.  This
+        is acheived by replacing all forward slashes in the URL with
+        ``@``.
+
+        :param url: The URL to escape
+        :type url: string
+        :returns: string
+        """
         return os.path.join(self.basepath, url.replace('/', '@'))
 
     def read_files(self):
+        """ Read and parse locally downloaded metadata files.  This
+        function should call :func:`process_files` as its final
+        step. This should also populate :attr:`pkgnames`. """
         pass
 
     def process_files(self, deps, prov):
+        """ Given dicts of depends and provides generated by
+        :func:`read_files`, this generates :attr:`deps` and
+        :attr:`provides` and calls :func:`save_state` to save the
+        cached data to disk.
+
+        Both arguments are dicts of dicts of lists.  Keys are the
+        arches of packages contained in this source; values are dicts
+        whose keys are package names and values are lists of either
+        dependencies for each package the symbols provided by each
+        package.
+
+        :param deps: A dict of dependencies found in the metadata for
+                     this source.
+        :type deps: dict; see above.
+        :param prov: A dict of symbols provided by packages in this
+                     repository.
+        :type prov: dict; see above.
+        """
         self.deps['global'] = dict()
         self.provides['global'] = dict()
         for barch in deps:
@@ -336,19 +569,41 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
         self.save_state()
 
     def filter_unknown(self, unknown):
+        """ After :func:`complete`, filter out packages that appear in
+        the list of unknown packages but should not be presented to
+        the user.  :attr:`unknown_filter` is called to assess whether
+        or not a package is expected to be unknown.
+
+        :param unknown: A set of unknown packages.  The set should be
+                        modified in place.
+        :type unknown: set of strings
+        """
         unknown.difference_update(set([u for u in unknown
                                        if self.unknown_filter(u)]))
 
     def update(self):
+        """ Download metadata from the upstream repository and cache
+        it locally.
+
+        :raises: ValueError - If any URL in :attr:`urls` is malformed
+        :raises: OSError - If there is an error writing the local
+                 cache
+        :raises: HTTPError - If there is an error fetching the remote
+                 data
+        """
         for url in self.urls:
             self.logger.info("Packages: Updating %s" % url)
             fname = self.escape_url(url)
             try:
-                data = fetch_url(url)
-                open(fname, 'w').write(data)
+                open(fname, 'w').write(fetch_url(url))
             except ValueError:
                 self.logger.error("Packages: Bad url string %s" % url)
                 raise
+            except OSError:
+                err = sys.exc_info()[1]
+                self.logger.error("Packages: Could not write data from %s to "
+                                  "local cache at %s: %s" % (url, fname, err))
+                raise
             except HTTPError:
                 err = sys.exc_info()[1]
                 self.logger.error("Packages: Failed to fetch url %s. HTTP "
@@ -356,6 +611,15 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
                 raise
 
     def applies(self, metadata):
+        """ Return true if this source applies to the given client,
+        i.e., the client is in all necessary groups and
+        :ref:`server-plugins-generators-packages-magic-groups`.
+
+        :param metadata: The client metadata to check to see if this
+                         source applies
+        :type metadata: Bcfg2.Server.Plugins.Metadata.ClientMetadata
+        :returns: bool
+        """
         # check base groups
         if not self.magic_groups_match(metadata):
             return False
@@ -368,36 +632,80 @@ class Source(Bcfg2.Server.Plugin.Debuggable):
         return True
 
     def get_arches(self, metadata):
+        """ Get a list of architectures that the given client has and
+        for which this source provides packages for.  The return value
+        will always include ``global``.
+
+        :param metadata: The client metadata to get matching
+                         architectures for
+        :type metadata: Bcfg2.Server.Plugins.Metadata.ClientMetadata
+        :returns: list of strings
+        """
         return ['global'] + [a for a in self.arches if a in metadata.groups]
 
-    def get_deps(self, metadata, pkgname):
-        for arch in self.get_arches(metadata):
-            if pkgname in self.deps[arch]:
-                return self.deps[arch][pkgname]
-        return []
+    def get_deps(self, metadata, package):
+        """ Get a list of the dependencies of the given package.
 
-    def get_provides(self, metadata, required):
+        :param package: The name of the symbol
+        :type package: string
+        :returns: list of strings
+        """
         for arch in self.get_arches(metadata):
-            if required in self.provides[arch]:
-                return self.provides[arch][required]
+            if package in self.deps[arch]:
+                return self.deps[arch][package]
         return []
 
-    def is_package(self, metadata, pkg):
-        return (pkg in self.pkgnames and
-                pkg not in self.blacklist and
-                (len(self.whitelist) == 0 or pkg in self.whitelist))
+    def get_provides(self, metadata, package):
+        """ Get a list of all symbols provided by the given package.
 
-    def get_package(self, metadata, package):
-        return package
+        :param package: The name of the package
+        :type package: string
+        :returns: list of strings
+        """
+        for arch in self.get_arches(metadata):
+            if package in self.provides[arch]:
+                return self.provides[arch][package]
+        return []
 
-    def get_group(self, metadata, group, ptype=None):
+    def is_package(self, metadata, package):  # pylint: disable=W0613
+        """ Return True if a package is a package, False otherwise.
+
+        The base implementation returns True if any Source object's
+        :func:`Bcfg2.Server.Plugins.Packages.Source.Source.is_package`
+        returns True.
+
+        :param package: The name of the package
+        :type package: string
+        :returns: bool
+        """
+        return (package in self.pkgnames and
+                package not in self.blacklist and
+                (len(self.whitelist) == 0 or package in self.whitelist))
+
+    def get_group(self, metadata, group, ptype=None):  # pylint: disable=W0613
+        """ Get the list of packages of the given type in a package
+        group.
+
+        :param group: The name of the group to query
+        :type group: string
+        :param ptype: The type of packages to get, for backends that
+                      support multiple package types in package groups
+                      (e.g., "recommended," "optional," etc.)
+        :type ptype: string
+        :returns: list of strings - package names
+        """
         return []
 
     def magic_groups_match(self, metadata):
-        """ check to see if this source applies to the given host
-        metadata by checking 'magic' (base) groups only, or if magic
-        groups are off """
-        # we always check that arch matches
+        """ Returns True if the client's
+        :ref:`server-plugins-generators-packages-magic-groups` match
+        the magic groups this source.  Also returns True if magic
+        groups are off in the configuration and the client's
+        architecture matches (i.e., architecture groups are *always*
+        checked).
+
+        :returns: bool
+        """
         found_arch = False
         for arch in self.arches:
             if arch in metadata.groups:
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/Yum.py b/src/lib/Bcfg2/Server/Plugins/Packages/Yum.py
index b8648fdde..7de8d1fb3 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/Yum.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/Yum.py
@@ -621,10 +621,10 @@ class YumSource(Source):
             (self.packages, self.deps, self.provides,
              self.filemap, self.url_map) = cPickle.load(data)
 
-    def get_urls(self):
+    @property
+    def urls(self):
         return [self._get_urls_from_repodata(m['url'], m['arch'])
                 for m in self.url_map]
-    urls = property(get_urls)
 
     def _get_urls_from_repodata(self, url, arch):
         if self.use_yum:
diff --git a/src/lib/Bcfg2/Server/Plugins/Packages/__init__.py b/src/lib/Bcfg2/Server/Plugins/Packages/__init__.py
index 40bcabae9..06b1b78c0 100644
--- a/src/lib/Bcfg2/Server/Plugins/Packages/__init__.py
+++ b/src/lib/Bcfg2/Server/Plugins/Packages/__init__.py
@@ -9,8 +9,8 @@ from Bcfg2.Compat import ConfigParser, urlopen
 from Bcfg2.Server.Plugins.Packages import Collection
 from Bcfg2.Server.Plugins.Packages.PackagesSources import PackagesSources
 
-yum_config_default = "/etc/yum.repos.d/bcfg2.repo"
-apt_config_default = "/etc/apt/sources.d/bcfg2"
+YUM_CONFIG_DEFAULT = "/etc/yum.repos.d/bcfg2.repo"
+APT_CONFIG_DEFAULT = "/etc/apt/sources.d/bcfg2"
 
 
 class Packages(Bcfg2.Server.Plugin.Plugin,
@@ -104,11 +104,11 @@ class Packages(Bcfg2.Server.Plugin.Plugin,
             if (entry.get("name") == \
                     self.core.setup.cfp.get("packages",
                                             "yum_config",
-                                            default=yum_config_default) or
+                                            default=YUM_CONFIG_DEFAULT) or
                 entry.get("name") == \
                     self.core.setup.cfp.get("packages",
                                             "apt_config",
-                                            default=apt_config_default)):
+                                            default=APT_CONFIG_DEFAULT)):
                 self.create_config(entry, metadata)
 
     def HandlesEntry(self, entry, metadata):
@@ -125,11 +125,11 @@ class Packages(Bcfg2.Server.Plugin.Plugin,
             if (entry.get("name") == \
                     self.core.setup.cfp.get("packages",
                                             "yum_config",
-                                            default=yum_config_default) or
+                                            default=YUM_CONFIG_DEFAULT) or
                 entry.get("name") == \
                     self.core.setup.cfp.get("packages",
                                             "apt_config",
-                                            default=apt_config_default)):
+                                            default=APT_CONFIG_DEFAULT)):
                 return True
         return False