diff --git a/py/envoy.base.utils/envoy/base/utils/abstract/project/changelog.py b/py/envoy.base.utils/envoy/base/utils/abstract/project/changelog.py index ec79718d22..a941a4bc83 100644 --- a/py/envoy.base.utils/envoy/base/utils/abstract/project/changelog.py +++ b/py/envoy.base.utils/envoy/base/utils/abstract/project/changelog.py @@ -31,6 +31,7 @@ CHANGELOG_CURRENT_DIR_PATH = "changelogs/current" CHANGELOG_ENTRY_GLOB = "*/*.rst" CHANGELOG_SECTIONS_PATH = "changelogs/sections.yaml" +CHANGELOG_AREAS_PATH = "changelogs/areas.yaml" ENTRY_SEPARATOR = "__" CHANGELOG_SUMMARY_PATH = "changelogs/summary.md" CHANGELOG_URL_TPL = ( @@ -331,6 +332,19 @@ def sections(self) -> typing.ChangelogSectionsDict: f"({self.sections_path})\n{e}") return cast(typing.ChangelogSectionsDict, e.value) + @cached_property + def areas(self) -> typing.ChangelogAreasDict: + if not self.areas_path.exists(): + return cast(typing.ChangelogAreasDict, {}) + try: + return utils.from_yaml( + self.areas_path, + typing.ChangelogAreasDict) + except (_yaml.reader.ReaderError, utils.TypeCastingError) as e: + raise exceptions.ChangelogError( + "Failed to parse changelog areas " + f"({self.areas_path}): {e}") + def validate_sections( self, data: typing.ChangelogDict, @@ -360,6 +374,10 @@ def validate_sections( def sections_path(self) -> pathlib.Path: return self.project.path.joinpath(CHANGELOG_SECTIONS_PATH) + @property + def areas_path(self) -> pathlib.Path: + return self.project.path.joinpath(CHANGELOG_AREAS_PATH) + @property def summary_path(self) -> pathlib.Path: return self.project.path.joinpath(CHANGELOG_SUMMARY_PATH) diff --git a/py/envoy.base.utils/envoy/base/utils/interface.py b/py/envoy.base.utils/envoy/base/utils/interface.py index f9d0a5643e..777219d7d5 100644 --- a/py/envoy.base.utils/envoy/base/utils/interface.py +++ b/py/envoy.base.utils/envoy/base/utils/interface.py @@ -233,6 +233,12 @@ async def is_pending(self) -> bool: `Pending`.""" raise NotImplementedError + @property + @abstracts.interfacemethod + def areas(self) -> typing.ChangelogAreasDict: + """Changelog areas.""" + raise NotImplementedError + @property @abstracts.interfacemethod def sections(self) -> typing.ChangelogSectionsDict: diff --git a/py/envoy.base.utils/envoy/base/utils/typing.py b/py/envoy.base.utils/envoy/base/utils/typing.py index 31f1b1062b..61d52367a0 100644 --- a/py/envoy.base.utils/envoy/base/utils/typing.py +++ b/py/envoy.base.utils/envoy/base/utils/typing.py @@ -54,6 +54,13 @@ class ChangelogSectionDict(BaseChangelogSectionDict, total=False): ChangelogSectionsDict = dict[str, ChangelogSectionDict] + +class ChangelogAreaDict(BaseChangelogSectionDict): + pass + + +ChangelogAreasDict = dict[str, ChangelogAreaDict] + VersionConfigDict = dict[str, str] diff --git a/py/envoy.base.utils/tests/test_abstract_project_changelogs.py b/py/envoy.base.utils/tests/test_abstract_project_changelogs.py index 5007776e28..cb2e96a2a6 100644 --- a/py/envoy.base.utils/tests/test_abstract_project_changelogs.py +++ b/py/envoy.base.utils/tests/test_abstract_project_changelogs.py @@ -387,6 +387,51 @@ def test_abstract_changelogs_sections(patches, raises): f"({m_path.return_value}): {str(error)}")) +@pytest.mark.parametrize("exists", [True, False]) +@pytest.mark.parametrize( + "raises", + [None, Exception, yaml.reader.ReaderError, exceptions.TypeCastingError]) +def test_abstract_changelogs_areas(patches, exists, raises): + changelogs = DummyChangelogs("PROJECT") + patched = patches( + "utils.from_yaml", + ("AChangelogs.areas_path", + dict(new_callable=PropertyMock)), + prefix="envoy.base.utils.abstract.project.changelog") + + with patched as (m_yaml, m_path): + m_path.return_value.exists.return_value = exists + if exists and raises: + error = raises("AN ERROR OCCURRED", 7, 23, "Y", "Z") + m_yaml.side_effect = error + if not exists: + assert changelogs.areas == {} + elif raises == Exception: + with pytest.raises(Exception): + changelogs.areas + elif raises in (yaml.reader.ReaderError, exceptions.TypeCastingError): + with pytest.raises(exceptions.ChangelogError) as e: + changelogs.areas + else: + assert changelogs.areas == m_yaml.return_value + + assert ( + ("areas" in changelogs.__dict__) + == (not raises or not exists)) + if not exists: + assert not m_yaml.called + return + assert ( + m_yaml.call_args + == [(m_path.return_value, typing.ChangelogAreasDict), {}]) + if raises not in (yaml.reader.ReaderError, exceptions.TypeCastingError): + return + assert ( + e.value.args[0] + == ("Failed to parse changelog areas " + f"({m_path.return_value}): {str(error)}")) + + def test_abstract_changelogs_sections_path(): project = MagicMock() changelogs = DummyChangelogs(project) @@ -399,6 +444,18 @@ def test_abstract_changelogs_sections_path(): assert "sections_path" not in changelogs.__dict__ +def test_abstract_changelogs_areas_path(): + project = MagicMock() + changelogs = DummyChangelogs(project) + assert ( + changelogs.areas_path + == project.path.joinpath.return_value) + assert ( + project.path.joinpath.call_args + == [(abstract.project.changelog.CHANGELOG_AREAS_PATH, ), {}]) + assert "areas_path" not in changelogs.__dict__ + + def test_abstract_changelogs_validate_sections(): changelogs = DummyChangelogs("PROJECT") changelogs.sections = {"known": {"title": "Known"}} diff --git a/py/envoy.code.check/envoy/code/check/abstract/changelog.py b/py/envoy.code.check/envoy/code/check/abstract/changelog.py index 3e5f16c79c..f677f4c68d 100644 --- a/py/envoy.code.check/envoy/code/check/abstract/changelog.py +++ b/py/envoy.code.check/envoy/code/check/abstract/changelog.py @@ -1,6 +1,7 @@ import itertools import pathlib +import re from datetime import datetime from functools import cached_property from collections.abc import Iterator @@ -20,6 +21,15 @@ MAX_VERSION_FOR_CHANGES_SECTION = "1.16" +try: + from envoy.base.utils.abstract.project.changelog import ( + CHANGELOG_AREAS_PATH, + ) +except ImportError: + CHANGELOG_AREAS_PATH = "changelogs/areas.yaml" +VALID_CHANGELOG_AREA_RE = re.compile(r"^[a-z0-9_\-/]+$") +VALID_CHANGELOG_AREA_PATTERN = r"[a-z0-9_\-/]+" +CHANGELOG_AREAS_FILE = pathlib.Path(CHANGELOG_AREAS_PATH) @abstracts.implementer(interface.IChangelogChangesChecker) @@ -30,8 +40,10 @@ class AChangelogChangesChecker(metaclass=abstracts.Abstraction): def __init__( self, - sections: utils.typing.ChangelogSectionsDict) -> None: + sections: utils.typing.ChangelogSectionsDict, + areas: "utils.typing.ChangelogAreasDict") -> None: self.sections = sections + self.areas = areas @property # type:ignore @abstracts.interfacemethod @@ -120,10 +132,41 @@ def check_entry_filename( area, slug = path.stem.split(ENTRY_SEPARATOR, 1) if not area: return f"{path}: Area part of filename is empty" + if self.areas and area not in self.areas: + return ( + f"{path}: Invalid area '{area}'. " + f"Valid areas come from {CHANGELOG_AREAS_PATH}") if not slug: return f"{path}: Slug part of filename is empty" return None + def check_areas_file(self) -> tuple[str, ...]: + if not self.areas: + return () + title_areas: dict[str, list[str]] = {} + errors = [] + for area, area_data in self.areas.items(): + title = area_data["title"] + title_areas.setdefault(title, []).append(area) + if not VALID_CHANGELOG_AREA_RE.match(area): + errors.append( + f"{CHANGELOG_AREAS_FILE}: " + f"Invalid area key '{area}' " + f"(must match {VALID_CHANGELOG_AREA_PATTERN})") + if not VALID_CHANGELOG_AREA_RE.match(title): + errors.append( + f"{CHANGELOG_AREAS_FILE}: " + f"Invalid title '{title}' for area '{area}' " + f"(must match {VALID_CHANGELOG_AREA_PATTERN})") + for title, areas in sorted(title_areas.items()): + if len(areas) < 2: + continue + errors.append( + f"{CHANGELOG_AREAS_FILE}: " + f"Duplicate title '{title}' used by areas: " + f"{', '.join(sorted(areas))}") + return tuple(errors) + def check_entry_content( self, path: pathlib.Path) -> str | None: @@ -196,15 +239,17 @@ def entry_dir(self) -> pathlib.Path | None: @async_property(cache=True) async def errors(self) -> tuple[str, ...]: + areas_errors = await self.check_areas_file() entry_errors = await self.check_entry_files() try: return ( *self.check_version(), *await self.check_date(), *await self.check_sections(), + *areas_errors, *entry_errors) except utils.exceptions.ChangelogParseError as e: - return (*entry_errors, f"{self.version}: {e}") + return (*areas_errors, *entry_errors, f"{self.version}: {e}") @async_property async def invalid_date(self) -> str | None: @@ -286,6 +331,12 @@ async def check_entry_files(self) -> tuple[str, ...]: self.checker.check_entry_files, paths) + async def check_areas_file(self) -> tuple[str, ...]: + areas = self.project.changelogs.areas + if not self.is_current or not areas: + return () + return await self.project.execute(self.checker.check_areas_file) + def check_version(self) -> tuple[str, ...]: errors = [] if self.duplicate_current: @@ -322,7 +373,8 @@ def changes_checker_class( @cached_property def changes_checker(self) -> interface.IChangelogChangesChecker: return self.changes_checker_class( - self.project.changelogs.sections) + self.project.changelogs.sections, + self.project.changelogs.areas) @property # type:ignore @abstracts.interfacemethod diff --git a/py/envoy.code.check/envoy/code/check/interface.py b/py/envoy.code.check/envoy/code/check/interface.py index da5fac7fe8..12b6afefa7 100644 --- a/py/envoy.code.check/envoy/code/check/interface.py +++ b/py/envoy.code.check/envoy/code/check/interface.py @@ -150,6 +150,10 @@ def check_entry_content( path: pathlib.Path) -> str | None: raise NotImplementedError + @abstracts.interfacemethod + def check_areas_file(self) -> tuple[str, ...]: + raise NotImplementedError + @abstracts.interfacemethod def check_entry_files( self, diff --git a/py/envoy.code.check/tests/test_abstract_changelog.py b/py/envoy.code.check/tests/test_abstract_changelog.py index b7a96ff1ee..18084cba63 100644 --- a/py/envoy.code.check/tests/test_abstract_changelog.py +++ b/py/envoy.code.check/tests/test_abstract_changelog.py @@ -81,7 +81,8 @@ def test_changelogcheck_changes_checker(patches): assert ( m_class.return_value.call_args - == [(project.changelogs.sections, ), {}]) + == [(project.changelogs.sections, + project.changelogs.areas), {}]) assert "changes_checker" in changelog.__dict__ @@ -257,20 +258,29 @@ async def test_changelogstatus_errors(iters, patches, raises): ("AChangelogStatus.version", dict(new_callable=PropertyMock)), "AChangelogStatus.check_date", + "AChangelogStatus.check_areas_file", "AChangelogStatus.check_entry_files", "AChangelogStatus.check_sections", "AChangelogStatus.check_version", prefix="envoy.code.check.abstract.changelog") version_errors = iters(cb=lambda i: f"V{i}") date_errors = iters(cb=lambda i: f"D{i}") + areas_errors = iters(cb=lambda i: f"A{i}") sections_errors = iters(cb=lambda i: f"S{i}") entry_files_errors = iters(cb=lambda i: f"E{i}") - with patched as (m_version, m_date, m_entry_files, m_sections, m_versions): + with patched as ( + m_version, + m_date, + m_areas, + m_entry_files, + m_sections, + m_versions): if raises: error = raises("AN ERROR OCCURRED") m_date.side_effect = error m_date.return_value = date_errors + m_areas.return_value = areas_errors m_entry_files.return_value = entry_files_errors m_sections.return_value = sections_errors m_versions.return_value = version_errors @@ -280,9 +290,11 @@ async def test_changelogstatus_errors(iters, patches, raises): return expected_errors = ( (*version_errors, *date_errors, *sections_errors, + *areas_errors, *entry_files_errors) if not raises else ( + *areas_errors, *entry_files_errors, f"{m_version.return_value}: {error}")) assert ( @@ -292,7 +304,7 @@ async def test_changelogstatus_errors(iters, patches, raises): status, check.AChangelogStatus.errors.cache_name)["errors"]) - providers = [m_versions, m_date, m_entry_files] + providers = [m_versions, m_date, m_areas, m_entry_files] if not raises: providers.append(m_sections) for provider in providers: @@ -585,8 +597,44 @@ async def test_changelogstatus_check_sections(patches): sections.return_value), {}]) +@pytest.mark.parametrize("is_current", [True, False]) +@pytest.mark.parametrize("has_areas", [True, False]) +async def test_changelogstatus_check_areas_file( + patches, is_current, has_areas): + checker = MagicMock() + status = check.AChangelogStatus(checker, MagicMock()) + patched = patches( + ("AChangelogStatus.is_current", + dict(new_callable=PropertyMock)), + ("AChangelogStatus.project", + dict(new_callable=PropertyMock)), + prefix="envoy.code.check.abstract.changelog") + + with patched as (m_current, m_project): + m_current.return_value = is_current + m_project.return_value.changelogs.areas = ( + {"known": {"title": "known"}} + if has_areas + else {}) + m_project.return_value.execute = AsyncMock() + result = await status.check_areas_file() + + if not is_current or not has_areas: + assert result == () + assert not m_project.return_value.execute.called + return + + assert result == m_project.return_value.execute.return_value + assert ( + m_project.return_value.execute.call_args + == [(status.checker.check_areas_file, ), {}]) + + class DummyChangelogChangesChecker(check.AChangelogChangesChecker): + def __init__(self, sections, areas=None): + super().__init__(sections, {} if areas is None else areas) + @property def change_checkers(self): return super().change_checkers @@ -598,6 +646,7 @@ def test_changeschecker_constructor(): changelog = DummyChangelogChangesChecker("SECTIONS") assert changelog.sections == "SECTIONS" + assert changelog.areas == {} with pytest.raises(NotImplementedError): changelog.change_checkers @@ -792,7 +841,10 @@ def test_changeschecker_check_section_name( ("Slug", "empty"))]) def test_changeschecker_check_entry_filename( section, stem, suffix, expected): - changelog = DummyChangelogChangesChecker({"bug_fixes": MagicMock()}) + changelog = DummyChangelogChangesChecker( + {"bug_fixes": MagicMock()}, + {"myarea": {"title": "myarea"}, + "area": {"title": "area"}}) path = MagicMock() path.parent.name = section path.stem = stem @@ -808,6 +860,88 @@ def test_changeschecker_check_entry_filename( assert substring in result +def test_changeschecker_check_entry_filename_invalid_area(): + changelog = DummyChangelogChangesChecker( + {"bug_fixes": MagicMock()}, + {"some_other_area": {"title": "some_other_area"}}) + path = MagicMock() + path.parent.name = "bug_fixes" + path.stem = "myarea__myslug" + path.suffix = ".rst" + + result = changelog.check_entry_filename(path) + + assert result is not None + assert "Invalid area" in result + assert "areas.yaml" in result + + +def test_changeschecker_check_entry_filename_without_areas(): + changelog = DummyChangelogChangesChecker({"bug_fixes": MagicMock()}, {}) + path = MagicMock() + path.parent.name = "bug_fixes" + path.stem = "myarea__myslug" + path.suffix = ".rst" + + assert changelog.check_entry_filename(path) is None + + +def test_changeschecker_check_areas_file_empty(): + changelog = DummyChangelogChangesChecker("SECTIONS", {}) + assert changelog.check_areas_file() == () + + +def test_changeschecker_check_areas_file_duplicate_titles(): + changelog = DummyChangelogChangesChecker( + "SECTIONS", + {"baz": {"title": "dup"}, + "bar": {"title": "dup"}}) + assert ( + changelog.check_areas_file() + == ( + "changelogs/areas.yaml: Duplicate title " + "'dup' used by areas: bar, baz", + )) + + +def test_changeschecker_check_areas_file_invalid_titles(): + changelog = DummyChangelogChangesChecker( + "SECTIONS", + {"a": {"title": "BAD"}, + "b": {"title": "bad.dot"}}) + assert ( + changelog.check_areas_file() + == ( + "changelogs/areas.yaml: Invalid title 'BAD' " + "for area 'a' (must match [a-z0-9_\\-/]+)", + "changelogs/areas.yaml: Invalid title 'bad.dot' " + "for area 'b' (must match [a-z0-9_\\-/]+)", + )) + + +def test_changeschecker_check_areas_file_invalid_keys(): + changelog = DummyChangelogChangesChecker( + "SECTIONS", + {"Bad": {"title": "bad"}, + "bad.dot": {"title": "also_bad"}}) + assert ( + changelog.check_areas_file() + == ( + "changelogs/areas.yaml: Invalid area key 'Bad' " + "(must match [a-z0-9_\\-/]+)", + "changelogs/areas.yaml: Invalid area key 'bad.dot' " + "(must match [a-z0-9_\\-/]+)", + )) + + +def test_changeschecker_check_areas_file_valid(): + changelog = DummyChangelogChangesChecker( + "SECTIONS", + {"my_area-1/sub": {"title": "my_area-1/sub"}, + "other": {"title": "other"}}) + assert changelog.check_areas_file() == () + + @pytest.mark.parametrize( "content,expected", [("", ("empty", )), diff --git a/py/envoy.code.check/tests/test_checker.py b/py/envoy.code.check/tests/test_checker.py index 06eb68d686..bb39fd60cc 100644 --- a/py/envoy.code.check/tests/test_checker.py +++ b/py/envoy.code.check/tests/test_checker.py @@ -115,7 +115,7 @@ def test_changelog_check_constructor(patches): def test_changeschecker_checkers(patches): - checker = check.ChangelogChangesChecker("SECTIONS") + checker = check.ChangelogChangesChecker("SECTIONS", {}) patched = patches( "BackticksCheck", "PunctuationCheck",