Merge pull request #161 from Yelp/paasta-check-validate

Make paasta check call validate (Closes #128)

paasta_tools/cli/cmds/check.py

@@ -21,6 +21,7 @@
 from service_configuration_lib import DEFAULT_SOA_DIR
 from service_configuration_lib import read_service_configuration
 
+from paasta_tools.cli.cmds.validate import paasta_validate_soa_configs
 from paasta_tools.cli.utils import figure_out_service_name
 from paasta_tools.cli.utils import get_file_contents
 from paasta_tools.cli.utils import is_file_in_dir
@@ -349,6 +350,7 @@ def paasta_check(args):
     deployments_check(service, soa_dir)
     sensu_check(service, service_path)
     smartstack_check(service, service_path)
+    paasta_validate_soa_configs(service_path)
 
 
 def read_dockerfile_lines(path):

paasta_tools/cli/cmds/validate.py

@@ -95,7 +95,7 @@ def validate_schema(file_path, file_type):
         config_file = get_file_contents(file_path)
     except IOError:
         print '%s: %s' % (FAILED_READING_FILE, file_path)
-        return 1
+        return False
     if extension == '.yaml':
         config_file_object = yaml.load(config_file)
     elif extension == '.json':
@@ -107,10 +107,10 @@ def validate_schema(file_path, file_type):
     except ValidationError as e:
         print '%s: %s' % (SCHEMA_INVALID, file_path)
         print '  Validation Message: %s' % e.message
-        return 1
+        return False
     else:
         print '%s: %s' % (SCHEMA_VALID, basename)
-        return 0
+        return True
 
 
 def validate_all_schemas(service_path):
@@ -122,16 +122,15 @@ def validate_all_schemas(service_path):
 
     path = os.path.join(service_path, '*.yaml')
 
-    returncode = 0
+    returncode = True
     for file_name in glob(path):
         if os.path.islink(file_name):
             continue
         basename = os.path.basename(file_name)
         for file_type in ['chronos', 'marathon']:
             if basename.startswith(file_type):
-                tmp_returncode = validate_schema(file_name, file_type)
-                if tmp_returncode != 0:
-                    returncode = tmp_returncode
+                if not validate_schema(file_name, file_type):
+                    returncode = False
     return returncode
 
 
@@ -155,10 +154,27 @@ def add_subparser(subparsers):
     validate_parser.set_defaults(command=paasta_validate)
 
 
+def check_service_path(service_path):
+    """Check that the specified path exists and has yaml files
+
+    :param service_path: Path to directory that should contain yaml files
+    """
+    if not service_path or not os.path.isdir(service_path):
+        print failure("%s is not a directory" % service_path,
+                      "http://paasta.readthedocs.org/en/latest/yelpsoa_configs.html")
+        return False
+    if not glob(os.path.join(service_path, "*.yaml")):
+        print failure("%s does not contain any .yaml files" % service_path,
+                      "http://paasta.readthedocs.org/en/latest/yelpsoa_configs.html")
+        return False
+    return True
+
+
 def get_service_path(service, soa_dir):
     """Determine the path of the directory containing the conf files
 
-    :param args: argparse.Namespace obj created from sys.args by cli
+    :param service: Name of service
+    :param soa_dir: Directory containing soa configs for all services
     """
     if service:
         service_path = os.path.join(soa_dir, service)
@@ -168,28 +184,22 @@ def get_service_path(service, soa_dir):
         else:
             print UNKNOWN_SERVICE
             return None
-    if not os.path.isdir(service_path):
-        print failure("%s is not a directory" % service_path,
-                      "http://paasta.readthedocs.org/en/latest/yelpsoa_configs.html")
-        return None
-    if not glob(os.path.join(service_path, "*.yaml")):
-        print failure("%s does not contain any .yaml files" % service_path,
-                      "http://paasta.readthedocs.org/en/latest/yelpsoa_configs.html")
-        return None
     return service_path
 
 
 def path_to_soa_dir_service(service_path):
+    """Split a service_path into its soa_dir and service name components"""
     soa_dir = os.path.dirname(service_path)
     service = os.path.basename(service_path)
     return soa_dir, service
 
 
 def validate_chronos(service_path):
+    """Check that any chronos configurations are valid"""
     soa_dir, service = path_to_soa_dir_service(service_path)
     instance_type = 'chronos'
 
-    returncode = 0
+    returncode = True
     for cluster in list_clusters(service, soa_dir, instance_type):
         for instance in list_all_instances_for_service(
                 service=service, clusters=[cluster], instance_type=instance_type,
@@ -202,34 +212,39 @@ def validate_chronos(service_path):
 
             if not checks_passed:
                 print invalid_chronos_instance(cluster, instance, "\n  ".join(unique_check_msgs))
-                returncode = 1
+                returncode = False
             else:
                 print valid_chronos_instance(cluster, instance)
     return returncode
 
 
+def paasta_validate_soa_configs(service_path):
+    """Analyze the service in serivce_path to determine if the conf files are valid
+
+    :param service_path: Path to directory containing soa conf yaml files for service
+    """
+    if not check_service_path(service_path):
+        return False
+
+    returncode = True
+
+    if not validate_all_schemas(service_path):
+        returncode = False
+
+    if not validate_chronos(service_path):
+        returncode = False
+
+    return returncode
+
+
 def paasta_validate(args):
-    """Analyze the service in the PWD to determine if conf files are all valid
+    """Generate a service_path from the provided args and call paasta_validate_soa_configs
 
     :param args: argparse.Namespace obj created from sys.args by cli
     """
-
     service = args.service
     soa_dir = args.yelpsoa_config_root
     service_path = get_service_path(service, soa_dir)
 
-    if service_path is None:
+    if not paasta_validate_soa_configs(service_path):
         sys.exit(1)
-
-    returncode = 0
-
-    tmp_returncode = validate_all_schemas(service_path)
-    if tmp_returncode != 0:
-        returncode = tmp_returncode
-
-    tmp_returncode = validate_chronos(service_path)
-    if tmp_returncode != 0:
-        returncode = tmp_returncode
-
-    if returncode != 0:
-        sys.exit(returncode)

tests/cli/test_cmds_check.py

@@ -53,7 +53,9 @@
 @patch('paasta_tools.cli.cmds.check.deployments_check')
 @patch('paasta_tools.cli.cmds.check.sensu_check')
 @patch('paasta_tools.cli.cmds.check.smartstack_check')
+@patch('paasta_tools.cli.cmds.check.paasta_validate_soa_configs')
 def test_check_paasta_check_calls_everything(
+        mock_paasta_validate_soa_configs,
         mock_smartstart_check,
         mock_sensu_check,
         mock_deployments_check,
@@ -88,6 +90,7 @@ def test_check_paasta_check_calls_everything(
     assert mock_yaml_check.called
     assert mock_sensu_check.called
     assert mock_smartstart_check.called
+    assert mock_paasta_validate_soa_configs.called
 
     service_path = os.path.join(args.yelpsoa_config_root,
                                 mock_figure_out_service_name.return_value)

tests/cli/test_cmds_validate.py

@@ -18,10 +18,12 @@
 from mock import patch
 from pytest import raises
 
+from paasta_tools.cli.cmds.validate import check_service_path
 from paasta_tools.cli.cmds.validate import get_schema
 from paasta_tools.cli.cmds.validate import get_service_path
 from paasta_tools.cli.cmds.validate import invalid_chronos_instance
 from paasta_tools.cli.cmds.validate import paasta_validate
+from paasta_tools.cli.cmds.validate import paasta_validate_soa_configs
 from paasta_tools.cli.cmds.validate import SCHEMA_INVALID
 from paasta_tools.cli.cmds.validate import SCHEMA_VALID
 from paasta_tools.cli.cmds.validate import UNKNOWN_SERVICE
@@ -33,16 +35,19 @@
 @patch('paasta_tools.cli.cmds.validate.validate_all_schemas')
 @patch('paasta_tools.cli.cmds.validate.validate_chronos')
 @patch('paasta_tools.cli.cmds.validate.get_service_path')
+@patch('paasta_tools.cli.cmds.validate.check_service_path')
 def test_paasta_validate_calls_everything(
+    mock_check_service_path,
     mock_get_service_path,
     mock_validate_chronos,
     mock_validate_all_schemas
 ):
     # Ensure each check in 'paasta_validate' is called
 
+    mock_check_service_path.return_value = True
     mock_get_service_path.return_value = 'unused_path'
-    mock_validate_all_schemas.return_value = 0
-    mock_validate_chronos.return_value = 0
+    mock_validate_all_schemas.return_value = True
+    mock_validate_chronos.return_value = True
 
     args = mock.MagicMock()
     args.service = None
@@ -79,6 +84,12 @@ def test_validate_unknown_service():
     assert excinfo.value.code == 1
 
 
+def test_validate_unknown_service_service_path():
+    service_path = 'unused/path'
+
+    assert not paasta_validate_soa_configs(service_path)
+
+
 @patch('paasta_tools.cli.cmds.validate.os.path.isdir')
 @patch('paasta_tools.cli.cmds.validate.glob')
 def test_get_service_path_cwd(
@@ -154,7 +165,7 @@ def test_marathon_validate_schema_list_hashes_good(
 """
     mock_get_file_contents.return_value = marathon_content
 
-    assert validate_schema('unused_service_path.yaml', 'marathon') == 0
+    assert validate_schema('unused_service_path.yaml', 'marathon')
 
     output = mock_stdout.getvalue()
 
@@ -175,7 +186,7 @@ def test_marathon_validate_schema_keys_outside_instance_blocks_bad(
     "page": false
 }
 """
-    assert validate_schema('unused_service_path.json', 'marathon') == 1
+    assert not validate_schema('unused_service_path.json', 'marathon')
 
     output = mock_stdout.getvalue()
 
@@ -195,7 +206,7 @@ def test_marathon_validate_invalid_key_bad(
     }
 }
 """
-    assert validate_schema('unused_service_path.json', 'marathon') == 1
+    assert not validate_schema('unused_service_path.json', 'marathon')
 
     output = mock_stdout.getvalue()
 
@@ -218,7 +229,7 @@ def test_chronos_validate_schema_list_hashes_good(
     }
 }
 """
-    assert validate_schema('unused_service_path.json', 'chronos') == 0
+    assert validate_schema('unused_service_path.json', 'chronos')
 
     output = mock_stdout.getvalue()
 
@@ -239,7 +250,7 @@ def test_chronos_validate_schema_keys_outside_instance_blocks_bad(
     "page": false
 }
 """
-    assert validate_schema('unused_service_path.json', 'chronos') == 1
+    assert not validate_schema('unused_service_path.json', 'chronos')
 
     output = mock_stdout.getvalue()
 
@@ -269,7 +280,7 @@ def test_failing_chronos_job_validate(
     mock_list_all_instances_for_service.return_value = [fake_instance]
     mock_load_chronos_job_config.return_value = mock_chronos_job
 
-    assert validate_chronos('fake_service_path') == 1
+    assert not validate_chronos('fake_service_path')
 
     output = mock_stdout.getvalue()
 
@@ -300,8 +311,45 @@ def test_validate_chronos_valid_instance(
     mock_list_all_instances_for_service.return_value = [fake_instance]
     mock_load_chronos_job_config.return_value = mock_chronos_job
 
-    assert validate_chronos('fake_service_path') == 0
+    assert validate_chronos('fake_service_path')
 
     output = mock_stdout.getvalue()
 
     assert valid_chronos_instance(fake_cluster, fake_instance) in output
+
+
+@patch('sys.stdout', new_callable=StringIO)
+def test_check_service_path_none(
+    mock_stdout
+):
+    service_path = None
+    assert not check_service_path(service_path)
+
+    output = mock_stdout.getvalue()
+    assert "%s is not a directory" % service_path in output
+
+
+@patch('sys.stdout', new_callable=StringIO)
+@patch('paasta_tools.cli.cmds.validate.os.path.isdir')
+def test_check_service_path_empty(
+    mock_isdir,
+    mock_stdout
+):
+    mock_isdir.return_value = True
+    service_path = 'fake/path'
+    assert not check_service_path(service_path)
+
+    output = mock_stdout.getvalue()
+    assert "%s does not contain any .yaml files" % service_path in output
+
+
+@patch('paasta_tools.cli.cmds.validate.os.path.isdir')
+@patch('paasta_tools.cli.cmds.validate.glob')
+def test_check_service_path_good(
+    mock_glob,
+    mock_isdir
+):
+    mock_isdir.return_value = True
+    mock_glob.return_value = True
+    service_path = 'fake/path'
+    assert check_service_path(service_path)