diff --git a/src/aap_eda/services/project/__init__.py b/src/aap_eda/services/project/__init__.py index e670a6e1a..1832e4f06 100644 --- a/src/aap_eda/services/project/__init__.py +++ b/src/aap_eda/services/project/__init__.py @@ -12,6 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -from .imports import ProjectImportService +from .imports import ProjectImportError, ProjectImportService -__all__ = ("ProjectImportService",) +__all__ = ("ProjectImportService", "ProjectImportError") diff --git a/src/aap_eda/services/project/imports.py b/src/aap_eda/services/project/imports.py index 1b71acec2..c6b397957 100644 --- a/src/aap_eda/services/project/imports.py +++ b/src/aap_eda/services/project/imports.py @@ -22,7 +22,6 @@ import yaml from django.core import exceptions -from django.db import utils from aap_eda.core import models from aap_eda.core.types import StrPath @@ -46,27 +45,39 @@ class ProjectImportError(Exception): pass +class MalformedError(Exception): + pass + + def _project_import_wrapper( func: Callable[[ProjectImportService, models.Project], None] ): @wraps(func) def wrapper(self: ProjectImportService, project: models.Project): project.import_state = models.Project.ImportState.RUNNING - project.save() + project.save(update_fields=["import_state"]) + error = None try: func(self, project) project.import_state = models.Project.ImportState.COMPLETED - project.save() - except (utils.IntegrityError, exceptions.ObjectDoesNotExist): - logger.exception("Object may have been deleted") except Exception as e: + project.import_state = models.Project.ImportState.FAILED + project.import_error = str(e) + error = e + finally: try: - project.import_state = models.Project.ImportState.FAILED - project.import_error = str(e) - project.save() + project.save(update_fields=["import_state", "import_error"]) except exceptions.ObjectDoesNotExist: - logger.exception("Project may have been deleted") - raise + raise ProjectImportError( + "Project may have been deleted" + ) from error + else: + if error and isinstance(error, ProjectImportError): + raise error + elif error: + raise ProjectImportError( + f"Failed to import the project: {str(error)}" + ) from error return wrapper @@ -224,7 +235,10 @@ def _try_load_rulebook( logger.warning("Invalid YAML file %s: %s", rulebook_path, exc) return None - if not self._is_rulebook_file(content): + try: + self._validate_rulebook_file(content) + except MalformedError as exc: + logger.warning("Malformed rulebook %s: %s", rulebook_path, exc) return None relpath = os.path.relpath(rulebook_path, rulebooks_dir) @@ -234,7 +248,22 @@ def _try_load_rulebook( content=content, ) - def _is_rulebook_file(self, data: Any) -> bool: + def _validate_rulebook_file(self, data: Any) -> None: if not isinstance(data, list): - return False - return all("rules" in entry for entry in data) + raise MalformedError("rulebook must contain a list of rulesets") + required_keys = ["name", "condition", "action"] + for ruleset in data: + if "rules" not in ruleset: + raise MalformedError("no rules in a ruleset") + rules = ruleset["rules"] + if not isinstance(rules, list): + raise MalformedError("ruleset must contain a list of rules") + for rule in rules: + if not all(key in rule for key in required_keys): + raise MalformedError( + f"ruleset must contain {required_keys}" + ) + if not all(rule.get(key) is not None for key in required_keys): + raise MalformedError( + f"rule's {required_keys} must have non empty values" + ) diff --git a/src/aap_eda/tasks/project.py b/src/aap_eda/tasks/project.py index d3582998e..c41c9dc29 100644 --- a/src/aap_eda/tasks/project.py +++ b/src/aap_eda/tasks/project.py @@ -16,7 +16,7 @@ from aap_eda.core import models from aap_eda.core.tasking import get_queue, job, unique_enqueue -from aap_eda.services.project import ProjectImportService +from aap_eda.services.project import ProjectImportError, ProjectImportService logger = logging.getLogger(__name__) PROJECT_TASKS_QUEUE = "default" @@ -27,7 +27,10 @@ def import_project(project_id: int): logger.info(f"Task started: Import project ( {project_id=} )") project = models.Project.objects.get(pk=project_id) - ProjectImportService().import_project(project) + try: + ProjectImportService().import_project(project) + except ProjectImportError as e: + logger.exception(e) logger.info(f"Task complete: Import project ( project_id={project.id} )") @@ -37,7 +40,10 @@ def sync_project(project_id: int): logger.info(f"Task started: Sync project ( {project_id=} )") project = models.Project.objects.get(pk=project_id) - ProjectImportService().sync_project(project) + try: + ProjectImportService().sync_project(project) + except ProjectImportError as e: + logger.exception(e) logger.info(f"Task complete: Sync project ( project_id={project.id} )") diff --git a/tests/integration/services/data/project-05-import.json b/tests/integration/services/data/project-05-import.json new file mode 100644 index 000000000..f829a63c9 --- /dev/null +++ b/tests/integration/services/data/project-05-import.json @@ -0,0 +1,20 @@ +[ + { + "name": "good.yml", + "rulesets": [ + { + "name": "Hello Events", + "rules": [ + { + "name": "Say Hello", + "action": { + "run_playbook": { + "name": "ansible.eda.hello" + } + } + } + ] + } + ] + } +] diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad1.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad1.yml new file mode 100644 index 000000000..34c682155 --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad1.yml @@ -0,0 +1,13 @@ +--- +name: Hello Events +hosts: all +sources: + - ansible.eda.range: + limit: 5 +rules: + - name: Say Hello + condition: event.i == 1 + action: + run_playbook: + name: ansible.eda.hello +... diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad10.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad10.yml new file mode 100644 index 000000000..d2e713121 --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad10.yml @@ -0,0 +1,11 @@ +--- +- name: Hello Events + hosts: all + sources: + - ansible.eda.range: + limit: 5 + rules: + - name: Say Hello + condition: event.i == 1 + action: +... diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad2.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad2.yml new file mode 100644 index 000000000..0d2b20c52 --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad2.yml @@ -0,0 +1,8 @@ +--- +- name: Hello Events + hosts: all + sources: + - ansible.eda.range: + limit: 5 + rules: bad +... diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad3.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad3.yml new file mode 100644 index 000000000..581ad5924 --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad3.yml @@ -0,0 +1,8 @@ +--- +- name: Hello Events + hosts: all + sources: + - ansible.eda.range: + limit: 5 + rules: +... diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad4.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad4.yml new file mode 100644 index 000000000..36d0f27e3 --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad4.yml @@ -0,0 +1,13 @@ +--- +- name: Hello Events + hosts: all + sources: + - ansible.eda.range: + limit: 5 + rules: + name: Say Hello + condition: event.i == 1 + action: + run_playbook: + name: ansible.eda.hello +... diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad5.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad5.yml new file mode 100644 index 000000000..4a5b7d65f --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad5.yml @@ -0,0 +1,12 @@ +--- +- name: Hello Events + hosts: all + sources: + - ansible.eda.range: + limit: 5 + rules: + - condition: event.i == 1 + action: + run_playbook: + name: ansible.eda.hello +... diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad6.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad6.yml new file mode 100644 index 000000000..26ebafffb --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad6.yml @@ -0,0 +1,13 @@ +--- +- name: Hello Events + hosts: all + sources: + - ansible.eda.range: + limit: 5 + rules: + - name: + condition: event.i == 1 + action: + run_playbook: + name: ansible.eda.hello +... diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad7.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad7.yml new file mode 100644 index 000000000..17c84777a --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad7.yml @@ -0,0 +1,12 @@ +--- +- name: Hello Events + hosts: all + sources: + - ansible.eda.range: + limit: 5 + rules: + - name: Say Hello + action: + run_playbook: + name: ansible.eda.hello +... diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad8.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad8.yml new file mode 100644 index 000000000..5690d9b9d --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad8.yml @@ -0,0 +1,13 @@ +--- +- name: Hello Events + hosts: all + sources: + - ansible.eda.range: + limit: 5 + rules: + - name: Say Hello + condition: + action: + run_playbook: + name: ansible.eda.hello +... diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad9.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad9.yml new file mode 100644 index 000000000..7018196a9 --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/bad9.yml @@ -0,0 +1,10 @@ +--- +- name: Hello Events + hosts: all + sources: + - ansible.eda.range: + limit: 5 + rules: + - name: Say Hello + condition: event.i == 1 +... diff --git a/tests/integration/services/data/project-05/extensions/eda/rulebooks/good.yml b/tests/integration/services/data/project-05/extensions/eda/rulebooks/good.yml new file mode 100644 index 000000000..e00b017eb --- /dev/null +++ b/tests/integration/services/data/project-05/extensions/eda/rulebooks/good.yml @@ -0,0 +1,13 @@ +--- +- name: Hello Events + hosts: all + sources: + - ansible.eda.range: + limit: 5 + rules: + - name: Say Hello + condition: event.i == 1 + action: + run_playbook: + name: ansible.eda.hello +... diff --git a/tests/integration/services/test_project_import.py b/tests/integration/services/test_project_import.py index 48734c63d..b490af54d 100644 --- a/tests/integration/services/test_project_import.py +++ b/tests/integration/services/test_project_import.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import json +import logging import os import re import shutil @@ -295,6 +296,50 @@ def clone_project(_url, path, *_args, **_kwargs): storage_save_patch.assert_not_called() +@pytest.mark.django_db +def test_project_import_with_invalid_rulebooks( + storage_save_patch, service_tempdir_patch, caplog +): + def clone_project(_url, path, *_args, **_kwargs): + src = DATA_DIR / "project-05" + shutil.copytree(src, path, symlinks=False) + return repo_mock + + repo_mock = mock.Mock(name="GitRepository()") + repo_mock.rev_parse.return_value = ( + "adc83b19e793491b1c6ea0fd8b46cd9f32e592fc" + ) + + git_mock = mock.Mock(name="GitRepository", spec=GitRepository) + git_mock.clone.side_effect = clone_project + + project = models.Project.objects.create( + name="test-project-04", url="https://git.example.com/repo.git" + ) + + logger = logging.getLogger("aap_eda") + propagate = logger.propagate + logger.propagate = True + caplog.set_level(logging.WARNING) + try: + service = ProjectImportService(git_cls=git_mock) + service.import_project(project) + finally: + logger.propagate = propagate + + assert project.git_hash == "adc83b19e793491b1c6ea0fd8b46cd9f32e592fc" + assert project.import_state == models.Project.ImportState.COMPLETED + assert caplog.text.count("WARNING") == 10 + + rulebooks = list(project.rulebook_set.order_by("name")) + assert len(rulebooks) == 1 + + with open(DATA_DIR / "project-05-import.json") as fp: + expected_rulebooks = json.load(fp) + for rulebook, expected in zip(rulebooks, expected_rulebooks): + assert_rulebook_is_valid(rulebook, expected) + + def assert_rulebook_is_valid(rulebook: models.Rulebook, expected: dict): assert rulebook.name == expected["name"]