From 3bc70265e982decbdc939503ae38fa993467b855 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Tue, 19 Jul 2022 16:57:42 +0200 Subject: [PATCH 1/5] Remove no-longer needed pylint ignores for type annotations Recent pylint (or Python+pylint) versions can deal with `Optional[str]` and similar type annotations without requiring the pylint annotation `# pylint: disable=unsubscriptable-object`. --- commodore/cluster.py | 2 -- commodore/helpers.py | 1 - commodore/inventory/__init__.py | 4 ---- commodore/postprocess/__init__.py | 4 ---- 4 files changed, 11 deletions(-) diff --git a/commodore/cluster.py b/commodore/cluster.py index c73708cb..332bfbc9 100644 --- a/commodore/cluster.py +++ b/commodore/cluster.py @@ -130,7 +130,6 @@ def render_target( inv: Inventory, target: str, components: dict[str, Component], - # pylint: disable=unsubscriptable-object component: Optional[str] = None, ): if not component: @@ -180,7 +179,6 @@ def render_target( } -# pylint: disable=unsubscriptable-object def update_target(cfg: Config, target: str, component: Optional[str] = None): click.secho(f"Updating Kapitan target for {target}...", bold=True) file = cfg.inventory.target_file(target) diff --git a/commodore/helpers.py b/commodore/helpers.py index 79737229..62da9432 100644 --- a/commodore/helpers.py +++ b/commodore/helpers.py @@ -222,7 +222,6 @@ def rm_tree_contents(basedir): os.unlink(f) -# pylint: disable=unsubscriptable-object def relsymlink(src: P, dest_dir: P, dest_name: Optional[str] = None): if dest_name is None: dest_name = src.name diff --git a/commodore/inventory/__init__.py b/commodore/inventory/__init__.py index 45f2c59d..f59fd2e5 100644 --- a/commodore/inventory/__init__.py +++ b/commodore/inventory/__init__.py @@ -80,15 +80,12 @@ def tenant_config_dir(self, tenant: str) -> P: def package_dir(self, pkgname: str) -> P: return self.classes_dir / pkgname - # pylint: disable=unsubscriptable-object def component_file(self, component: Union[Component, str]) -> P: return self.components_dir / f"{_component_name(component)}.yml" - # pylint: disable=unsubscriptable-object def defaults_file(self, component: Union[Component, str]) -> P: return self.defaults_dir / f"{_component_name(component)}.yml" - # pylint: disable=unsubscriptable-object def target_file(self, target: Union[Component, str]) -> P: return self.targets_dir / f"{_component_name(target)}.yml" @@ -101,7 +98,6 @@ def ensure_dirs(self): makedirs(self.targets_dir, exist_ok=True) -# pylint: disable=unsubscriptable-object def _component_name(component: Union[Component, str]) -> str: if isinstance(component, Component): return component.name diff --git a/commodore/postprocess/__init__.py b/commodore/postprocess/__init__.py index 97faa043..d2d8fff0 100644 --- a/commodore/postprocess/__init__.py +++ b/commodore/postprocess/__init__.py @@ -36,18 +36,14 @@ class Filter: filterargs: dict enabled: bool - # PyLint complains about ClassVar not being subscriptable - # pylint: disable=unsubscriptable-object _run_handlers: ClassVar[dict[str, FilterFunc]] = { "builtin": run_builtin_filter, "jsonnet": run_jsonnet_filter, } - # pylint: disable=unsubscriptable-object _validate_handlers: ClassVar[dict[str, ValidateFunc]] = { "builtin": validate_builtin_filter, "jsonnet": validate_jsonnet_filter, } - # pylint: disable=unsubscriptable-object _required_keys: ClassVar[set[str]] = {"type", "path", "filter"} def __init__(self, fd: dict): From a154173322893a8a09b93077cdde63b2724b9569 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Mon, 18 Jul 2022 18:18:25 +0200 Subject: [PATCH 2/5] WIP - Support different versions per alias of a component Implemented: * Create separate worktrees for each component alias * Create class symlinks for each component alias * Create each alias target with only the defaults and component class for the alias worktree * Works only for components which use `${_base_directory}` in their config (kapitan.compile and kapitan.dependencies mainly) TODO: * Actually allow users to specify version for each alias separately * Cleanup changes * Add tests * Update docs --- commodore/cluster.py | 17 ++++++--- commodore/component/__init__.py | 45 +++++++++++++++++++++-- commodore/dependency_mgmt/__init__.py | 52 +++++++++++++++++++++++++-- commodore/postprocess/jsonnet.py | 8 ++--- 4 files changed, 108 insertions(+), 14 deletions(-) diff --git a/commodore/cluster.py b/commodore/cluster.py index 332bfbc9..5acdfff9 100644 --- a/commodore/cluster.py +++ b/commodore/cluster.py @@ -143,22 +143,29 @@ def render_target( "_instance": target, } if not bootstrap: - parameters["_base_directory"] = str(components[component].target_directory) + parameters["_base_directory"] = str( + components[component].alias_directory(target) + ) for c in components: - if inv.defaults_file(c).is_file(): - classes.append(f"defaults.{c}") + defaults_file = inv.defaults_file(c) + if c == component and target != component: + # Special case alias defaults symlink + defaults_file = inv.defaults_file(target) + + if defaults_file.is_file(): + classes.append(f"defaults.{defaults_file.stem}") else: click.secho(f" > Default file for class {c} missing", fg="yellow") classes.append("global.commodore") if not bootstrap: - if not inv.component_file(component).is_file(): + if not inv.component_file(target).is_file(): raise click.ClickException( f"Target rendering failed for {target}: component class is missing" ) - classes.append(f"components.{component}") + classes.append(f"components.{target}") parameters["kapitan"] = { "vars": { "target": target, diff --git a/commodore/component/__init__.py b/commodore/component/__init__.py index 5f6c8cbf..1ba6dc7d 100644 --- a/commodore/component/__init__.py +++ b/commodore/component/__init__.py @@ -18,6 +18,8 @@ class Component: _version: Optional[str] = None _dir: P _sub_path: str + _aliases: dict[str, str] + _work_dir: Optional[P] # pylint: disable=too-many-arguments def __init__( @@ -43,6 +45,8 @@ def __init__( self.version = version self._sub_path = sub_path self._repo = None + self._aliases = {self.name: self.version or ""} + self._work_dir = work_dir @property def name(self) -> str: @@ -87,15 +91,30 @@ def repo_directory(self) -> P: @property def target_directory(self) -> P: - return self._dir / self._sub_path + return self.alias_directory(self.name) @property def class_file(self) -> P: - return self.target_directory / "class" / f"{self.name}.yml" + return self.alias_class_file(self.name) @property def defaults_file(self) -> P: - return self.target_directory / "class" / "defaults.yml" + return self.alias_defaults_file(self.name) + + def alias_directory(self, alias: str) -> P: + apath = self._dependency.get_component(alias) + if not apath: + raise ValueError(f"unknown alias {alias} for component {self.name}") + return apath / self._sub_path + + def alias_class_file(self, alias: str) -> P: + return self.alias_directory(alias) / "class" / f"{self.name}.yml" + + def alias_defaults_file(self, alias: str) -> P: + return self.alias_directory(alias) / "class" / "defaults.yml" + + def has_alias(self, alias: str): + return alias in self._aliases @property def lib_files(self) -> Iterable[P]: @@ -126,6 +145,26 @@ def parameters_key(self): def checkout(self): self._dependency.checkout_component(self.name, self.version) + def register_alias(self, alias: str, version: str): + if not self._work_dir: + raise ValueError( + f"Can't register alias on component {self.name} " + + "which isn't configured with a working directory" + ) + if alias in self._aliases: + raise ValueError( + f"alias {alias} already registered on component {self.name}" + ) + self._aliases[alias] = version + self._dependency.register_component(alias, component_dir(self._work_dir, alias)) + + def checkout_alias(self, alias: str): + if alias not in self._aliases: + raise ValueError( + f"alias {alias} is not registered on component {self.name}" + ) + self._dependency.checkout_component(alias, self._aliases[alias]) + def render_jsonnetfile_json(self, component_params): """ Render jsonnetfile.json from jsonnetfile.jsonnet diff --git a/commodore/dependency_mgmt/__init__.py b/commodore/dependency_mgmt/__init__.py index fa8dd9fa..a3515740 100644 --- a/commodore/dependency_mgmt/__init__.py +++ b/commodore/dependency_mgmt/__init__.py @@ -37,6 +37,27 @@ def create_component_symlinks(cfg, component: Component): ) +def create_alias_symlinks(cfg, component: Component, alias: str): + if not component.has_alias(alias): + raise ValueError( + f"component {component.name} doesn't have alias {alias} registered" + ) + relsymlink( + component.alias_class_file(alias), + cfg.inventory.components_dir, + dest_name=f"{alias}.yml", + ) + inventory_default = cfg.inventory.defaults_file(alias) + relsymlink( + component.alias_defaults_file(alias), + inventory_default.parent, + dest_name=inventory_default.name, + ) + # TODO: How do we handle lib files when symlinking aliases? Code in the component + # alias itself should be able to find the right library. We need to define what + # version of the library is visible for other components. + + def create_package_symlink(cfg, pname: str, package: Package): """ Create package symlink in the inventory. @@ -85,6 +106,20 @@ def fetch_components(cfg: Config): cfg.register_component(c) create_component_symlinks(cfg, c) + components = cfg.get_components() + + for alias, component in component_aliases.items(): + if alias == component: + # Nothing to setup for identity alias + continue + + c = components[component] + # TODO: Set alias version instead of component version + c.register_alias(alias, c.version or "") + c.checkout_alias(alias) + + create_alias_symlinks(cfg, c, alias) + def register_components(cfg: Config): """ @@ -122,9 +157,9 @@ def register_components(cfg: Config): cfg.register_component(component) create_component_symlinks(cfg, component) - registered_components = cfg.get_components().keys() + registered_components = cfg.get_components() pruned_aliases = { - a: c for a, c in component_aliases.items() if c in registered_components + a: c for a, c in component_aliases.items() if c in registered_components.keys() } pruned = sorted(set(component_aliases.keys()) - set(pruned_aliases.keys())) if len(pruned) > 0: @@ -133,6 +168,19 @@ def register_components(cfg: Config): ) cfg.register_component_aliases(pruned_aliases) + for alias, cn in pruned_aliases.items(): + if alias == cn: + # Nothing to setup for identity alias + continue + + c = registered_components[cn] + # TODO: Set alias version + c.register_alias(alias, c.version) + if not component_dir(cfg.work_dir, alias).is_dir(): + raise click.ClickException(f"Missing alias checkout for '{alias} as {cn}'") + + create_alias_symlinks(cfg, c, alias) + def fetch_packages(cfg: Config): """ diff --git a/commodore/postprocess/jsonnet.py b/commodore/postprocess/jsonnet.py index 30edd9c6..5cfe44f9 100644 --- a/commodore/postprocess/jsonnet.py +++ b/commodore/postprocess/jsonnet.py @@ -124,8 +124,8 @@ def _inventory() -> dict[str, Any]: write_jsonnet_output(output_dir, output) -def _filter_file(component: Component, filterpath: str) -> P: - return component.target_directory / filterpath +def _filter_file(component: Component, instance: str, filterpath: str) -> P: + return component.alias_directory(instance) / filterpath def run_jsonnet_filter( @@ -141,7 +141,7 @@ def run_jsonnet_filter( Run user-supplied jsonnet as postprocessing filter. This is the original way of doing postprocessing filters. """ - filterfile = _filter_file(component, filterid) + filterfile = _filter_file(component, instance, filterid) # pylint: disable=c-extension-no-member jsonnet_runner( config.work_dir, @@ -157,6 +157,6 @@ def run_jsonnet_filter( # pylint: disable=unused-argument def validate_jsonnet_filter(config: Config, c: Component, instance: str, fd: dict): - filterfile = _filter_file(c, fd["filter"]) + filterfile = _filter_file(c, instance, fd["filter"]) if not filterfile.is_file(): raise ValueError("Jsonnet filter definition does not exist") From 8942dfdc8d3167309c5c1a25db638832a0873d1f Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Tue, 19 Jul 2022 15:00:39 +0200 Subject: [PATCH 3/5] Update tests for new alias checkout structure --- tests/conftest.py | 25 ++++++++++++-------- tests/test_dependency_mgmt.py | 18 +++++++++++--- tests/test_postprocess.py | 3 +++ tests/test_render_inventory.py | 6 ++++- tests/test_target.py | 43 +++++++++++++++++++++++++--------- 5 files changed, 70 insertions(+), 25 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 0de687b2..ea6b72e2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -80,29 +80,34 @@ def api_data(): class MockMultiDependency: _repo: Repo - _target_dir: Path - _name: str + _components: dict[str, Path] + _packages: dict[str, Path] def __init__(self, repo: Repo): self._repo = repo + self._components = {} + self._packages = {} def register_component(self, name: str, target_dir: Path): - self._target_dir = target_dir - self._name = name + assert name not in self._components + self._components[name] = target_dir def checkout_component(self, name, version): - assert name == self._name + assert name in self._components assert version == "master" - self._repo.clone(self._target_dir) + self._repo.clone(self._components[name]) def register_package(self, name: str, target_dir: Path): - self._target_dir = target_dir - self._name = f"pkg.{name}" + assert name not in self._packages + self._packages[name] = target_dir def checkout_package(self, name, version): - assert name == self._name + assert name in self._packages assert version == "master" - self._repo.clone(self._target_dir) + self._repo.clone(self._packages[name]) + + def get_component(self, name) -> Path: + return self._components[name] @pytest.fixture diff --git a/tests/test_dependency_mgmt.py b/tests/test_dependency_mgmt.py index 0cc5036c..5d71f990 100644 --- a/tests/test_dependency_mgmt.py +++ b/tests/test_dependency_mgmt.py @@ -199,7 +199,7 @@ def test_fetch_components_is_minimal( assert not (tmp_path / "dependencies" / component).exists() -def _setup_register_components(tmp_path: Path): +def _setup_register_components(tmp_path: Path, aliases: dict[str, str] = {}): inv = Inventory(tmp_path) inv.ensure_dirs() component_dirs = ["foo", "bar", "baz"] @@ -215,6 +215,14 @@ def _setup_register_components(tmp_path: Path): with open(cpath / "class" / f"{directory}.yml", "w") as f: f.write("") + for alias, cn in aliases.items(): + cpath = tmp_path / "dependencies" / cn + if not cpath.is_dir(): + continue + apath = tmp_path / "dependencies" / alias + assert not apath.is_dir() + os.symlink(cpath, apath) + return component_dirs, other_dirs @@ -244,8 +252,10 @@ def test_register_components( def test_register_components_and_aliases( patch_discover, patch_read, config: Config, tmp_path: Path ): - component_dirs, other_dirs = _setup_register_components(tmp_path) alias_data = {"fooer": "foo"} + component_dirs, other_dirs = _setup_register_components( + tmp_path, aliases=alias_data + ) patch_discover.return_value = (component_dirs, alias_data) patch_read.return_value = { cn: DependencySpec(f"https://fake.repo.url/{cn}.git", "master", "") @@ -295,13 +305,15 @@ def test_register_unknown_components( def test_register_dangling_aliases( patch_discover, patch_read, config: Config, tmp_path: Path, capsys ): - component_dirs, other_dirs = _setup_register_components(tmp_path) # add some dangling aliases alias_data = {"quxer": "qux", "quuxer": "quux"} # generate expected output should_miss = sorted(set(alias_data.keys())) # add an alias that should work alias_data["bazzer"] = "baz" + component_dirs, other_dirs = _setup_register_components( + tmp_path, aliases=alias_data + ) patch_discover.return_value = (component_dirs, alias_data) patch_read.return_value = { diff --git a/tests/test_postprocess.py b/tests/test_postprocess.py index ccb9fd6e..fba681a8 100644 --- a/tests/test_postprocess.py +++ b/tests/test_postprocess.py @@ -155,6 +155,9 @@ def _setup(tmp_path, f, alias="test-component"): config = Config(work_dir=tmp_path) cdep = MultiDependency("https://fake.repo.url/", tmp_path / "dependencies") component = Component("test-component", dependency=cdep, work_dir=tmp_path) + if alias != "test-component": + component.register_alias(alias, "master") + os.symlink(component.target_directory, component.alias_directory(alias)) config.register_component(component) aliases = {alias: "test-component"} config.register_component_aliases(aliases) diff --git a/tests/test_render_inventory.py b/tests/test_render_inventory.py index 72b1a39a..8a65721b 100644 --- a/tests/test_render_inventory.py +++ b/tests/test_render_inventory.py @@ -7,7 +7,7 @@ from commodore.cluster import update_target from commodore.component import Component from commodore.config import Config -from commodore.dependency_mgmt import create_component_symlinks +from commodore.dependency_mgmt import create_component_symlinks, create_alias_symlinks from commodore.helpers import kapitan_inventory, yaml_dump, yaml_load from conftest import MockMultiDependency @@ -28,7 +28,10 @@ def _setup(tmp_path: Path): os.makedirs(tmp_path / "dependencies" / "test") cdep = MockMultiDependency(git.Repo.init(tmp_path / "repo.git")) c = Component("test", cdep, work_dir=tmp_path) + c.register_alias("test-1", "master") os.makedirs(c.class_file.parent) + # Create alias checkout by symlinking component directory + os.symlink(c.target_directory, c.alias_directory("test-1")) yaml_dump( { @@ -74,6 +77,7 @@ def _setup(tmp_path: Path): cfg.register_component(c) create_component_symlinks(cfg, c) cfg.register_component_aliases({"test-1": "test"}) + create_alias_symlinks(cfg, c, "test-1") for alias, component in cfg.get_component_aliases().items(): update_target(cfg, alias, component=component) diff --git a/tests/test_target.py b/tests/test_target.py index b87f31f9..458e80a3 100644 --- a/tests/test_target.py +++ b/tests/test_target.py @@ -1,6 +1,7 @@ """ Unit-tests for target generation """ +from __future__ import annotations import os import click @@ -17,18 +18,27 @@ class MockComponent: def __init__(self, base_dir: P, name: str): self.name = name - self._target_directory = base_dir / name + self._base_dir = base_dir + self.aliases = {self.name: "master"} @property def target_directory(self): - return self._target_directory + return self._base_dir / self.name + + def alias_directory(self, alias: str): + assert alias in self.aliases + return self._base_dir / alias + + def register_alias(self, alias: str, version: str): + assert alias not in self.aliases + self.aliases[alias] = version def cluster_from_data(apidata) -> cluster.Cluster: return cluster.Cluster(apidata["cluster"], apidata["tenant"]) -def _setup_working_dir(inv: Inventory, components): +def _setup_working_dir(inv: Inventory, components, aliases: dict[str, str] = {}): for cls in components: defaults = inv.defaults_file(cls) os.makedirs(defaults.parent, exist_ok=True) @@ -37,6 +47,14 @@ def _setup_working_dir(inv: Inventory, components): os.makedirs(component.parent, exist_ok=True) component.touch() + for alias in aliases: + defaults = inv.defaults_file(alias) + os.makedirs(defaults.parent, exist_ok=True) + defaults.touch() + component = inv.component_file(alias) + os.makedirs(component.parent, exist_ok=True) + component.touch() + def test_render_bootstrap_target(tmp_path: P): components = ["foo", "bar"] @@ -103,22 +121,23 @@ def test_render_target(tmp_path: P): def test_render_aliased_target(tmp_path: P): components = ["foo", "bar"] inv = Inventory(work_dir=tmp_path) - _setup_working_dir(inv, components) + _setup_working_dir(inv, components, aliases={"fooer": "foo"}) components = { "foo": MockComponent(tmp_path, "foo"), "bar": MockComponent(tmp_path, "bar"), "baz": MockComponent(tmp_path, "baz"), } + components["foo"].register_alias("fooer", "master") target = cluster.render_target(inv, "fooer", components, component="foo") classes = [ "params.cluster", - "defaults.foo", + "defaults.fooer", "defaults.bar", "global.commodore", - "components.foo", + "components.fooer", ] assert target != "" print(target) @@ -130,13 +149,13 @@ def test_render_aliased_target(tmp_path: P): assert target["parameters"]["kapitan"]["vars"]["target"] == "fooer" assert target["parameters"]["foo"] == "${fooer}" assert target["parameters"]["_instance"] == "fooer" - assert target["parameters"]["_base_directory"] == str(tmp_path / "foo") + assert target["parameters"]["_base_directory"] == str(tmp_path / "fooer") def test_render_aliased_target_with_dash(tmp_path: P): components = ["foo-comp", "bar"] inv = Inventory(work_dir=tmp_path) - _setup_working_dir(inv, components) + _setup_working_dir(inv, components, aliases={"foo-1": "foo-comp"}) components = { "foo-comp": MockComponent(tmp_path, "foo-comp"), @@ -144,14 +163,16 @@ def test_render_aliased_target_with_dash(tmp_path: P): "baz": MockComponent(tmp_path, "baz"), } + components["foo-comp"].register_alias("foo-1", "master") + target = cluster.render_target(inv, "foo-1", components, component="foo-comp") classes = [ "params.cluster", - "defaults.foo-comp", + "defaults.foo-1", "defaults.bar", "global.commodore", - "components.foo-comp", + "components.foo-1", ] assert target != "" print(target) @@ -163,7 +184,7 @@ def test_render_aliased_target_with_dash(tmp_path: P): assert target["parameters"]["kapitan"]["vars"]["target"] == "foo-1" assert target["parameters"]["foo_comp"] == "${foo_1}" assert target["parameters"]["_instance"] == "foo-1" - assert target["parameters"]["_base_directory"] == str(tmp_path / "foo-comp") + assert target["parameters"]["_base_directory"] == str(tmp_path / "foo-1") def test_render_params(api_data, tmp_path: P): From 74bc54d068da200eced9a6a0df7489f71a4d0718 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Tue, 19 Jul 2022 16:46:23 +0200 Subject: [PATCH 4/5] WIP - Allow users to specify instance versions We add support for specifying instance versions in `parameters.components`. Commodore falls back to the version/url/path specified for the component when the keys are not provided for component aliases. Note that the actual dependency handling doesn't yet support overriding URL/path for aliases. --- commodore/compile.py | 2 +- commodore/component/__init__.py | 4 ++ commodore/dependency_mgmt/__init__.py | 26 ++++++--- commodore/dependency_mgmt/version_parsing.py | 60 +++++++++++++++++--- 4 files changed, 76 insertions(+), 16 deletions(-) diff --git a/commodore/compile.py b/commodore/compile.py index e877d199..820ba65c 100644 --- a/commodore/compile.py +++ b/commodore/compile.py @@ -194,7 +194,7 @@ def setup_compile_environment(config: Config) -> tuple[dict[str, Any], Iterable[ config.register_component_deprecations(cluster_parameters) # Raise exception if component version override without URL is present in the # hierarchy. - verify_version_overrides(cluster_parameters) + verify_version_overrides(cluster_parameters, config.get_component_aliases()) for component in config.get_components().values(): ckey = component.parameters_key diff --git a/commodore/component/__init__.py b/commodore/component/__init__.py index 1ba6dc7d..7e496423 100644 --- a/commodore/component/__init__.py +++ b/commodore/component/__init__.py @@ -89,6 +89,10 @@ def version(self, version: str): def repo_directory(self) -> P: return self._dir + @property + def sub_path(self) -> str: + return self._sub_path + @property def target_directory(self) -> P: return self.alias_directory(self.name) diff --git a/commodore/dependency_mgmt/__init__.py b/commodore/dependency_mgmt/__init__.py index a3515740..4faa2d13 100644 --- a/commodore/dependency_mgmt/__init__.py +++ b/commodore/dependency_mgmt/__init__.py @@ -88,7 +88,7 @@ def fetch_components(cfg: Config): component_names, component_aliases = _discover_components(cfg) click.secho("Registering component aliases...", bold=True) cfg.register_component_aliases(component_aliases) - cspecs = _read_components(cfg, component_names) + cspecs = _read_components(cfg, component_aliases) click.secho("Fetching components...", bold=True) for cn in component_names: cspec = cspecs[cn] @@ -114,8 +114,14 @@ def fetch_components(cfg: Config): continue c = components[component] - # TODO: Set alias version instead of component version - c.register_alias(alias, c.version or "") + aspec = cspecs[alias] + if aspec.url != c.repo_url or aspec.path != c._sub_path: + # TODO: Figure out how we'll handle URL/subpath overrides + raise NotImplementedError( + "URL/path override for component alias not supported" + ) + print(alias, aspec) + c.register_alias(alias, aspec.version) c.checkout_alias(alias) create_alias_symlinks(cfg, c, alias) @@ -131,7 +137,7 @@ def register_components(cfg: Config): click.secho("Discovering included components...", bold=True) try: components, component_aliases = _discover_components(cfg) - cspecs = _read_components(cfg, components) + cspecs = _read_components(cfg, component_aliases) except KeyError as e: raise click.ClickException(f"While discovering components: {e}") click.secho("Registering components and aliases...", bold=True) @@ -174,8 +180,10 @@ def register_components(cfg: Config): continue c = registered_components[cn] - # TODO: Set alias version - c.register_alias(alias, c.version) + aspec = cspecs[alias] + if aspec.url != c.repo_url or aspec.path != c.sub_path: + raise NotImplementedError("Changing alias sub path / URL NYI") + c.register_alias(alias, aspec.version) if not component_dir(cfg.work_dir, alias).is_dir(): raise click.ClickException(f"Missing alias checkout for '{alias} as {cn}'") @@ -238,9 +246,13 @@ def register_packages(cfg: Config): create_package_symlink(cfg, p, pkg) -def verify_version_overrides(cluster_parameters): +def verify_version_overrides(cluster_parameters, component_aliases: dict[str, str]): errors = [] + aliases = set(component_aliases.keys()) - set(component_aliases.values()) for cname, cspec in cluster_parameters["components"].items(): + if cname in aliases: + # We don't require an url in component alias version configs + continue if "url" not in cspec: errors.append(f"component '{cname}'") diff --git a/commodore/dependency_mgmt/version_parsing.py b/commodore/dependency_mgmt/version_parsing.py index ed475322..a30d1f0a 100644 --- a/commodore/dependency_mgmt/version_parsing.py +++ b/commodore/dependency_mgmt/version_parsing.py @@ -2,6 +2,7 @@ from collections.abc import Iterable from dataclasses import dataclass +from typing import Optional from enum import Enum @@ -33,18 +34,31 @@ class DependencySpec: path: str @classmethod - def parse(cls, info: dict[str, str]) -> DependencySpec: - if "url" not in info: + def parse( + cls, + info: dict[str, str], + base_config: Optional[DependencySpec] = None, + ) -> DependencySpec: + if "url" not in info and not base_config: raise DependencyParseError("url") - if "version" not in info: + if "version" not in info and not base_config: raise DependencyParseError("version") path = info.get("path", "") if path.startswith("/"): path = path[1:] - return DependencySpec(info["url"], info["version"], path) + if base_config: + url = info.get("url", base_config.url) + version = info.get("version", base_config.version) + if path not in info: + path = base_config.path + else: + url = info["url"] + version = info["version"] + + return DependencySpec(url, version, path) def _read_versions( @@ -53,6 +67,8 @@ def _read_versions( dependency_names: Iterable[str], require_key: bool = True, ignore_class_notfound: bool = False, + aliases: dict[str, str] = {}, + fallback: dict[str, DependencySpec] = {}, ) -> dict[str, DependencySpec]: deps_key = dependency_type.value deptype_str = dependency_type.name.lower() @@ -71,15 +87,26 @@ def _read_versions( # just set deps to the empty dict. deps = {} + if aliases: + all_dep_keys = set(aliases.keys()) + else: + all_dep_keys = deps.keys() for depname in dependency_names: - if depname not in deps: + if depname not in all_dep_keys: raise click.ClickException( f"Unknown {deptype_str} '{depname}'." + f" Please add it to 'parameters.{deps_key}'" ) try: - dep = DependencySpec.parse(deps[depname]) + basename_for_dep = aliases.get(depname, depname) + print(depname, basename_for_dep) + print(deps.get(depname, {})) + print(fallback.get(basename_for_dep)) + dep = DependencySpec.parse( + deps.get(depname, {}), + base_config=fallback.get(basename_for_dep), + ) except DependencyParseError as e: raise click.ClickException( f"{deptype_cap} '{depname}' is missing field '{e.field}'" @@ -96,9 +123,26 @@ def _read_versions( def _read_components( - cfg: Config, component_names: Iterable[str] + cfg: Config, component_aliases: dict[str, str] ) -> dict[str, DependencySpec]: - return _read_versions(cfg, DepType.COMPONENT, component_names) + component_names = set(component_aliases.values()) + alias_names = set(component_aliases.keys()) - component_names + + component_versions = _read_versions(cfg, DepType.COMPONENT, component_names) + alias_versions = _read_versions( + cfg, + DepType.COMPONENT, + alias_names, + aliases=component_aliases, + fallback=component_versions, + ) + + for alias, aspec in alias_versions.items(): + if alias in component_versions: + raise ValueError("alias name already in component_versions?") + component_versions[alias] = aspec + + return component_versions def _read_packages( From 591f0818fe7b6f180cb11f477a356a112e6904ce Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Tue, 19 Jul 2022 16:54:47 +0200 Subject: [PATCH 5/5] Update tests to work with new alias version structure --- tests/test_dependency_mgmt.py | 6 ++++-- tests/test_dependency_mgmt_version_parsing.py | 8 +++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/test_dependency_mgmt.py b/tests/test_dependency_mgmt.py index 5d71f990..5639e7ba 100644 --- a/tests/test_dependency_mgmt.py +++ b/tests/test_dependency_mgmt.py @@ -261,6 +261,7 @@ def test_register_components_and_aliases( cn: DependencySpec(f"https://fake.repo.url/{cn}.git", "master", "") for cn in component_dirs } + patch_read.return_value["fooer"] = patch_read.return_value["foo"] dependency_mgmt.register_components(config) @@ -320,6 +321,7 @@ def test_register_dangling_aliases( cn: DependencySpec(f"https://fake.repo.url/{cn}.git", "master", "") for cn in component_dirs } + patch_read.return_value["bazzer"] = patch_read.return_value["baz"] dependency_mgmt.register_components(config) @@ -473,10 +475,10 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: ) def test_verify_component_version_overrides(cluster_params: dict, expected: str): if expected == "": - dependency_mgmt.verify_version_overrides(cluster_params) + dependency_mgmt.verify_version_overrides(cluster_params, {}) else: with pytest.raises(click.ClickException) as e: - dependency_mgmt.verify_version_overrides(cluster_params) + dependency_mgmt.verify_version_overrides(cluster_params, {}) assert expected in str(e) diff --git a/tests/test_dependency_mgmt_version_parsing.py b/tests/test_dependency_mgmt_version_parsing.py index b31df792..f10d7d3d 100644 --- a/tests/test_dependency_mgmt_version_parsing.py +++ b/tests/test_dependency_mgmt_version_parsing.py @@ -23,7 +23,9 @@ @patch.object(version_parsing, "kapitan_inventory") def test_read_components(patch_inventory, config: Config): components = _setup_mock_inventory(patch_inventory) - cspecs = version_parsing._read_components(config, ["test-component"]) + cspecs = version_parsing._read_components( + config, {"test-component": "test-component"} + ) # check that exactly 'test-component' is discovered assert {"test-component"} == set(cspecs.keys()) @@ -34,7 +36,7 @@ def test_read_components(patch_inventory, config: Config): @patch.object(version_parsing, "kapitan_inventory") def test_read_components_multiple(patch_inventory, config: Config): components = _setup_mock_inventory(patch_inventory) - cspecs = version_parsing._read_components(config, components.keys()) + cspecs = version_parsing._read_components(config, {k: k for k in components.keys()}) # check that exactly 'test-component' is discovered assert set(components.keys()) == set(cspecs.keys()) assert all(components[cn]["url"] == cspecs[cn].url for cn in components.keys()) @@ -80,7 +82,7 @@ def test_read_components_exc( } with pytest.raises(click.ClickException) as exc_info: - _ = version_parsing._read_components(config, ckeys) + _ = version_parsing._read_components(config, {k: k for k in ckeys}) assert exc_info.value.args[0] == exctext