From 65343977d3cc1c2662ef1fecf0b9cfcc81998cea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20V=C3=A1gner?= Date: Thu, 31 Oct 2024 16:01:12 +0100 Subject: [PATCH] Add link config validation --- .pre-commit-config.yaml | 2 + pyproject.toml | 1 + tests/unit/test_config.py | 134 ++++++++++++++++++++++++++++++++++ tests/unit/test_utils.py | 63 ++-------------- tmt/base.py | 3 +- tmt/cli.py | 3 +- tmt/config/__init__.py | 91 +++++++++++++++++++++++ tmt/config/models/__init__.py | 13 ++++ tmt/config/models/link.py | 20 +++++ tmt/templates/__init__.py | 3 +- tmt/trying.py | 3 +- tmt/utils/__init__.py | 67 ----------------- tmt/utils/jira.py | 51 +++---------- 13 files changed, 286 insertions(+), 168 deletions(-) create mode 100644 tests/unit/test_config.py create mode 100644 tmt/config/__init__.py create mode 100644 tmt/config/models/__init__.py create mode 100644 tmt/config/models/link.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index b21c98add2..d23ce81ecc 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -38,6 +38,7 @@ repos: - "jinja2>=2.11.3" # 3.1.2 / 3.1.2 - "packaging>=20" # 20 seems to be available with RHEL8 - "pint>=0.16.1" # 0.16.1 + - "pydantic>=1.10, <2.0" - "pygments>=2.7.4" # 2.7.4 is the current one available for RHEL9 - "requests>=2.25.1" # 2.28.2 / 2.31.0 - "ruamel.yaml>=0.16.6" # 0.17.32 / 0.17.32 @@ -86,6 +87,7 @@ repos: - "jinja2>=2.11.3" # 3.1.2 / 3.1.2 - "packaging>=20" # 20 seems to be available with RHEL8 - "pint>=0.16.1" # 0.16.1 / 0.19.x TODO: Pint 0.20 requires larger changes to tmt.hardware + - "pydantic>=1.10, <2.0" - "pygments>=2.7.4" # 2.7.4 is the current one available for RHEL9 - "requests>=2.25.1" # 2.28.2 / 2.31.0 - "ruamel.yaml>=0.16.6" # 0.17.32 / 0.17.32 diff --git a/pyproject.toml b/pyproject.toml index 0f125de355..380b6bf946 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -36,6 +36,7 @@ dependencies = [ # F39 / PyPI "jinja2>=2.11.3", # 3.1.2 / 3.1.2 "packaging>=20", # 20 seems to be available with RHEL8 "pint>=0.16.1", # 0.16.1 + "pydantic>=1.10, <2.0", "pygments>=2.7.4", # 2.7.4 is the current one available for RHEL9 "requests>=2.25.1", # 2.28.2 / 2.31.0 "ruamel.yaml>=0.16.6", # 0.17.32 / 0.17.32 diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py new file mode 100644 index 0000000000..73229f6752 --- /dev/null +++ b/tests/unit/test_config.py @@ -0,0 +1,134 @@ +import queue +import re +import textwrap +import threading +import unittest +import unittest.mock +from unittest.mock import MagicMock + +import fmf +import pytest + +import tmt.config +from tmt.utils import Path + + +@pytest.fixture +def config_path(tmppath: Path, monkeypatch) -> Path: + config_path = tmppath / 'config' + config_path.mkdir() + monkeypatch.setattr(tmt.config, 'effective_config_dir', MagicMock(return_value=config_path)) + return config_path + + +def test_config(config_path: Path): + """ Config smoke test """ + run = Path('/var/tmp/tmt/test') + config1 = tmt.config.Config() + config1.last_run = run + config2 = tmt.config.Config() + assert config2.last_run.resolve() == run.resolve() + + +def test_last_run_race(tmppath: Path, monkeypatch): + """ Race in last run symlink shouldn't be fatal """ + config_path = tmppath / 'config' + config_path.mkdir() + monkeypatch.setattr(tmt.config, 'effective_config_dir', MagicMock(return_value=config_path)) + mock_logger = unittest.mock.MagicMock() + monkeypatch.setattr(tmt.utils.log, 'warning', mock_logger) + config = tmt.config.Config() + results = queue.Queue() + threads = [] + + def create_last_run(config, counter): + try: + last_run_path = tmppath / f"run-{counter}" + last_run_path.mkdir() + val = config.last_run = last_run_path + results.put(val) + except Exception as err: + results.put(err) + + total = 20 + for i in range(total): + threads.append(threading.Thread(target=create_last_run, args=(config, i))) + for t in threads: + t.start() + for t in threads: + t.join() + + all_good = True + for _ in threads: + value = results.get() + if isinstance(value, Exception): + # Print exception for logging + print(value) + all_good = False + assert all_good + # Getting into race is not certain, do not assert + # assert mock_logger.called + assert config.last_run, "Some run was stored as last run" + + +def test_link_config_invalid(config_path: Path): + config_yaml = textwrap.dedent(""" + issue-tracker: + - type: jiRA + url: invalid_url + tmt-web-url: https:// + unknown: value + additional_key: + foo: bar + """).strip() + fmf.Tree.init(path=config_path) + (config_path / 'link.fmf').write_text(config_yaml) + + with pytest.raises(tmt.utils.MetadataError) as error: + _ = tmt.config.Config().link + + cause = str(error.value.__cause__) + assert '6 validation errors for LinkConfig' in cause + assert re.search(r'type\s*value is not a valid enumeration member', cause) + assert re.search(r'url\s*invalid or missing URL scheme', cause) + assert re.search(r'tmt-web-url\s*URL host invalid', cause) + assert re.search(r'unknown\s*extra fields not permitted', cause) + assert re.search(r'token\s*field required', cause) + assert re.search(r'additional_key\s*extra fields not permitted', cause) + + +def test_link_config_valid(config_path: Path): + config_yaml = textwrap.dedent(""" + issue-tracker: + - type: jira + url: https://issues.redhat.com + tmt-web-url: https://tmt-web-url.com + token: secret + """).strip() + fmf.Tree.init(path=config_path) + (config_path / 'link.fmf').write_text(config_yaml) + + link = tmt.config.Config().link + + assert link.issue_tracker[0].type == 'jira' + assert link.issue_tracker[0].url == 'https://issues.redhat.com' + assert link.issue_tracker[0].tmt_web_url == 'https://tmt-web-url.com' + assert link.issue_tracker[0].token == 'secret' + + +def test_link_config_missing(config_path: Path): + fmf.Tree.init(path=config_path) + + assert tmt.config.Config().link is None + + +def test_link_config_empty(config_path: Path): + fmf.Tree.init(path=config_path) + (config_path / 'link.fmf').touch() + + with pytest.raises(tmt.utils.SpecificationError) as error: + _ = tmt.config.Config().link + + cause = str(error.value.__cause__) + assert '1 validation error for LinkConfig' in cause + assert re.search(r'issue-tracker\s*field required', cause) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 22655866a0..9901f4f033 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -17,6 +17,7 @@ import pytest import tmt +import tmt.config import tmt.log import tmt.plugins import tmt.steps.discover @@ -204,56 +205,6 @@ def test_inject_auth_git_url(monkeypatch) -> None: inject_auth_git_url('https://example.com/broken/something') -def test_config(): - """ Config smoke test """ - run = Path('/var/tmp/tmt/test') - config1 = tmt.utils.Config() - config1.last_run = run - config2 = tmt.utils.Config() - assert config2.last_run.resolve() == run.resolve() - - -def test_last_run_race(tmppath: Path, monkeypatch): - """ Race in last run symlink shouldn't be fatal """ - config_path = tmppath / 'config' - config_path.mkdir() - monkeypatch.setattr(tmt.utils, 'effective_config_dir', MagicMock(return_value=config_path)) - mock_logger = unittest.mock.MagicMock() - monkeypatch.setattr(tmt.utils.log, 'warning', mock_logger) - config = tmt.utils.Config() - results = queue.Queue() - threads = [] - - def create_last_run(config, counter): - try: - last_run_path = tmppath / f"run-{counter}" - last_run_path.mkdir() - val = config.last_run = last_run_path - results.put(val) - except Exception as err: - results.put(err) - - total = 20 - for i in range(total): - threads.append(threading.Thread(target=create_last_run, args=(config, i))) - for t in threads: - t.start() - for t in threads: - t.join() - - all_good = True - for _ in threads: - value = results.get() - if isinstance(value, Exception): - # Print exception for logging - print(value) - all_good = False - assert all_good - # Getting into race is not certain, do not assert - # assert mock_logger.called - assert config.last_run, "Some run was stored as last run" - - def test_workdir_env_var(tmppath: Path, monkeypatch, root_logger): """ Test TMT_WORKDIR_ROOT environment variable """ # Cannot use monkeypatch.context() as it is not present for CentOS Stream 8 @@ -1737,9 +1688,9 @@ def tearDown(self): shutil.rmtree(self.tmp) @unittest.mock.patch('jira.JIRA.add_simple_link') - @unittest.mock.patch('tmt.utils.Config') + @unittest.mock.patch('tmt.config.Config.fmf_tree', new_callable=unittest.mock.PropertyMock) def test_jira_link_test_only(self, mock_config_tree, mock_add_simple_link) -> None: - mock_config_tree.return_value.fmf_tree = self.config_tree + mock_config_tree.return_value = self.config_tree test = tmt.Tree(logger=self.logger, path=self.tmp).tests(names=['tmp/test'])[0] tmt.utils.jira.link( tmt_objects=[test], @@ -1752,9 +1703,9 @@ def test_jira_link_test_only(self, mock_config_tree, mock_add_simple_link) -> No assert '&test-path=%2Ftests%2Funit%2Ftmp' in result['url'] @unittest.mock.patch('jira.JIRA.add_simple_link') - @unittest.mock.patch('tmt.utils.Config') + @unittest.mock.patch('tmt.config.Config.fmf_tree', new_callable=unittest.mock.PropertyMock) def test_jira_link_test_plan_story(self, mock_config_tree, mock_add_simple_link) -> None: - mock_config_tree.return_value.fmf_tree = self.config_tree + mock_config_tree.return_value = self.config_tree test = tmt.Tree(logger=self.logger, path=self.tmp).tests(names=['tmp/test'])[0] plan = tmt.Tree(logger=self.logger, path=self.tmp).plans(names=['tmp'])[0] story = tmt.Tree(logger=self.logger, path=self.tmp).stories(names=['tmp'])[0] @@ -1778,9 +1729,9 @@ def test_jira_link_test_plan_story(self, mock_config_tree, mock_add_simple_link) assert '&story-path=%2Ftests%2Funit%2Ftmp' in result['url'] @unittest.mock.patch('jira.JIRA.add_simple_link') - @unittest.mock.patch('tmt.utils.Config') + @unittest.mock.patch('tmt.config.Config.fmf_tree', new_callable=unittest.mock.PropertyMock) def test_create_link_relation(self, mock_config_tree, mock_add_simple_link) -> None: - mock_config_tree.return_value.fmf_tree = self.config_tree + mock_config_tree.return_value = self.config_tree test = tmt.Tree(logger=self.logger, path=self.tmp).tests(names=['tmp/test'])[0] tmt.utils.jira.link( tmt_objects=[test], diff --git a/tmt/base.py b/tmt/base.py index 21999409e2..d7ee58892a 100644 --- a/tmt/base.py +++ b/tmt/base.py @@ -36,6 +36,7 @@ import tmt.base import tmt.checks +import tmt.config import tmt.convert import tmt.export import tmt.frameworks @@ -3363,7 +3364,7 @@ def __init__(self, logger: tmt.log.Logger) -> None: """ Initialize tree, workdir and plans """ # Use the last run id if requested - self.config = tmt.utils.Config() + self.config = tmt.config.Config() if cli_invocation is not None: if cli_invocation.options.get('last'): diff --git a/tmt/cli.py b/tmt/cli.py index 70b30e3738..8cb17bded7 100644 --- a/tmt/cli.py +++ b/tmt/cli.py @@ -16,6 +16,7 @@ import tmt import tmt.base +import tmt.config import tmt.convert import tmt.export import tmt.identifier @@ -2171,7 +2172,7 @@ def completion(**kwargs: Any) -> None: def setup_completion(shell: str, install: bool, context: Context) -> None: """ Setup completion based on the shell """ - config = tmt.utils.Config() + config = tmt.config.Config() # Fish gets installed into its special location where it is automatically # loaded. if shell == 'fish': diff --git a/tmt/config/__init__.py b/tmt/config/__init__.py new file mode 100644 index 0000000000..adbff3d4c1 --- /dev/null +++ b/tmt/config/__init__.py @@ -0,0 +1,91 @@ +import functools +import os +from contextlib import suppress +from typing import Optional, cast + +import fmf +import fmf.utils +from pydantic import ValidationError + +import tmt.utils +from tmt._compat.pathlib import Path +from tmt.config.models.link import LinkConfig + +# Config directory +DEFAULT_CONFIG_DIR = Path('~/.config/tmt') + + +def effective_config_dir() -> Path: + """ + Find out what the actual config directory is. + + If ``TMT_CONFIG_DIR`` variable is set, it is used. Otherwise, + :py:const:`DEFAULT_CONFIG_DIR` is picked. + """ + + if 'TMT_CONFIG_DIR' in os.environ: + return Path(os.environ['TMT_CONFIG_DIR']).expanduser() + + return DEFAULT_CONFIG_DIR.expanduser() + + +class Config: + """ User configuration """ + + def __init__(self) -> None: + """ Initialize config directory path """ + self.path = effective_config_dir() + self.logger = tmt.utils.log + + try: + self.path.mkdir(parents=True, exist_ok=True) + except OSError as error: + raise tmt.utils.GeneralError( + f"Failed to create config '{self.path}'.") from error + + @property + def _last_run_symlink(self) -> Path: + return self.path / 'last-run' + + @property + def last_run(self) -> Optional[Path]: + """ Get the last run workdir path """ + return self._last_run_symlink.resolve() if self._last_run_symlink.is_symlink() else None + + @last_run.setter + def last_run(self, workdir: Path) -> None: + """ Set the last run to the given run workdir """ + + with suppress(OSError): + self._last_run_symlink.unlink() + + try: + self._last_run_symlink.symlink_to(workdir) + except FileExistsError: + # Race when tmt runs in parallel + self.logger.warning( + f"Unable to mark '{workdir}' as the last run, " + "'tmt run --last' might not pick the right run directory.") + except OSError as error: + raise tmt.utils.GeneralError( + f"Unable to save last run '{self.path}'.\n{error}") + + @functools.cached_property + def fmf_tree(self) -> fmf.Tree: + """ Return the configuration tree """ + try: + return fmf.Tree(self.path) + except fmf.utils.RootError as error: + raise tmt.utils.MetadataError(f"Config tree not found in '{self.path}'.") from error + + @property + def link(self) -> Optional[LinkConfig]: + """ Return the link configuration, if present. """ + link_config = cast(Optional[fmf.Tree], self.fmf_tree.find('/link')) + if not link_config: + return None + try: + return LinkConfig.parse_obj(link_config.data) + except ValidationError as error: + raise tmt.utils.SpecificationError( + f"Invalid link configuration in '{link_config.name}'.") from error diff --git a/tmt/config/models/__init__.py b/tmt/config/models/__init__.py new file mode 100644 index 0000000000..a7dbb88727 --- /dev/null +++ b/tmt/config/models/__init__.py @@ -0,0 +1,13 @@ +from pydantic import BaseModel, Extra + + +def create_alias(name: str) -> str: + return name.replace('_', '-') + + +class BaseConfig(BaseModel): + class Config: + # Accept only keys with dashes instead of underscores + alias_generator = create_alias + extra = Extra.forbid + validate_assignment = True diff --git a/tmt/config/models/link.py b/tmt/config/models/link.py new file mode 100644 index 0000000000..27d8a5b324 --- /dev/null +++ b/tmt/config/models/link.py @@ -0,0 +1,20 @@ +from enum import Enum + +from pydantic import HttpUrl + +from . import BaseConfig + + +class IssueTrackerType(str, Enum): + jira = 'jira' + + +class IssueTracker(BaseConfig): + type: IssueTrackerType + url: HttpUrl + tmt_web_url: HttpUrl + token: str + + +class LinkConfig(BaseConfig): + issue_tracker: list[IssueTracker] diff --git a/tmt/templates/__init__.py b/tmt/templates/__init__.py index 9f7ffadc85..26fcb71f40 100644 --- a/tmt/templates/__init__.py +++ b/tmt/templates/__init__.py @@ -2,11 +2,12 @@ from typing import Any, Optional import tmt +import tmt.config import tmt.utils import tmt.utils.templates from tmt.utils import Path -DEFAULT_CUSTOM_TEMPLATES_PATH = tmt.utils.Config().path / 'templates' +DEFAULT_CUSTOM_TEMPLATES_PATH = tmt.config.Config().path / 'templates' DEFAULT_PLAN_NAME = "/default/plan" INIT_TEMPLATES = ['mini', 'base', 'full'] TEMPLATE_FILE_SUFFIX = '.j2' diff --git a/tmt/trying.py b/tmt/trying.py index e0f7f9b484..4ba4c2d702 100644 --- a/tmt/trying.py +++ b/tmt/trying.py @@ -12,6 +12,7 @@ import tmt import tmt.base +import tmt.config import tmt.log import tmt.steps import tmt.steps.execute @@ -164,7 +165,7 @@ def get_default_plans(self, run: tmt.base.Run) -> list[Plan]: # plans starting with the default user plan name (there might be # more than just one). try: - config_tree = tmt.utils.Config().fmf_tree + config_tree = tmt.config.Config().fmf_tree plan_name = re.escape(USER_PLAN_NAME) # cast: once fmf is properly annotated, cast() would not be needed. # pyright isn't able to infer the type. diff --git a/tmt/utils/__init__.py b/tmt/utils/__init__.py index c1b258ba8d..1a2c916ac9 100644 --- a/tmt/utils/__init__.py +++ b/tmt/utils/__init__.py @@ -26,7 +26,6 @@ import urllib.parse from collections import Counter from collections.abc import Iterable, Iterator, Sequence -from contextlib import suppress from math import ceil from re import Pattern from threading import Thread @@ -146,23 +145,6 @@ def configure_constant(default: int, envvar: str) -> int: DEFAULT_PLUGIN_ORDER_REQUIRES = 70 DEFAULT_PLUGIN_ORDER_RECOMMENDS = 75 -# Config directory -DEFAULT_CONFIG_DIR = Path('~/.config/tmt') - - -def effective_config_dir() -> Path: - """ - Find out what the actual config directory is. - - If ``TMT_CONFIG_DIR`` variable is set, it is used. Otherwise, - :py:const:`DEFAULT_CONFIG_DIR` is picked. - """ - - if 'TMT_CONFIG_DIR' in os.environ: - return Path(os.environ['TMT_CONFIG_DIR']).expanduser() - - return DEFAULT_CONFIG_DIR.expanduser() - # Special process return codes @@ -869,55 +851,6 @@ def as_environ(self) -> Iterator[None]: ] -class Config: - """ User configuration """ - - def __init__(self) -> None: - """ Initialize config directory path """ - self.path = effective_config_dir() - - try: - self.path.mkdir(parents=True, exist_ok=True) - except OSError as error: - raise GeneralError( - f"Failed to create config '{self.path}'.") from error - - @property - def _last_run_symlink(self) -> Path: - return self.path / 'last-run' - - @property - def last_run(self) -> Optional[Path]: - """ Get the last run workdir path """ - return self._last_run_symlink.resolve() if self._last_run_symlink.is_symlink() else None - - @last_run.setter - def last_run(self, workdir: Path) -> None: - """ Set the last run to the given run workdir """ - - with suppress(OSError): - self._last_run_symlink.unlink() - - try: - self._last_run_symlink.symlink_to(workdir) - except FileExistsError: - # Race when tmt runs in parallel - log.warning( - f"Unable to mark '{workdir}' as the last run, " - "'tmt run --last' might not pick the right run directory.") - except OSError as error: - raise GeneralError( - f"Unable to save last run '{self.path}'.\n{error}") - - @functools.cached_property - def fmf_tree(self) -> fmf.Tree: - """ Return the configuration tree """ - try: - return fmf.Tree(self.path) - except fmf.utils.RootError as error: - raise MetadataError(f"Config tree not found in '{self.path}'.") from error - - # TODO: `StreamLogger` is a dedicated thread following given stream, passing their content to # tmt's logging methods. Thread is needed because of some amount of blocking involved in the # process, but it has a side effect of `NO_COLOR` envvar being ignored. When tmt spots `NO_COLOR` diff --git a/tmt/utils/jira.py b/tmt/utils/jira.py index b52fba34b0..b657934f53 100644 --- a/tmt/utils/jira.py +++ b/tmt/utils/jira.py @@ -5,16 +5,15 @@ import fmf.utils import tmt.base +import tmt.config import tmt.log import tmt.utils +from tmt.config.models.link import IssueTracker, IssueTrackerType from tmt.plugins import ModuleImporter if TYPE_CHECKING: import jira -# Config section item for issue trackers -IssueTracker = dict[Any, Any] - # Test, plan or story TmtObject = Union['tmt.base.Test', 'tmt.base.Plan', 'tmt.base.Story'] @@ -55,16 +54,9 @@ class JiraInstance: def __init__(self, issue_tracker: IssueTracker, logger: tmt.log.Logger): """ Initialize Jira instance from the issue tracker config """ - def assert_string(key: str) -> str: - value = issue_tracker.get(key) - if not isinstance(value, str): - raise tmt.utils.GeneralError( - f"Invalid '{key}' value '{value}' in issue tracker config.") - return value - - self.url: str = assert_string("url") - self.tmt_web_url: str = assert_string("tmt-web-url") - self.token: str = assert_string("token") + self.url = str(issue_tracker.url) + self.tmt_web_url = str(issue_tracker.tmt_web_url) + self.token = issue_tracker.token self.logger = logger jira_module = import_jira(logger) @@ -84,44 +76,21 @@ def from_issue_url( # Check for the 'link' config section, exit if config missing try: - config_tree = tmt.utils.Config().fmf_tree - link_config = cast(Optional[fmf.Tree], config_tree.find('/link')) + link_config = tmt.config.Config().link except tmt.utils.MetadataError: return None if not link_config: return None - # Check the list of configured issues trackers - issue_trackers: Any = link_config.data.get('issue-tracker') - - if not issue_trackers: - raise tmt.utils.GeneralError( - "No 'issue-tracker' section found in the 'link' config.") - - if not isinstance(issue_trackers, list): - raise tmt.utils.GeneralError( - "The 'issue-tracker' section should be a 'list'.") - # Find Jira instance matching the issue url - issue_tracker: Any - for issue_tracker in issue_trackers: - if not isinstance(issue_tracker, dict): - raise tmt.utils.GeneralError( - "Issue tracker config should be a 'dict'.") - + for issue_tracker in link_config.issue_tracker: # Tracker type must match - issue_tracker_type: Any = issue_tracker.get("type") - if not isinstance(issue_tracker_type, str) or issue_tracker_type != "jira": + if issue_tracker.type != IssueTrackerType.jira: continue # Issue url must match - jira_server_url: Any = issue_tracker.get("url") - if not isinstance(jira_server_url, str): - raise tmt.utils.GeneralError( - "Issue tracker 'url' should be a string.") - - if issue_url.startswith(jira_server_url): - return JiraInstance(cast(IssueTracker, issue_tracker), logger=logger) + if issue_url.startswith(issue_tracker.url): + return JiraInstance(issue_tracker, logger=logger) return None