From e35cf2c80f4834da4fdf5a0d6032d8ae7c8564ff Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Thu, 3 May 2018 15:22:44 +0100 Subject: [PATCH] [fix] Render documents by each site to avoid false positives This patchset changes pegleg's lint function `_verify_deckhand_render` to render each batch of documents by site, rather than attempting to render all documents for every site in one go, which results in false positives. For example, given 3 sites -- site1, site2, and site3 -- Deckhand will now render all documents from global, all documents in type, and all documents in site1, then repeat the same for site2, then repeat the same for site3, and finally aggregate the error results from each Deckhand render output, which is returned back to the user. Change-Id: If853ba0331470d7ec66dfc6116e626251a9f496d --- src/bin/pegleg/pegleg/engine/lint.py | 33 +++-- src/bin/pegleg/pegleg/engine/util/files.py | 5 + src/bin/pegleg/setup.py | 2 +- src/bin/pegleg/tests/unit/engine/__init__.py | 0 src/bin/pegleg/tests/unit/engine/test_lint.py | 68 +++++++++ .../{ => engine}/test_selectable_linting.py | 0 .../tests/unit/engine/test_util_files.py | 55 ++++++++ src/bin/pegleg/tests/unit/fixtures.py | 131 ++++++++++++++++++ .../tests/unit/test_engine_util_files.py | 31 ----- src/bin/pegleg/tox.ini | 2 +- 10 files changed, 284 insertions(+), 43 deletions(-) create mode 100644 src/bin/pegleg/tests/unit/engine/__init__.py create mode 100644 src/bin/pegleg/tests/unit/engine/test_lint.py rename src/bin/pegleg/tests/unit/{ => engine}/test_selectable_linting.py (100%) create mode 100644 src/bin/pegleg/tests/unit/engine/test_util_files.py create mode 100644 src/bin/pegleg/tests/unit/fixtures.py delete mode 100644 src/bin/pegleg/tests/unit/test_engine_util_files.py diff --git a/src/bin/pegleg/pegleg/engine/lint.py b/src/bin/pegleg/pegleg/engine/lint.py index b350008c..d5a4f84e 100644 --- a/src/bin/pegleg/pegleg/engine/lint.py +++ b/src/bin/pegleg/pegleg/engine/lint.py @@ -160,18 +160,31 @@ def _verify_document(document, schemas, filename): def _verify_deckhand_render(fail_on_missing_sub_src=False): - documents = [] + sitenames = list(util.files.list_sites()) + documents_by_site = {s: [] for s in sitenames} - for filename in util.files.all(): - with open(filename) as f: - documents.extend(list(yaml.safe_load_all(f))) + for sitename in sitenames: + params = util.definition.load_as_params(sitename) + paths = util.files.directories_for(**params) + filenames = set(util.files.search(paths)) + for filename in filenames: + with open(filename) as f: + documents_by_site[sitename].extend(list(yaml.safe_load_all(f))) - _, errors = util.deckhand.deckhand_render( - documents=documents, - fail_on_missing_sub_src=fail_on_missing_sub_src, - validate=True, - ) - return errors + all_errors = [] + + for sitename, documents in documents_by_site.items(): + LOG.debug('Rendering documents for site: %s.', sitename) + _, errors = util.deckhand.deckhand_render( + documents=documents, + fail_on_missing_sub_src=fail_on_missing_sub_src, + validate=True, + ) + LOG.debug('Generated %d rendering errors for site: %s.', len(errors), + sitename) + all_errors.extend(errors) + + return list(set(all_errors)) def _layer(data): diff --git a/src/bin/pegleg/pegleg/engine/util/files.py b/src/bin/pegleg/pegleg/engine/util/files.py index c2fd3e9a..3db8e8d7 100644 --- a/src/bin/pegleg/pegleg/engine/util/files.py +++ b/src/bin/pegleg/pegleg/engine/util/files.py @@ -104,6 +104,11 @@ def _create_tree(root_path, *, tree=FULL_STRUCTURE): os.makedirs(path, mode=0o775, exist_ok=True) _create_tree(path, tree=data) + for filename, yaml_data in tree.get('files', {}).items(): + path = os.path.join(root_path, filename) + with open(path, 'w') as f: + yaml.safe_dump(yaml_data, f) + def directories_for(*, site_name, revision, site_type): library_list = [ diff --git a/src/bin/pegleg/setup.py b/src/bin/pegleg/setup.py index bdbec08f..53d15a5d 100644 --- a/src/bin/pegleg/setup.py +++ b/src/bin/pegleg/setup.py @@ -17,7 +17,7 @@ from setuptools import setup setup( name='pegleg', version='0.1.0', - packages=['pegleg'], + packages=['pegleg', 'tests'], entry_points={ 'console_scripts': [ 'pegleg=pegleg.cli:main', diff --git a/src/bin/pegleg/tests/unit/engine/__init__.py b/src/bin/pegleg/tests/unit/engine/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/src/bin/pegleg/tests/unit/engine/test_lint.py b/src/bin/pegleg/tests/unit/engine/test_lint.py new file mode 100644 index 00000000..5448b115 --- /dev/null +++ b/src/bin/pegleg/tests/unit/engine/test_lint.py @@ -0,0 +1,68 @@ +# Copyright 2018 AT&T Intellectual Property. All other rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import mock + +from pegleg.engine import lint +from pegleg.engine.util import files +from tests.unit.fixtures import create_tmp_deployment_files + + +def test_verify_deckhand_render_site_documents_separately( + create_tmp_deployment_files): + expected_documents = { + 'cicd': [ + 'global-common', 'global-v1.0', 'cicd-type-common', + 'cicd-type-v1.0', 'cicd-chart', 'cicd-passphrase' + ], + 'lab': [ + 'global-common', 'global-v1.0', 'lab-type-common', 'lab-type-v1.0', + 'lab-chart', 'lab-passphrase' + ], + } + + with mock.patch( + 'pegleg.engine.util.deckhand.deckhand_render', + autospec=True) as mock_render: + mock_render.return_value = (None, []) + + result = lint._verify_deckhand_render() + assert result == [] + + sites = files.list_sites() + + # Verify that both expected site types are listed. + assert sorted(sites) == ['cicd', 'lab'] + # Verify that Deckhand called render twice, once for each site. + assert mock_render.call_count == 2 + + mock_calls = list(mock_render.mock_calls) + for mock_call in mock_calls: + documents = mock_call[2]['documents'] + assert len(documents) == 7 + + # Verify one site_definition.yaml per site. + site_definitions = [x for x in documents if isinstance(x, dict)] + assert len(site_definitions) == 1 + + site_definition = site_definitions[0] + site_type = site_definition['data']['site_type'] + + assert site_definition['data']['revision'] == 'v1.0' + assert site_type in expected_documents + + # Verify expected documents collected per site. + other_documents = expected_documents[site_type] + for other_document in other_documents: + assert other_document in documents diff --git a/src/bin/pegleg/tests/unit/test_selectable_linting.py b/src/bin/pegleg/tests/unit/engine/test_selectable_linting.py similarity index 100% rename from src/bin/pegleg/tests/unit/test_selectable_linting.py rename to src/bin/pegleg/tests/unit/engine/test_selectable_linting.py diff --git a/src/bin/pegleg/tests/unit/engine/test_util_files.py b/src/bin/pegleg/tests/unit/engine/test_util_files.py new file mode 100644 index 00000000..7c40de8c --- /dev/null +++ b/src/bin/pegleg/tests/unit/engine/test_util_files.py @@ -0,0 +1,55 @@ +# Copyright 2018 AT&T Intellectual Property. All other rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import tempfile + +from pegleg import config +from pegleg.engine.util import files +from tests.unit.fixtures import create_tmp_deployment_files + + +def test_no_non_yamls(tmpdir): + p = tmpdir.mkdir("deployment_files").mkdir("global") + for x in range(3): # Create 3 YAML files + p.join("good-%d.yaml" % x).write('fake-content') + p.join("bad.txt").write("fake-content") + config.set_primary_repo(str(tmpdir.listdir()[0])) + results = list(files.all()) + + assert 3 == len(results) + # Make sure only YAML files are returned + for i in results: + assert i.endswith('.yaml') + + +def test_list_all_files(create_tmp_deployment_files): + expected_files = sorted([ + 'deployment_files/global/common/global-common.yaml', + 'deployment_files/global/v1.0/global-v1.0.yaml', + 'deployment_files/type/cicd/common/cicd-type-common.yaml', + 'deployment_files/type/cicd/v1.0/cicd-type-v1.0.yaml', + 'deployment_files/type/lab/common/lab-type-common.yaml', + 'deployment_files/type/lab/v1.0/lab-type-v1.0.yaml', + 'deployment_files/site/cicd/secrets/passphrases/cicd-passphrase.yaml', + 'deployment_files/site/cicd/site-definition.yaml', + 'deployment_files/site/cicd/software/charts/cicd-chart.yaml', + 'deployment_files/site/lab/secrets/passphrases/lab-passphrase.yaml', + 'deployment_files/site/lab/site-definition.yaml', + 'deployment_files/site/lab/software/charts/lab-chart.yaml', + ]) + actual_files = sorted(files.all()) + + assert len(actual_files) == len(expected_files) + for idx, file in enumerate(actual_files): + assert file.endswith(expected_files[idx]) diff --git a/src/bin/pegleg/tests/unit/fixtures.py b/src/bin/pegleg/tests/unit/fixtures.py new file mode 100644 index 00000000..df7b2d49 --- /dev/null +++ b/src/bin/pegleg/tests/unit/fixtures.py @@ -0,0 +1,131 @@ +# Copyright 2018 AT&T Intellectual Property. All other rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from __future__ import absolute_import +import os +import tempfile + +import pytest +import yaml + +from pegleg import config +from pegleg.engine.util import files + + +@pytest.fixture() +def create_tmp_deployment_files(tmpdir): + """Fixture that creates a temporary directory structure.""" + orig_primary_repo = config.get_primary_repo() + sitenames = ['cicd', 'lab'] + + SITE_TEST_STRUCTURE = { + 'directories': { + 'secrets': { + 'directories': { + 'passphrases': { + 'files': {} + }, + }, + }, + 'software': { + 'directories': { + 'charts': { + 'files': {} + }, + }, + }, + }, + 'files': {} + } + + p = tmpdir.mkdir("deployment_files") + config.set_primary_repo(str(p)) + + # Create global directories and files. + files._create_tree( + os.path.join(str(p), 'global'), + tree={ + 'directories': { + 'common': { + 'files': { + 'global-common.yaml': 'global-common' + } + }, + 'v1.0': { + 'files': { + 'global-v1.0.yaml': 'global-v1.0' + } + } + } + }) + + # Create type directories and files. + files._create_tree( + os.path.join(str(p), 'type'), + tree={ + 'directories': { + site: { + 'directories': { + 'common': { + 'files': { + '%s-type-common.yaml' % site: + ('%s-type-common' % site) + } + }, + 'v1.0': { + 'files': { + '%s-type-v1.0.yaml' % site: + ('%s-type-v1.0' % site) + } + } + } + } + for site in sitenames + } + }) + + # Create site directories and files. + for site in sitenames: + site_definition = """ +--- +data: + revision: v1.0 + site_type: %s +metadata: + layeringDefinition: {abstract: false, layer: site} + name: %s + schema: metadata/Document/v1 + storagePolicy: cleartext +schema: pegleg/SiteDefinition/v1 +... +""" % (site, site) + + test_structure = SITE_TEST_STRUCTURE.copy() + test_structure['directories']['secrets']['directories']['passphrases'][ + 'files'] = { + '%s-passphrase.yaml' % site: '%s-passphrase' % site + } + test_structure['directories']['software']['directories']['charts'][ + 'files'] = { + '%s-chart.yaml' % site: '%s-chart' % site + } + test_structure['files']['site-definition.yaml'] = yaml.safe_load( + site_definition) + + cicd_path = os.path.join(str(p), files._site_path(site)) + files._create_tree(cicd_path, tree=test_structure) + + yield + + config.set_primary_repo(orig_primary_repo) diff --git a/src/bin/pegleg/tests/unit/test_engine_util_files.py b/src/bin/pegleg/tests/unit/test_engine_util_files.py deleted file mode 100644 index 7a5b1ea8..00000000 --- a/src/bin/pegleg/tests/unit/test_engine_util_files.py +++ /dev/null @@ -1,31 +0,0 @@ -# Copyright 2018 AT&T Intellectual Property. All other rights reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -import tempfile - -from pegleg import config -from pegleg.engine.util import files - - -def test_no_non_yamls(tmpdir): - p = tmpdir.mkdir("site_yamls").mkdir("global") - for x in range(3): # Create 3 YAML files - p.join("good-%d.yaml" % x).write('fake-content') - p.join("bad.txt").write("fake-content") - config.set_primary_repo(str(tmpdir.listdir()[0])) - results = list(files.all()) - - assert 3 == len(results) - # Make sure only YAML files are returned - for i in results: - assert i.endswith('.yaml') diff --git a/src/bin/pegleg/tox.ini b/src/bin/pegleg/tox.ini index 0658b6a4..c6795d50 100644 --- a/src/bin/pegleg/tox.ini +++ b/src/bin/pegleg/tox.ini @@ -28,4 +28,4 @@ commands = flake8 {toxinidir}/pegleg [flake8] -ignore = E251,W503 +ignore = E125,E251,W503