diff --git a/paasta_tools/cli/cmds/check.py b/paasta_tools/cli/cmds/check.py index 300a29142e..541401ac41 100644 --- a/paasta_tools/cli/cmds/check.py +++ b/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): diff --git a/paasta_tools/cli/cmds/validate.py b/paasta_tools/cli/cmds/validate.py index 6e8940f492..70315aa1d4 100644 --- a/paasta_tools/cli/cmds/validate.py +++ b/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) diff --git a/tests/cli/test_cmds_check.py b/tests/cli/test_cmds_check.py index e878319e67..39f4ccf83f 100644 --- a/tests/cli/test_cmds_check.py +++ b/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) diff --git a/tests/cli/test_cmds_validate.py b/tests/cli/test_cmds_validate.py index c1a6f4fb65..5ef81b4c19 100644 --- a/tests/cli/test_cmds_validate.py +++ b/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)