CLI: improve configuration error handling

Bug: T158747
Change-Id: Ibdd41c62ad3a86528b59b5c662634929812f9b55

cumin/cli.py

@@ -136,9 +136,9 @@ def parse_args(argv=None):
 def get_running_user():
     """Ensure it's running as root and that the original user is detected and return it."""
     if os.getenv('USER') != 'root':
-        raise RuntimeError('Unsufficient privileges, run with sudo')
+        raise CuminError('Insufficient privileges, run with sudo')
     if os.getenv('SUDO_USER') in (None, 'root'):
-        raise RuntimeError('Unable to determine real user')
+        raise CuminError('Unable to determine real user, logged in as root?')
 
     return os.getenv('SUDO_USER')
 
@@ -173,8 +173,17 @@ def parse_config(config_file):
     Arguments:
     config_file -- the path of the configuration file to load
     """
-    with open(config_file, 'r') as f:
-        config = yaml.safe_load(f)
+    try:
+        with open(config_file, 'r') as f:
+            config = yaml.safe_load(f)
+    except IOError as e:
+        raise CuminError('Unable to read configuration file: {message}'.format(message=e))
+    except yaml.parser.ParserError as e:
+        raise CuminError("Unable to parse configuration file '{config}':\n{message}".format(
+            config=config_file, message=e))
+
+    if config is None:
+        raise CuminError("Empty configuration found in '{config}'".format(config=config_file))
 
     return config
 
@@ -257,7 +266,7 @@ def get_hosts(args, config):
     elif not sys.stdout.isatty():
         message = 'Not in a TTY but neither DRY-RUN nor FORCE mode were specified.'
         stderr(message)
-        raise RuntimeError(message)
+        raise CuminError(message)
 
     for i in xrange(10):
         stderr('Confirm to continue [y/n]?', end=' ')
@@ -351,6 +360,9 @@ def main(argv=None):
         user = get_running_user()
         config = parse_config(args.config)
         setup_logging(config['log_file'], debug=args.debug)
+    except CuminError as e:
+        stderr(e)
+        return 2
     except Exception as e:
         stderr('Caught {name} exception: {msg}'.format(name=e.__class__.__name__, msg=e))
         return 3

cumin/tests/unit/test_cli.py

@@ -1,15 +1,17 @@
 """CLI tests."""
 
-import unittest
+import os
 import sys
+import tempfile
+import unittest
 
 from functools import wraps
 from logging import DEBUG, INFO
 from StringIO import StringIO
 
 import mock
 
-from cumin import cli
+from cumin import cli, CuminError
 
 # Environment variables
 _ENV = {'USER': 'root', 'SUDO_USER': 'user'}
@@ -80,12 +82,12 @@ def test_get_running_user(self):
         """Unsufficient permissions or unknown user should raise RuntimeError and a proper user should be detected."""
         env = {'USER': None, 'SUDO_USER': None}
         with mock.patch('os.getenv', env.get):
-            with self.assertRaisesRegexp(RuntimeError, r'Unsufficient privileges, run with sudo'):
+            with self.assertRaisesRegexp(CuminError, r'Insufficient privileges, run with sudo'):
                 cli.get_running_user()
 
         env = {'USER': 'root', 'SUDO_USER': None}
         with mock.patch('os.getenv', env.get):
-            with self.assertRaisesRegexp(RuntimeError, r'Unable to determine real user'):
+            with self.assertRaisesRegexp(CuminError, r'Unable to determine real user'):
                 cli.get_running_user()
 
         with mock.patch('os.getenv', _ENV.get):
@@ -94,21 +96,39 @@ def test_get_running_user(self):
     @mock.patch('cumin.cli.os')
     @mock.patch('cumin.cli.RotatingFileHandler')
     @mock.patch('cumin.cli.logger')
-    def test_setup_logging(self, logging, file_handler, os):
+    def test_setup_logging(self, logging, file_handler, mocked_os):
         """Calling setup_logging() should properly setup the logger."""
-        os.path.exists.return_value = False
+        mocked_os.path.exists.return_value = False
         cli.setup_logging('/path/to/filename')
         logging.setLevel.assert_called_with(INFO)
 
-        os.path.exists.return_value = True
+        mocked_os.path.exists.return_value = True
         cli.setup_logging('filename', debug=True)
         logging.setLevel.assert_called_with(DEBUG)
 
-    def test_parse_config(self):
+    def test_parse_config_ok(self):
         """The configuration file is properly parsed and accessible."""
         config = cli.parse_config('doc/examples/config.yaml')
         self.assertTrue('log_file' in config)
 
+    def test_parse_config_non_existent(self):
+        """A CuminError is raised if the configuration file is not available."""
+        with self.assertRaisesRegexp(CuminError, 'Unable to read configuration file'):
+            cli.parse_config('not_existent_config.yaml')
+
+    def test_parse_config_invalid(self):
+        """A CuminError is raised if the configuration cannot be parsed."""
+        invalid_yaml = '\n'.join((
+            'foo:',
+            '  bar: baz',
+            '  - foobar',
+        ))
+        tmpfile, tmpfilepath = tempfile.mkstemp(suffix='config.yaml', prefix='cumin', text=True)
+        os.write(tmpfile, invalid_yaml)
+
+        with self.assertRaisesRegexp(CuminError, 'Unable to parse configuration file'):
+            cli.parse_config(tmpfilepath)
+
     @mock.patch('cumin.cli.stderr')
     @mock.patch('cumin.cli.raw_input')
     @mock.patch('cumin.cli.sys.stdout.isatty')
@@ -185,7 +205,7 @@ def test_get_hosts_no_tty_ko(self, isatty, stderr):
         args = cli.parse_args(argv=['host1', 'command1'])
         config = {'backend': 'direct'}
         isatty.return_value = False
-        with self.assertRaisesRegexp(RuntimeError, 'Not in a TTY but neither DRY-RUN nor FORCE mode were specified'):
+        with self.assertRaisesRegexp(CuminError, 'Not in a TTY but neither DRY-RUN nor FORCE mode were specified'):
             cli.get_hosts(args, config)
         self.assertTrue(stderr.called)