Rework locating config file for checkconfig and upgrade-master.

This changes the logic of checkconfig so that,
1. If a file is passed, that file is used.
2. If a directory is passed containing `buildbot.tac`, that file is
   loaded and `configfile` is defined there, that file is used.
3. If `buildbot.tac` doesn't exist, or doesn't define `configfile`,
   `master.cfg` from that directory is used.

The logic for upgrade-master is similar, except only directories are
supported, so (1) is skipped.

master/buildbot/scripts/base.py

@@ -38,19 +38,16 @@ def print_error(error_message):
 
     return True
 
-def getConfigFileWithFallback(basedir, defaultName='master.cfg'):
-    configFile = os.path.abspath(os.path.join(basedir, defaultName))
-    if os.path.exists(configFile):
-        return configFile
+def getConfigFileFromTac(basedir):
     # execute the .tac file to see if its configfile location exists
     tacFile = os.path.join(basedir, 'buildbot.tac')
     if os.path.exists(tacFile):
         # don't mess with the global namespace
         tacGlobals = {}
         execfile(tacFile, tacGlobals)
-        return tacGlobals["configfile"]
-    # No config file found; return default location and fail elsewhere
-    return configFile
+        return tacGlobals.get("configfile", "master.cfg")
+    else:
+        return "master.cfg"
 
 class SubcommandOptions(usage.Options):
     # subclasses should set this to a list-of-lists in order to source the

master/buildbot/scripts/checkconfig.py

@@ -16,35 +16,35 @@
 import sys
 import os
 from buildbot import config
-from buildbot.scripts.base import getConfigFileWithFallback
-
-class ConfigLoader(object):
-    def __init__(self, basedir=os.getcwd(), configFileName='master.cfg'):
-        self.basedir = os.path.abspath(basedir)
-        self.configFileName = getConfigFileWithFallback(basedir, configFileName)
-
-    def load(self, quiet=False):
-        try:
-            config.MasterConfig.loadConfig(
-                    self.basedir, self.configFileName)
-        except config.ConfigErrors, e:
-            if not quiet:
-                print >> sys.stderr, "Configuration Errors:"
-                for e in e.errors:
-                    print >> sys.stderr, "  " + e
-            return 1
+from buildbot.scripts.base import getConfigFileFromTac
 
+def _loadConfig(basedir, configFile, quiet):
+    try:
+        config.MasterConfig.loadConfig(
+                basedir, configFile)
+    except config.ConfigErrors, e:
         if not quiet:
-            print "Config file is good!"
-        return 0
+            print >> sys.stderr, "Configuration Errors:"
+            for e in e.errors:
+                print >> sys.stderr, "  " + e
+        return 1
+
+    if not quiet:
+        print "Config file is good!"
+    return 0
+
 
 def checkconfig(config):
     quiet = config.get('quiet')
-    configFileName = config.get('configFile')
+    configFile = config.get('configFile')
 
-    if os.path.isdir(configFileName):
-        cl = ConfigLoader(basedir=configFileName)
+    if os.path.isdir(configFile):
+        basedir = configFile
+        configFile = getConfigFileFromTac(basedir)
     else:
-        cl = ConfigLoader(configFileName=configFileName)
+        basedir = os.getcwd()
+
+    return _loadConfig(basedir=basedir, configFile=configFile, quiet=quiet)
+
 
-    return cl.load(quiet=quiet)
+__all__ = ['checkconfig']

master/buildbot/scripts/upgrade_master.py

@@ -159,7 +159,7 @@ def upgradeMaster(config, _noMonkey=False):
 
     os.chdir(config['basedir'])
 
-    configFile = base.getConfigFileWithFallback(config['basedir'])
+    configFile = base.getConfigFileFromTac(config['basedir'])
     master_cfg = loadConfig(config, configFile)
     if not master_cfg:
         defer.returnValue(1)

master/buildbot/test/unit/test_scripts_base.py

@@ -54,30 +54,66 @@ def test_isBuildmasterDir_matches(self):
         self.assertWasQuiet()
 
 class TestTacFallback(dirs.DirsMixin, unittest.TestCase):
+    """
+    Tests for L{base.getConfigFileFromTac}.
+    """
 
     def setUp(self):
+        """
+        Create a base directory.
+        """
         self.basedir = os.path.abspath('basedir')
-        self.stdout = cStringIO.StringIO()
-        self.filename = 'master.cfg'
         return self.setUpDirs('basedir')
 
-    def test_tacFallback_location_from_tac(self):
+    def _createBuildbotTac(self, configfile=None):
+        """
+        Create a C{buildbot.tac} that points to a given C{configfile}
+        and create that file.
+
+        @param configfile: Config file to point at and create.
+        @type configfile: L{str}
+        """
         tacfile = os.path.join(self.basedir, "buildbot.tac")
-        otherConfigFile = os.path.join(self.basedir, "other.cfg")
         with open(tacfile, "wt") as f:
-            f.write("configfile = '%s'" % otherConfigFile)
-        with open(otherConfigFile, "wt") as f:
-            f.write("#dummy")
-        self.filename = base.getConfigFileWithFallback(self.basedir)
-        self.assertEqual(self.filename, otherConfigFile)
-
-    def test_tacFallback_noFallback(self):
-        defaultFilename = self.filename
-        with open(self.filename, "wt") as f:
-            f.write("#dummy")
-        self.filename = base.getConfigFileWithFallback(self.basedir)
-        self.assertEqual(self.filename,
-                         os.path.join(self.basedir, defaultFilename))
+            if configfile is not None:
+                f.write("configfile = %r" % configfile)
+            else:
+                f.write("#dummy")
+
+
+    def test_getConfigFileFromTac(self):
+        """
+        When L{getConfigFileFromTac} is passed a C{basedir}
+        containing a C{buildbot.tac}, it reads the location
+        of the config file from there.
+        """
+        self._createBuildbotTac("other.cfg")
+        foundConfigFile = base.getConfigFileFromTac(
+                basedir=self.basedir)
+        self.assertEqual(foundConfigFile, "other.cfg")
+
+    def test_getConfigFileFromTac_fallback(self):
+        """
+        When L{getConfigFileFromTac} is passed a C{basedir}
+        which doesn't contain a C{buildbot.tac},
+        it returns C{master.cfg}
+        """
+        foundConfigFile = base.getConfigFileFromTac(
+                basedir=self.basedir)
+        self.assertEqual(foundConfigFile, 'master.cfg')
+
+
+    def test_getConfigFileFromTac_tacWithoutConfigFile(self):
+        """
+        When L{getConfigFileFromTac} is passed a C{basedir}
+        containing a C{buildbot.tac}, but C{buildbot.tac} doesn't
+        define C{configfile}, L{getConfigFileFromTac} returns C{master.cfg}
+        """
+        self._createBuildbotTac()
+        foundConfigFile = base.getConfigFileFromTac(
+                basedir=self.basedir)
+        self.assertEqual(foundConfigFile, 'master.cfg')
+
 
 
 class TestSubcommandOptions(unittest.TestCase):

master/buildbot/test/unit/test_scripts_checkconfig.py

@@ -35,7 +35,7 @@ def tearDown(self):
 
     # tests
 
-    def do_test_load(self, by_name=False, config='', other_files={},
+    def do_test_load(self, config='', other_files={},
                            stdout_re=None, stderr_re=None):
         configFile = os.path.join('configdir', 'master.cfg')
         with open(configFile, "w") as f:
@@ -51,16 +51,12 @@ def do_test_load(self, by_name=False, config='', other_files={},
             with open(fn, "w") as f:
                 f.write(contents)
 
-        if by_name:
-            cl = checkconfig.ConfigLoader(configFileName=configFile)
-        else:
-            cl = checkconfig.ConfigLoader(basedir='configdir')
-
         old_stdout, old_stderr = sys.stdout, sys.stderr
         stdout = sys.stdout = cStringIO.StringIO()
         stderr = sys.stderr = cStringIO.StringIO()
         try:
-            cl.load()
+            checkconfig._loadConfig(
+                    basedir='configdir', configFile="master.cfg", quiet=False)
         finally:
             sys.stdout, sys.stderr = old_stdout, old_stderr
         if stdout_re:
@@ -144,25 +140,20 @@ def test_success_import_package(self):
 class TestCheckconfig(unittest.TestCase):
 
     def setUp(self):
-        self.ConfigLoader = mock.Mock(name='ConfigLoader')
-        self.instance = mock.Mock(name='ConfigLoader()')
-        self.ConfigLoader.return_value = self.instance
-        self.instance.load.return_value = 3
-        self.patch(checkconfig, 'ConfigLoader', self.ConfigLoader)
+        self.loadConfig = mock.Mock(spec=checkconfig._loadConfig, return_value=3)
+        self.patch(checkconfig, '_loadConfig', self.loadConfig)
 
     def test_checkconfig_given_dir(self):
         self.assertEqual(checkconfig.checkconfig(dict(configFile='.')), 3)
-        self.ConfigLoader.assert_called_with(basedir='.')
-        self.instance.load.assert_called_with(quiet=None)
+        self.loadConfig.assert_called_with(basedir='.', configFile='master.cfg', quiet=None)
 
     def test_checkconfig_given_file(self):
         config = dict(configFile='master.cfg')
         self.assertEqual(checkconfig.checkconfig(config), 3)
-        self.ConfigLoader.assert_called_with(configFileName='master.cfg')
-        self.instance.load.assert_called_with(quiet=None)
+        self.loadConfig.assert_called_with(basedir=os.getcwd(), configFile='master.cfg', quiet=None)
 
     def test_checkconfig_quiet(self):
         config = dict(configFile='master.cfg', quiet=True)
         self.assertEqual(checkconfig.checkconfig(config), 3)
-        self.instance.load.assert_called_with(quiet=True)
+        self.loadConfig.assert_called_with(basedir=os.getcwd(), configFile='master.cfg', quiet=True)