From 3bc70265e982decbdc939503ae38fa993467b855 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Tue, 19 Jul 2022 16:57:42 +0200 Subject: [PATCH 01/16] 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 c73708cbf..332bfbc96 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 79737229a..62da9432d 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 45f2c59da..f59fd2e5a 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 97faa0435..d2d8fff05 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 02/16] 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 332bfbc96..5acdfff94 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 5f6c8cbf0..1ba6dc7d7 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 fa8dd9fa3..a35157408 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 30edd9c69..5cfe44f9e 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 03/16] 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 0de687b24..ea6b72e2c 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 0cc5036c7..5d71f9907 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 ccb9fd6ea..fba681a85 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 72b1a39a8..8a65721b3 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 b87f31f96..458e80a39 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 04/16] 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 e877d199a..820ba65c6 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 1ba6dc7d7..7e4964232 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 a35157408..4faa2d131 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 ed475322e..a30d1f0a8 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 05/16] 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 5d71f9907..5639e7bae 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 b31df7927..f10d7d3dc 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 From 1ed6ef60acf70a74aa4113ae23a022fb2cd6d5c2 Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Tue, 21 Jan 2025 18:15:18 +0100 Subject: [PATCH 06/16] Add support for overriding repo URL in component instances --- commodore/component/__init__.py | 11 ++- commodore/dependency_mgmt/__init__.py | 16 ++-- tests/test_dependency_mgmt.py | 127 ++++++++++++++++++++++---- 3 files changed, 130 insertions(+), 24 deletions(-) diff --git a/commodore/component/__init__.py b/commodore/component/__init__.py index c13c89e12..96ba8b308 100644 --- a/commodore/component/__init__.py +++ b/commodore/component/__init__.py @@ -134,6 +134,10 @@ def sub_path(self) -> str: def repo_directory(self) -> P: return self._dir + @property + def work_directory(self) -> P: + return self._work_dir + @property def target_directory(self) -> P: return self.alias_directory(self.name) @@ -218,12 +222,15 @@ def register_alias(self, alias: str, version: str): alias, component_dir(self._work_dir, alias) ) - def checkout_alias(self, alias: str): + def checkout_alias(self, alias: str, alias_dependency: MultiDependency = None): if alias not in self._aliases: raise ValueError( f"alias {alias} is not registered on component {self.name}" ) - if self._dependency: + + if alias_dependency: + alias_dependency.checkout_component(alias, self._aliases[alias]) + elif self._dependency: self._dependency.checkout_component(alias, self._aliases[alias]) def is_checked_out(self) -> bool: diff --git a/commodore/dependency_mgmt/__init__.py b/commodore/dependency_mgmt/__init__.py index 00ef680c3..65efdf6ab 100644 --- a/commodore/dependency_mgmt/__init__.py +++ b/commodore/dependency_mgmt/__init__.py @@ -123,14 +123,16 @@ def fetch_components(cfg: Config): c = components[component] aspec = cspecs[alias] + adep = None 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" + adep = cfg.register_dependency_repo(aspec.url) + adep.register_component( + alias, component_dir(c.work_directory, alias) ) + print(alias, aspec) c.register_alias(alias, aspec.version) - c.checkout_alias(alias) + c.checkout_alias(alias, adep) create_alias_symlinks(cfg, c, alias) @@ -297,8 +299,10 @@ def verify_version_overrides(cluster_parameters, component_aliases: dict[str, st 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: + # but we do require the base component to have one + if component_aliases[cname] not in cluster_parameters["components"]: + errors.append(f"component '{component_aliases[cname]}' (imported as {cname})") + elif "url" not in cspec: errors.append(f"component '{cname}'") for pname, pspec in cluster_parameters.get("packages", {}).items(): diff --git a/tests/test_dependency_mgmt.py b/tests/test_dependency_mgmt.py index 02515db5a..5e27429c1 100644 --- a/tests/test_dependency_mgmt.py +++ b/tests/test_dependency_mgmt.py @@ -27,27 +27,41 @@ from conftest import MockMultiDependency -def setup_components_upstream(tmp_path: Path, components: Iterable[str]): +def setup_components_upstream(tmp_path: Path, components: Iterable[str], aliases: dict[str, str]={}): # Prepare minimum component directories upstream = tmp_path / "upstream" component_specs = {} for component in components: - repo_path = upstream / component - url = f"file://{repo_path.resolve()}" - version = None - repo = git.Repo.init(repo_path) + component_specs[component] = _prepare_repository(upstream, component, component) - class_dir = repo_path / "class" - class_dir.mkdir(parents=True, exist_ok=True) - (class_dir / "defaults.yml").touch(exist_ok=True) - (class_dir / f"{component}.yml").touch(exist_ok=True) + for alias in aliases.keys(): + if aliases[alias] == component: + component_specs[alias] = component_specs[component] - repo.index.add(["class/defaults.yml", f"class/{component}.yml"]) - repo.index.commit("component defaults") - component_specs[component] = DependencySpec(url, version, "") + return component_specs + +def setup_aliases_upstream(tmp_path: Path, aliases: Iterable[tuple[str,str]]): + upstream = tmp_path / "upstream" + component_specs = {} + for alias, component in aliases: + component_specs[alias] = _prepare_repository(upstream, alias, component) return component_specs +def _prepare_repository(upstream: Path, repo_name: str, component_name: str): + repo_path = upstream / repo_name + url = f"file://{repo_path.resolve()}" + version = None + repo = git.Repo.init(repo_path) + + class_dir = repo_path / "class" + class_dir.mkdir(parents=True, exist_ok=True) + (class_dir / "defaults.yml").touch(exist_ok=True) + (class_dir / f"{component_name}.yml").touch(exist_ok=True) + + repo.index.add(["class/defaults.yml", f"class/{component_name}.yml"]) + repo.index.commit("component defaults") + return DependencySpec(url, version, "") def test_create_component_symlinks_fails(config: Config, tmp_path: Path, mockdep): component = Component("my-component", mockdep, work_dir=tmp_path) @@ -167,6 +181,41 @@ def test_fetch_components(patch_discover, patch_read, config: Config, tmp_path: ).is_symlink() assert (tmp_path / "dependencies" / component).is_dir() +@patch("commodore.dependency_mgmt._read_components") +@patch("commodore.dependency_mgmt._discover_components") +def test_fetch_components_with_alias_version(patch_discover, patch_read, config: Config, tmp_path: Path): + components = ["component-one", "component-two"] + aliases = {"alias-one": "component-one", "alias-two": "component-two"} + patch_discover.return_value = (components, aliases) + + forked_aliases = [("alias-two", "component-two")] + aspecs = setup_aliases_upstream(tmp_path, forked_aliases) + rv = setup_components_upstream(tmp_path, components, aliases) + rv["alias-two"] = aspecs["alias-two"] + + patch_read.return_value = rv + + dependency_mgmt.fetch_components(config) + + for component in components: + assert component in config._components + assert ( + tmp_path / "inventory" / "classes" / "components" / f"{component}.yml" + ).is_symlink() + assert ( + tmp_path / "inventory" / "classes" / "defaults" / f"{component}.yml" + ).is_symlink() + assert (tmp_path / "dependencies" / component).is_dir() + + for alias in aliases.keys(): + assert alias in config._component_aliases + assert ( + tmp_path / "inventory" / "classes" / "components" / f"{alias}.yml" + ).is_symlink() + assert ( + tmp_path / "inventory" / "classes" / "defaults" / f"{alias}.yml" + ).is_symlink() + @patch("commodore.dependency_mgmt._read_components") @patch("commodore.dependency_mgmt._discover_components") @@ -407,7 +456,7 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: @pytest.mark.parametrize( - "cluster_params,expected", + "cluster_params,aliases,expected", [ ( { @@ -426,6 +475,7 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: }, }, }, + {}, "", ), ( @@ -446,6 +496,7 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: }, }, }, + {}, "Version override specified for component 'component_1' which has no URL", ), ( @@ -467,6 +518,7 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: }, }, }, + {}, "Version overrides specified for component 'component_1' and component 'component_2' which have no URL", ), ( @@ -483,6 +535,7 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: } }, }, + {}, "Version override specified for package 'package-1' which has no URL", ), ( @@ -498,6 +551,7 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: } }, }, + {}, "Version overrides specified for component 'component-1' and package 'package-1' which have no URL", ), ( @@ -516,17 +570,58 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: } }, }, + {}, "Version overrides specified for component 'component-1', " + "component 'component-2', and package 'package-1' which have no URL", ), + ( + { + "components": { + "component-1": { + "version": "v1.2.3", + "url": "https://example.com/component-1.git", + }, + "component-2": { + "version": "v1.2.3", + "url": "https://example.com/component-2.git", + }, + "alias-1": { + "version": "v1.2.4", + "url": "https://example.com/component-1-fork.git", + }, + "alias-2": { + "version": "v1.2.4", + }, + }, + }, + { + "alias-1": "component-1", + "alias-2": "component-2", + }, + "", + ), + ( + { + "components": { + "alias-1": { + "version": "v1.2.4", + "url": "https://example.com/component-1-fork.git", + }, + }, + }, + { + "alias-1": "component-1", + }, + "Version override specified for component 'component-1' (imported as alias-1) which has no URL", + ), ], ) -def test_verify_component_version_overrides(cluster_params: dict, expected: str): +def test_verify_component_version_overrides(cluster_params: dict, aliases: dict, expected: str): if expected == "": - dependency_mgmt.verify_version_overrides(cluster_params, {}) + dependency_mgmt.verify_version_overrides(cluster_params, aliases) else: with pytest.raises(click.ClickException) as e: - dependency_mgmt.verify_version_overrides(cluster_params, {}) + dependency_mgmt.verify_version_overrides(cluster_params, aliases) assert expected in str(e) From 7894de9485ef4c272ac43ffde3e552f0d36c3178 Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Tue, 21 Jan 2025 19:31:41 +0100 Subject: [PATCH 07/16] Verify whether component supports multi-versioning --- commodore/config.py | 26 ++++++++++++++++++----- tests/test_config.py | 49 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/commodore/config.py b/commodore/config.py index cb2e2608a..6e7db815b 100644 --- a/commodore/config.py +++ b/commodore/config.py @@ -377,14 +377,23 @@ def get_component_aliases(self): return self._component_aliases def register_component_aliases(self, aliases: dict[str, str]): - self._component_aliases = aliases + self._component_aliases.update(aliases) def verify_component_aliases(self, cluster_parameters: dict): + print(cluster_parameters) for alias, cn in self._component_aliases.items(): - if alias != cn and not _component_is_aliasable(cluster_parameters, cn): - raise click.ClickException( - f"Component {cn} with alias {alias} does not support instantiation." - ) + if alias != cn: + if not _component_is_aliasable(cluster_parameters, cn): + raise click.ClickException( + f"Component {cn} with alias {alias} does not support instantiation." + ) + + cv = cluster_parameters.get('components', {}).get(alias, {}) + alias_has_version = cv.get('url') != None or cv.get('version') != None + if alias_has_version and not _component_supports_alias_version(cluster_parameters, cn, alias): + raise click.ClickException( + f"Component {cn} with alias {alias} does not support overriding instance version." + ) def get_component_alias_versioninfos(self) -> dict[str, InstanceVersionInfo]: return { @@ -452,6 +461,13 @@ def _component_is_aliasable(cluster_parameters: dict, component_name: str): cmeta = cluster_parameters[ckey].get("_metadata", {}) return cmeta.get("multi_instance", False) +def _component_supports_alias_version(cluster_parameters: dict, component_name: str, alias: str,): + ckey = component_parameters_key(component_name) + cmeta = cluster_parameters[ckey].get("_metadata", {}) + akey = component_parameters_key(alias) + ameta = cluster_parameters.get(akey, {}).get("_metadata", {}) + return cmeta.get("multi_versioning", False) and ameta.get("multi_versioning", False) + def set_fact_value(facts: dict[str, Any], raw_key: str, value: Any) -> None: """Set value for nested fact at `raw_key` (expected form `path.to.key`) to `value`. diff --git a/tests/test_config.py b/tests/test_config.py index 9f636df40..3d93d9470 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -20,8 +20,9 @@ parse_dynamic_fact_value, parse_dynamic_facts_from_cli, ) +from commodore.component import Component from commodore.package import Package -from commodore.multi_dependency import dependency_key +from commodore.multi_dependency import dependency_key, MultiDependency def test_verify_component_aliases_no_instance(config): @@ -40,7 +41,51 @@ def test_verify_component_aliases_explicit_no_instance(config): config.verify_component_aliases(params) -def test_verify_component_aliases_metadata(config): +def test_verify_component_aliases_explicit_no_multiversion_exception(config): + alias_data = {"baz": "bar"} + config.register_component_aliases(alias_data) + params = {"components": {"bar": {"url": "foo", "version": "v1.0.0"}, "baz": {"version": "v1.1.0"}}, "bar": {"_metadata": {"multi_instance": True, "multi_versioning": False}, "namespace": "syn-bar"}, "baz": {"_metadata": {"multi_instance": True, "multi_versioning": False}, "namespace": "syn-baz"}} + + with pytest.raises(click.ClickException) as e: + config.verify_component_aliases(params) + + assert "Component bar with alias baz does not support overriding instance version." in str( + e.value + ) + +def test_verify_component_aliases_explicit_no_multiversion_in_alias_exception(config): + alias_data = {"baz": "bar"} + config.register_component_aliases(alias_data) + params = {"components": {"bar": {"url": "foo", "version": "v1.0.0"}, "baz": {"version": "v1.1.0"}}, "bar": {"_metadata": {"multi_instance": True, "multi_versioning": True}, "namespace": "syn-bar"}, "baz": {"_metadata": {"multi_instance": True, "multi_versioning": False}, "namespace": "syn-baz"}} + + with pytest.raises(click.ClickException) as e: + config.verify_component_aliases(params) + + assert "Component bar with alias baz does not support overriding instance version." in str( + e.value + ) + +def test_verify_component_multiversion_exception(config): + alias_data = {"baz": "bar"} + config.register_component_aliases(alias_data) + params = {"components": {"bar": {"url": "foo", "version": "v1.0.0"}, "baz": {"version": "v1.1.0"}}, "bar": {"_metadata": {"multi_instance": True}}} + + with pytest.raises(click.ClickException) as e: + config.verify_component_aliases(params) + + assert "Component bar with alias baz does not support overriding instance version." in str( + e.value + ) + +def test_verify_component_multiversion(config): + alias_data = {"baz": "bar"} + config.register_component_aliases(alias_data) + params = {"components": {"bar": {"url": "foo", "version": "v1.0.0"}, "baz": {"version": "v1.1.0"}}, "bar": {"_metadata": {"multi_instance": True, "multi_versioning": True}, "namespace": "syn-bar"}, "baz": {"_metadata": {"multi_versioning": True}, "namespace": "syn-baz"}} + + config.verify_component_aliases(params) + + +def test_verify_component_aliases_metadata(config, tmp_path: Path, mockdep): alias_data = {"baz": "bar"} config.register_component_aliases(alias_data) params = {"bar": {"_metadata": {"multi_instance": True}, "namespace": "syn-bar"}} From 7ffd2f29b2f20d86b333673945c3199856de7da2 Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Tue, 21 Jan 2025 19:38:02 +0100 Subject: [PATCH 08/16] Apply black, flake8, mypy --- README.md | 6 +-- commodore/component/__init__.py | 6 ++- commodore/config.py | 17 +++++-- commodore/dependency_mgmt/__init__.py | 13 +++-- tests/test_config.py | 73 ++++++++++++++++++++++----- tests/test_dependency_mgmt.py | 18 +++++-- 6 files changed, 103 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index cd4eaa625..7d2308ed5 100644 --- a/README.md +++ b/README.md @@ -115,9 +115,9 @@ Commodore also supports additional processing on the output of Kapitan, such as 1. Run linting and tests - Auto format with autopep8 + Automatically apply Black formatting ```console - poetry run autopep + poetry run black . ``` List all Tox targets @@ -132,7 +132,7 @@ Commodore also supports additional processing on the output of Kapitan, such as Run just a specific target ```console - poetry run tox -e py38 + poetry run tox -e py312 ``` diff --git a/commodore/component/__init__.py b/commodore/component/__init__.py index 96ba8b308..3ff0b55c3 100644 --- a/commodore/component/__init__.py +++ b/commodore/component/__init__.py @@ -135,7 +135,7 @@ def repo_directory(self) -> P: return self._dir @property - def work_directory(self) -> P: + def work_directory(self) -> Optional[P]: return self._work_dir @property @@ -222,7 +222,9 @@ def register_alias(self, alias: str, version: str): alias, component_dir(self._work_dir, alias) ) - def checkout_alias(self, alias: str, alias_dependency: MultiDependency = None): + def checkout_alias( + self, alias: str, alias_dependency: Optional[MultiDependency] = None + ): if alias not in self._aliases: raise ValueError( f"alias {alias} is not registered on component {self.name}" diff --git a/commodore/config.py b/commodore/config.py index 6e7db815b..db13ab13d 100644 --- a/commodore/config.py +++ b/commodore/config.py @@ -388,9 +388,13 @@ def verify_component_aliases(self, cluster_parameters: dict): f"Component {cn} with alias {alias} does not support instantiation." ) - cv = cluster_parameters.get('components', {}).get(alias, {}) - alias_has_version = cv.get('url') != None or cv.get('version') != None - if alias_has_version and not _component_supports_alias_version(cluster_parameters, cn, alias): + cv = cluster_parameters.get("components", {}).get(alias, {}) + alias_has_version = ( + cv.get("url") is not None or cv.get("version") is not None + ) + if alias_has_version and not _component_supports_alias_version( + cluster_parameters, cn, alias + ): raise click.ClickException( f"Component {cn} with alias {alias} does not support overriding instance version." ) @@ -461,7 +465,12 @@ def _component_is_aliasable(cluster_parameters: dict, component_name: str): cmeta = cluster_parameters[ckey].get("_metadata", {}) return cmeta.get("multi_instance", False) -def _component_supports_alias_version(cluster_parameters: dict, component_name: str, alias: str,): + +def _component_supports_alias_version( + cluster_parameters: dict, + component_name: str, + alias: str, +): ckey = component_parameters_key(component_name) cmeta = cluster_parameters[ckey].get("_metadata", {}) akey = component_parameters_key(alias) diff --git a/commodore/dependency_mgmt/__init__.py b/commodore/dependency_mgmt/__init__.py index 65efdf6ab..ca0f34afe 100644 --- a/commodore/dependency_mgmt/__init__.py +++ b/commodore/dependency_mgmt/__init__.py @@ -126,9 +126,12 @@ def fetch_components(cfg: Config): adep = None if aspec.url != c.repo_url or aspec.path != c._sub_path: adep = cfg.register_dependency_repo(aspec.url) - adep.register_component( - alias, component_dir(c.work_directory, alias) - ) + wdir = c.work_directory + if not wdir: + raise ValueError( + "Cannot checkout repo for component alias if component does not have a working directory." + ) + adep.register_component(alias, component_dir(wdir, alias)) print(alias, aspec) c.register_alias(alias, aspec.version) @@ -301,7 +304,9 @@ def verify_version_overrides(cluster_parameters, component_aliases: dict[str, st # We don't require an url in component alias version configs # but we do require the base component to have one if component_aliases[cname] not in cluster_parameters["components"]: - errors.append(f"component '{component_aliases[cname]}' (imported as {cname})") + errors.append( + f"component '{component_aliases[cname]}' (imported as {cname})" + ) elif "url" not in cspec: errors.append(f"component '{cname}'") diff --git a/tests/test_config.py b/tests/test_config.py index 3d93d9470..3a1a4fe6a 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -20,9 +20,8 @@ parse_dynamic_fact_value, parse_dynamic_facts_from_cli, ) -from commodore.component import Component from commodore.package import Package -from commodore.multi_dependency import dependency_key, MultiDependency +from commodore.multi_dependency import dependency_key def test_verify_component_aliases_no_instance(config): @@ -44,48 +43,96 @@ def test_verify_component_aliases_explicit_no_instance(config): def test_verify_component_aliases_explicit_no_multiversion_exception(config): alias_data = {"baz": "bar"} config.register_component_aliases(alias_data) - params = {"components": {"bar": {"url": "foo", "version": "v1.0.0"}, "baz": {"version": "v1.1.0"}}, "bar": {"_metadata": {"multi_instance": True, "multi_versioning": False}, "namespace": "syn-bar"}, "baz": {"_metadata": {"multi_instance": True, "multi_versioning": False}, "namespace": "syn-baz"}} + params = { + "components": { + "bar": {"url": "foo", "version": "v1.0.0"}, + "baz": {"version": "v1.1.0"}, + }, + "bar": { + "_metadata": {"multi_instance": True, "multi_versioning": False}, + "namespace": "syn-bar", + }, + "baz": { + "_metadata": {"multi_instance": True, "multi_versioning": False}, + "namespace": "syn-baz", + }, + } with pytest.raises(click.ClickException) as e: config.verify_component_aliases(params) - assert "Component bar with alias baz does not support overriding instance version." in str( - e.value + assert ( + "Component bar with alias baz does not support overriding instance version." + in str(e.value) ) + def test_verify_component_aliases_explicit_no_multiversion_in_alias_exception(config): alias_data = {"baz": "bar"} config.register_component_aliases(alias_data) - params = {"components": {"bar": {"url": "foo", "version": "v1.0.0"}, "baz": {"version": "v1.1.0"}}, "bar": {"_metadata": {"multi_instance": True, "multi_versioning": True}, "namespace": "syn-bar"}, "baz": {"_metadata": {"multi_instance": True, "multi_versioning": False}, "namespace": "syn-baz"}} + params = { + "components": { + "bar": {"url": "foo", "version": "v1.0.0"}, + "baz": {"version": "v1.1.0"}, + }, + "bar": { + "_metadata": {"multi_instance": True, "multi_versioning": True}, + "namespace": "syn-bar", + }, + "baz": { + "_metadata": {"multi_instance": True, "multi_versioning": False}, + "namespace": "syn-baz", + }, + } with pytest.raises(click.ClickException) as e: config.verify_component_aliases(params) - assert "Component bar with alias baz does not support overriding instance version." in str( - e.value + assert ( + "Component bar with alias baz does not support overriding instance version." + in str(e.value) ) + def test_verify_component_multiversion_exception(config): alias_data = {"baz": "bar"} config.register_component_aliases(alias_data) - params = {"components": {"bar": {"url": "foo", "version": "v1.0.0"}, "baz": {"version": "v1.1.0"}}, "bar": {"_metadata": {"multi_instance": True}}} + params = { + "components": { + "bar": {"url": "foo", "version": "v1.0.0"}, + "baz": {"version": "v1.1.0"}, + }, + "bar": {"_metadata": {"multi_instance": True}}, + } with pytest.raises(click.ClickException) as e: config.verify_component_aliases(params) - assert "Component bar with alias baz does not support overriding instance version." in str( - e.value + assert ( + "Component bar with alias baz does not support overriding instance version." + in str(e.value) ) + def test_verify_component_multiversion(config): alias_data = {"baz": "bar"} config.register_component_aliases(alias_data) - params = {"components": {"bar": {"url": "foo", "version": "v1.0.0"}, "baz": {"version": "v1.1.0"}}, "bar": {"_metadata": {"multi_instance": True, "multi_versioning": True}, "namespace": "syn-bar"}, "baz": {"_metadata": {"multi_versioning": True}, "namespace": "syn-baz"}} + params = { + "components": { + "bar": {"url": "foo", "version": "v1.0.0"}, + "baz": {"version": "v1.1.0"}, + }, + "bar": { + "_metadata": {"multi_instance": True, "multi_versioning": True}, + "namespace": "syn-bar", + }, + "baz": {"_metadata": {"multi_versioning": True}, "namespace": "syn-baz"}, + } config.verify_component_aliases(params) -def test_verify_component_aliases_metadata(config, tmp_path: Path, mockdep): +def test_verify_component_aliases_metadata(config): alias_data = {"baz": "bar"} config.register_component_aliases(alias_data) params = {"bar": {"_metadata": {"multi_instance": True}, "namespace": "syn-bar"}} diff --git a/tests/test_dependency_mgmt.py b/tests/test_dependency_mgmt.py index 5e27429c1..9f7485443 100644 --- a/tests/test_dependency_mgmt.py +++ b/tests/test_dependency_mgmt.py @@ -27,7 +27,9 @@ from conftest import MockMultiDependency -def setup_components_upstream(tmp_path: Path, components: Iterable[str], aliases: dict[str, str]={}): +def setup_components_upstream( + tmp_path: Path, components: Iterable[str], aliases: dict[str, str] = {} +): # Prepare minimum component directories upstream = tmp_path / "upstream" component_specs = {} @@ -40,7 +42,8 @@ def setup_components_upstream(tmp_path: Path, components: Iterable[str], aliases return component_specs -def setup_aliases_upstream(tmp_path: Path, aliases: Iterable[tuple[str,str]]): + +def setup_aliases_upstream(tmp_path: Path, aliases: Iterable[tuple[str, str]]): upstream = tmp_path / "upstream" component_specs = {} for alias, component in aliases: @@ -48,6 +51,7 @@ def setup_aliases_upstream(tmp_path: Path, aliases: Iterable[tuple[str,str]]): return component_specs + def _prepare_repository(upstream: Path, repo_name: str, component_name: str): repo_path = upstream / repo_name url = f"file://{repo_path.resolve()}" @@ -63,6 +67,7 @@ def _prepare_repository(upstream: Path, repo_name: str, component_name: str): repo.index.commit("component defaults") return DependencySpec(url, version, "") + def test_create_component_symlinks_fails(config: Config, tmp_path: Path, mockdep): component = Component("my-component", mockdep, work_dir=tmp_path) with pytest.raises(click.ClickException) as e: @@ -181,9 +186,12 @@ def test_fetch_components(patch_discover, patch_read, config: Config, tmp_path: ).is_symlink() assert (tmp_path / "dependencies" / component).is_dir() + @patch("commodore.dependency_mgmt._read_components") @patch("commodore.dependency_mgmt._discover_components") -def test_fetch_components_with_alias_version(patch_discover, patch_read, config: Config, tmp_path: Path): +def test_fetch_components_with_alias_version( + patch_discover, patch_read, config: Config, tmp_path: Path +): components = ["component-one", "component-two"] aliases = {"alias-one": "component-one", "alias-two": "component-two"} patch_discover.return_value = (components, aliases) @@ -616,7 +624,9 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: ), ], ) -def test_verify_component_version_overrides(cluster_params: dict, aliases: dict, expected: str): +def test_verify_component_version_overrides( + cluster_params: dict, aliases: dict, expected: str +): if expected == "": dependency_mgmt.verify_version_overrides(cluster_params, aliases) else: From 5b1b243a963662f7dde830eb11830b8385b475a9 Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Wed, 22 Jan 2025 23:53:17 +0100 Subject: [PATCH 09/16] Increase code coverage and support overriding repo path --- commodore/component/__init__.py | 14 ++++---- commodore/dependency_mgmt/__init__.py | 9 +++--- commodore/dependency_mgmt/version_parsing.py | 2 +- tests/test_dependency_mgmt.py | 32 +++++++++++++++---- tests/test_dependency_mgmt_version_parsing.py | 30 ++++++++++++++--- 5 files changed, 64 insertions(+), 23 deletions(-) diff --git a/commodore/component/__init__.py b/commodore/component/__init__.py index 3ff0b55c3..e5caffdfd 100644 --- a/commodore/component/__init__.py +++ b/commodore/component/__init__.py @@ -19,7 +19,7 @@ class Component: _version: Optional[str] = None _dir: P _sub_path: str - _aliases: dict[str, str] + _aliases: dict[str, tuple[str, str]] _work_dir: Optional[P] @classmethod @@ -59,7 +59,7 @@ def __init__( self.version = version self._sub_path = sub_path self._repo = None - self._aliases = {self.name: self.version or ""} + self._aliases = {self.name: (self.version or "", self.sub_path or "")} self._work_dir = work_dir @property @@ -160,6 +160,8 @@ 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}") + if alias in self._aliases: + return apath / self._aliases[alias][1] return apath / self._sub_path def alias_class_file(self, alias: str) -> P: @@ -206,7 +208,7 @@ def checkout(self): ) self._dependency.checkout_component(self.name, self.version) - def register_alias(self, alias: str, version: str): + def register_alias(self, alias: str, version: str, sub_path: str = ""): if not self._work_dir: raise ValueError( f"Can't register alias on component {self.name} " @@ -216,7 +218,7 @@ def register_alias(self, alias: str, version: str): raise ValueError( f"alias {alias} already registered on component {self.name}" ) - self._aliases[alias] = version + self._aliases[alias] = (version, sub_path) if self._dependency: self._dependency.register_component( alias, component_dir(self._work_dir, alias) @@ -231,9 +233,9 @@ def checkout_alias( ) if alias_dependency: - alias_dependency.checkout_component(alias, self._aliases[alias]) + alias_dependency.checkout_component(alias, self._aliases[alias][0]) elif self._dependency: - self._dependency.checkout_component(alias, self._aliases[alias]) + self._dependency.checkout_component(alias, self._aliases[alias][0]) def is_checked_out(self) -> bool: return self.target_dir is not None and self.target_dir.is_dir() diff --git a/commodore/dependency_mgmt/__init__.py b/commodore/dependency_mgmt/__init__.py index ca0f34afe..ad25dbdf6 100644 --- a/commodore/dependency_mgmt/__init__.py +++ b/commodore/dependency_mgmt/__init__.py @@ -124,7 +124,7 @@ def fetch_components(cfg: Config): c = components[component] aspec = cspecs[alias] adep = None - if aspec.url != c.repo_url or aspec.path != c._sub_path: + if aspec.url != c.repo_url: adep = cfg.register_dependency_repo(aspec.url) wdir = c.work_directory if not wdir: @@ -134,7 +134,7 @@ def fetch_components(cfg: Config): adep.register_component(alias, component_dir(wdir, alias)) print(alias, aspec) - c.register_alias(alias, aspec.version) + c.register_alias(alias, aspec.version, aspec.path) c.checkout_alias(alias, adep) create_alias_symlinks(cfg, c, alias) @@ -216,9 +216,8 @@ def register_components(cfg: Config): c = registered_components[cn] 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 aspec.url == c.repo_url and aspec.path == c.sub_path: + 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}'") diff --git a/commodore/dependency_mgmt/version_parsing.py b/commodore/dependency_mgmt/version_parsing.py index a30d1f0a8..aea883059 100644 --- a/commodore/dependency_mgmt/version_parsing.py +++ b/commodore/dependency_mgmt/version_parsing.py @@ -52,7 +52,7 @@ def parse( if base_config: url = info.get("url", base_config.url) version = info.get("version", base_config.version) - if path not in info: + if not path: path = base_config.path else: url = info["url"] diff --git a/tests/test_dependency_mgmt.py b/tests/test_dependency_mgmt.py index 9f7485443..38c040379 100644 --- a/tests/test_dependency_mgmt.py +++ b/tests/test_dependency_mgmt.py @@ -43,29 +43,37 @@ def setup_components_upstream( return component_specs -def setup_aliases_upstream(tmp_path: Path, aliases: Iterable[tuple[str, str]]): +def setup_aliases_upstream( + tmp_path: Path, aliases: Iterable[tuple[str, str]], sub_path: str = "" +): upstream = tmp_path / "upstream" component_specs = {} for alias, component in aliases: - component_specs[alias] = _prepare_repository(upstream, alias, component) + component_specs[alias] = _prepare_repository( + upstream, alias, component, sub_path + ) return component_specs -def _prepare_repository(upstream: Path, repo_name: str, component_name: str): +def _prepare_repository( + upstream: Path, repo_name: str, component_name: str, sub_path: str = "" +): repo_path = upstream / repo_name url = f"file://{repo_path.resolve()}" version = None repo = git.Repo.init(repo_path) - class_dir = repo_path / "class" + class_dir = repo_path / sub_path / "class" class_dir.mkdir(parents=True, exist_ok=True) (class_dir / "defaults.yml").touch(exist_ok=True) (class_dir / f"{component_name}.yml").touch(exist_ok=True) - repo.index.add(["class/defaults.yml", f"class/{component_name}.yml"]) + class_path = f"{sub_path}/class".strip("/") + + repo.index.add([f"{class_path}/defaults.yml", f"{class_path}/{component_name}.yml"]) repo.index.commit("component defaults") - return DependencySpec(url, version, "") + return DependencySpec(url, version, sub_path) def test_create_component_symlinks_fails(config: Config, tmp_path: Path, mockdep): @@ -193,13 +201,23 @@ def test_fetch_components_with_alias_version( patch_discover, patch_read, config: Config, tmp_path: Path ): components = ["component-one", "component-two"] - aliases = {"alias-one": "component-one", "alias-two": "component-two"} + aliases = { + "alias-one": "component-one", + "alias-two": "component-two", + "alias-three": "component-two", + } patch_discover.return_value = (components, aliases) forked_aliases = [("alias-two", "component-two")] + forked_aliases2 = [("alias-three", "component-two")] + # setup one alias with different repo aspecs = setup_aliases_upstream(tmp_path, forked_aliases) + # setup other alias with a subdirectory + aspecs2 = setup_aliases_upstream(tmp_path, forked_aliases2, "subdirectory") + rv = setup_components_upstream(tmp_path, components, aliases) rv["alias-two"] = aspecs["alias-two"] + rv["alias-three"] = aspecs2["alias-three"] patch_read.return_value = rv diff --git a/tests/test_dependency_mgmt_version_parsing.py b/tests/test_dependency_mgmt_version_parsing.py index b70a8d305..bfc340649 100644 --- a/tests/test_dependency_mgmt_version_parsing.py +++ b/tests/test_dependency_mgmt_version_parsing.py @@ -23,15 +23,37 @@ @patch.object(version_parsing, "kapitan_inventory") def test_read_components(patch_inventory, config: Config): - components = _setup_mock_inventory(patch_inventory) + components = _setup_mock_inventory( + patch_inventory, + aliases={"test-component": "test-alias", "other-component": "test-alias2"}, + ) + components["test-alias"] = { + "url": "https://example.com/test", + } + components["test-alias2"] = { + "version": "v0.0.1", + "path": "subpath", + } cspecs = version_parsing._read_components( - config, {"test-component": "test-component"} + config, + { + "test-component": "test-component", + "test-alias": "test-component", + "test-alias2": "other-component", + }, ) - # check that exactly 'test-component' is discovered - assert {"test-component"} == set(cspecs.keys()) + # check that exactly the listed components are discovered + assert {"other-component", "test-component", "test-alias", "test-alias2"} == set( + cspecs.keys() + ) assert components["test-component"]["url"] == cspecs["test-component"].url assert components["test-component"]["version"] == cspecs["test-component"].version + assert components["test-component"]["version"] == cspecs["test-alias"].version + assert components["test-alias2"]["version"] == cspecs["test-alias2"].version + assert components["test-alias"]["url"] == cspecs["test-alias"].url + assert components["other-component"]["url"] == cspecs["test-alias2"].url + assert components["test-alias2"]["path"] == cspecs["test-alias2"].path @patch.object(version_parsing, "kapitan_inventory") From 4eed4a827c7d95d499fa6cb21dcf0499320245da Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Fri, 24 Jan 2025 11:24:39 +0100 Subject: [PATCH 10/16] Increase coverage of error cases --- tests/test_dependency_mgmt.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/test_dependency_mgmt.py b/tests/test_dependency_mgmt.py index 38c040379..585eeaa8a 100644 --- a/tests/test_dependency_mgmt.py +++ b/tests/test_dependency_mgmt.py @@ -403,6 +403,26 @@ def test_register_components_and_aliases( assert alias not in aliases +@patch("commodore.dependency_mgmt._read_components") +@patch("commodore.dependency_mgmt._discover_components") +def test_register_components_and_aliases_raises( + patch_discover, patch_read, config: Config, tmp_path: Path +): + alias_data = {"fooer": "foo"} + component_dirs, other_dirs = _setup_register_components(tmp_path) + patch_discover.return_value = (component_dirs, alias_data) + patch_read.return_value = { + 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"] + + with pytest.raises(Exception) as excinfo: + dependency_mgmt.register_components(config) + + assert "Missing alias checkout for 'fooer as foo'" in str(excinfo.value) + + @patch("commodore.dependency_mgmt._read_components") @patch("commodore.dependency_mgmt._discover_components") def test_register_unknown_components( From ff83124f8a85e45624ff0f50a13e3f8ecb6e6ba7 Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Tue, 28 Jan 2025 16:25:02 +0100 Subject: [PATCH 11/16] Implement suggestions from code review --- commodore/config.py | 1 - commodore/dependency_mgmt/__init__.py | 19 +++++++------------ commodore/dependency_mgmt/version_parsing.py | 3 --- tests/test_component.py | 1 - 4 files changed, 7 insertions(+), 17 deletions(-) diff --git a/commodore/config.py b/commodore/config.py index db13ab13d..d13c0a1ed 100644 --- a/commodore/config.py +++ b/commodore/config.py @@ -380,7 +380,6 @@ def register_component_aliases(self, aliases: dict[str, str]): self._component_aliases.update(aliases) def verify_component_aliases(self, cluster_parameters: dict): - print(cluster_parameters) for alias, cn in self._component_aliases.items(): if alias != cn: if not _component_is_aliasable(cluster_parameters, cn): diff --git a/commodore/dependency_mgmt/__init__.py b/commodore/dependency_mgmt/__init__.py index ad25dbdf6..566477638 100644 --- a/commodore/dependency_mgmt/__init__.py +++ b/commodore/dependency_mgmt/__init__.py @@ -56,9 +56,6 @@ def create_alias_symlinks(cfg, component: Component, alias: str): 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): @@ -126,14 +123,8 @@ def fetch_components(cfg: Config): adep = None if aspec.url != c.repo_url: adep = cfg.register_dependency_repo(aspec.url) - wdir = c.work_directory - if not wdir: - raise ValueError( - "Cannot checkout repo for component alias if component does not have a working directory." - ) - adep.register_component(alias, component_dir(wdir, alias)) + adep.register_component(alias, component_dir(cfg.work_dir, alias)) - print(alias, aspec) c.register_alias(alias, aspec.version, aspec.path) c.checkout_alias(alias, adep) @@ -216,8 +207,12 @@ def register_components(cfg: Config): c = registered_components[cn] aspec = cspecs[alias] - if aspec.url == c.repo_url and aspec.path == c.sub_path: - c.register_alias(alias, aspec.version) + + if aspec.url != c.repo_url: + adep = cfg.register_dependency_repo(aspec.url) + adep.register_component(alias, component_dir(cfg.work_dir, alias)) + c.register_alias(alias, aspec.version, aspec.path) + if not component_dir(cfg.work_dir, alias).is_dir(): raise click.ClickException(f"Missing alias checkout for '{alias} as {cn}'") diff --git a/commodore/dependency_mgmt/version_parsing.py b/commodore/dependency_mgmt/version_parsing.py index aea883059..5318a901f 100644 --- a/commodore/dependency_mgmt/version_parsing.py +++ b/commodore/dependency_mgmt/version_parsing.py @@ -100,9 +100,6 @@ def _read_versions( try: 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), diff --git a/tests/test_component.py b/tests/test_component.py index 90ee83e99..ae7de71f6 100644 --- a/tests/test_component.py +++ b/tests/test_component.py @@ -61,7 +61,6 @@ def _setup_component( def _setup_existing_component(tmp_path: P, worktree=True): cr = Repo.init(tmp_path / ".repo", bare=True) upstream = tmp_path / "upstream" - cr u = cr.clone(upstream) with open(upstream / "README.md", "w") as f: f.write("# Dummy component\n") From fc7e23a7bb40bfd963763206ca6cc32b51db1ac5 Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Thu, 30 Jan 2025 11:19:07 +0100 Subject: [PATCH 12/16] Raise if alias was not registered when attempting to read alias directory --- commodore/component/__init__.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/commodore/component/__init__.py b/commodore/component/__init__.py index e5caffdfd..b0a214a04 100644 --- a/commodore/component/__init__.py +++ b/commodore/component/__init__.py @@ -160,9 +160,11 @@ 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}") - if alias in self._aliases: - return apath / self._aliases[alias][1] - return apath / self._sub_path + if alias not in self._aliases: + raise ValueError( + f"alias {alias} for component {self.name} has not been registered" + ) + return apath / self._aliases[alias][1] def alias_class_file(self, alias: str) -> P: return self.alias_directory(alias) / "class" / f"{self.name}.yml" From d9bc703ce3566753d3f0f12d955f477807fd92d6 Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Thu, 30 Jan 2025 13:44:39 +0100 Subject: [PATCH 13/16] Add documentation --- .../ROOT/pages/reference/architecture.adoc | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/docs/modules/ROOT/pages/reference/architecture.adoc b/docs/modules/ROOT/pages/reference/architecture.adoc index 69a094eb7..6777c2ef1 100644 --- a/docs/modules/ROOT/pages/reference/architecture.adoc +++ b/docs/modules/ROOT/pages/reference/architecture.adoc @@ -299,6 +299,52 @@ Commodore currently doesn't provide support for component authors to specify lib It's the responsibility of component authors to agree on an interface and to ensure that their implementations adhere to the interface. ==== +=== Using different component versions across instances + +With https://syn.tools/syn/SDDs/0033-commodore-component-instance-versioning.html[SDD0033], we've introduced support for instantiating components with different versions, meaning that different instances of the same component may use a different dependency or different versions of the same dependency. +Component authors must explicitly declare that their component supports multi-version instantiation. +Components advertise that they support multi-version instantiation by setting the field `multi_version` in `parameters.._metadata` to `true`. +Commodore will exit with an error if a hierarchy tries to override the version of a component instance where the component doesn't explicitly advertise multi-version support. + +Specifying the version of a component instance is done analogously to specifying the version of a base component or single-instance component. +The component version is specified in `parameters.components.`. The content is merged into `parameters.components.`. + +The version of the base component (`parameters.components.`) must always be specified explicitly, even if the component is only used with instance names. + +.tenant/common.yml +[source,yaml] +---- +applications: + - nfs-subdir-external-provisioner + - nfs-subdir-external-provisioner as nfs-2 + - nfs-subdir-external-provisioner as nfs-3 +parameters: + components: + nfs-subdir-external-provisioner: <1> + url: https://github.com/projectsyn/nfs-subdir-external-provisioner.git + version: v1.0.0 + nfs-2: + version: v1.1.0 <2> + nfs-3: + url: https://github.com/projectsyn/nfs-subdir-external-provisioner-fork.git <3> + version: v1.1.0 +---- +<1> The URL and version of the base component must always be specified. +Component instance version configurations will be merged into this base configuration. +<2> If only a version is specified for an instance, then the same URL as the base component will be used. +<3> It's possible to specify a different URL, for example, to use a fork of a component for this particular instance. + +[NOTE] +==== +If a component requires Jsonnet dependencies, those are always provided from the base (non-instantiated) version of the component. +In other words, if a component instance overrides the version, its Jsonnet dependencies are provided from a different component version. + +Similarly, if other components include references to a multi-version component's `defaults.yml` or to Jsonnet libraries provided by the multi-version component, then those files are always taken from the base (non-instantiated) +==== + + + + == Catalog Compilation Commodore uses https://kapitan.dev[Kapitan] to compile the cluster catalog. From 8329fcce1699e7cb69af23927dbc3cd060290708 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Fri, 31 Jan 2025 09:12:31 +0100 Subject: [PATCH 14/16] Update documentation to note that we now create a Git worktree per instance --- docs/modules/ROOT/pages/reference/architecture.adoc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/reference/architecture.adoc b/docs/modules/ROOT/pages/reference/architecture.adoc index 6777c2ef1..087c001c0 100644 --- a/docs/modules/ROOT/pages/reference/architecture.adoc +++ b/docs/modules/ROOT/pages/reference/architecture.adoc @@ -60,6 +60,10 @@ For each dependency which is stored in this repository, Commodore ensures that t Regardless of the value of key `path`, Commodore creates a checkout of the complete repository in `dependencies/`. However, Commodore will create a symlink to the specified path when making the dependency available in the hierarchy. +For components which are instantiated multiple times, Commodore ensures that an additional Git worktree in `dependencies/` exists for each component instance and is checked out to the instance's desired component version. +Once the Git worktree for a component instance exists, it's functionally mostly equivalent to a component Git worktree and is handled the same. +See the section on <<_component_instance_versions>> for details on specifying versions for different component instances and for cases where an instance Git worktree isn't fully equivalent to the base component Git worktree. + Commodore will make all included packages available before discovering and fetching components. This ensures that configuration packages can include additional components and can customize component versions. @@ -299,7 +303,7 @@ Commodore currently doesn't provide support for component authors to specify lib It's the responsibility of component authors to agree on an interface and to ensure that their implementations adhere to the interface. ==== -=== Using different component versions across instances +=== Component instance versions With https://syn.tools/syn/SDDs/0033-commodore-component-instance-versioning.html[SDD0033], we've introduced support for instantiating components with different versions, meaning that different instances of the same component may use a different dependency or different versions of the same dependency. Component authors must explicitly declare that their component supports multi-version instantiation. From 84bf538fdf33f18429ce63a7edff76545c8c2f73 Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Fri, 31 Jan 2025 11:11:53 +0100 Subject: [PATCH 15/16] Change "multi_versioning" to "multi_version" --- commodore/config.py | 2 +- tests/test_config.py | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/commodore/config.py b/commodore/config.py index d13c0a1ed..839ae6d7f 100644 --- a/commodore/config.py +++ b/commodore/config.py @@ -474,7 +474,7 @@ def _component_supports_alias_version( cmeta = cluster_parameters[ckey].get("_metadata", {}) akey = component_parameters_key(alias) ameta = cluster_parameters.get(akey, {}).get("_metadata", {}) - return cmeta.get("multi_versioning", False) and ameta.get("multi_versioning", False) + return cmeta.get("multi_version", False) and ameta.get("multi_version", False) def set_fact_value(facts: dict[str, Any], raw_key: str, value: Any) -> None: diff --git a/tests/test_config.py b/tests/test_config.py index 3a1a4fe6a..eba9dd629 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -49,11 +49,11 @@ def test_verify_component_aliases_explicit_no_multiversion_exception(config): "baz": {"version": "v1.1.0"}, }, "bar": { - "_metadata": {"multi_instance": True, "multi_versioning": False}, + "_metadata": {"multi_instance": True, "multi_version": False}, "namespace": "syn-bar", }, "baz": { - "_metadata": {"multi_instance": True, "multi_versioning": False}, + "_metadata": {"multi_instance": True, "multi_version": False}, "namespace": "syn-baz", }, } @@ -76,11 +76,11 @@ def test_verify_component_aliases_explicit_no_multiversion_in_alias_exception(co "baz": {"version": "v1.1.0"}, }, "bar": { - "_metadata": {"multi_instance": True, "multi_versioning": True}, + "_metadata": {"multi_instance": True, "multi_version": True}, "namespace": "syn-bar", }, "baz": { - "_metadata": {"multi_instance": True, "multi_versioning": False}, + "_metadata": {"multi_instance": True, "multi_version": False}, "namespace": "syn-baz", }, } @@ -123,10 +123,10 @@ def test_verify_component_multiversion(config): "baz": {"version": "v1.1.0"}, }, "bar": { - "_metadata": {"multi_instance": True, "multi_versioning": True}, + "_metadata": {"multi_instance": True, "multi_version": True}, "namespace": "syn-bar", }, - "baz": {"_metadata": {"multi_versioning": True}, "namespace": "syn-baz"}, + "baz": {"_metadata": {"multi_version": True}, "namespace": "syn-baz"}, } config.verify_component_aliases(params) From 21be94e92184ceb26c4406385885960a3fee1fc3 Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Fri, 31 Jan 2025 11:16:17 +0100 Subject: [PATCH 16/16] Update docs to incorporate PR feedback --- .../ROOT/pages/reference/architecture.adoc | 88 +++++++++---------- 1 file changed, 43 insertions(+), 45 deletions(-) diff --git a/docs/modules/ROOT/pages/reference/architecture.adoc b/docs/modules/ROOT/pages/reference/architecture.adoc index 087c001c0..9267da87c 100644 --- a/docs/modules/ROOT/pages/reference/architecture.adoc +++ b/docs/modules/ROOT/pages/reference/architecture.adoc @@ -192,6 +192,49 @@ Similar to Helm charts, the components themselves must make sure to not cause an This is required both for namespaced and non-namespaced resources. Components can make use of the meta-parameter `_instance` to ensure objects don't collide, as that parameter is guaranteed to be unique to each instance. +=== Component instance versions + +With https://syn.tools/syn/SDDs/0033-commodore-component-instance-versioning.html[SDD0033], we've introduced support for instantiating components with different versions, meaning that different instances of the same component may use a different dependency or different versions of the same dependency. +Component authors must explicitly declare that their component supports multi-version instantiation. +Components advertise that they support multi-version instantiation by setting the field `multi_version` in `parameters.._metadata` to `true`. +Commodore will exit with an error if a hierarchy tries to override the version of a component instance where the component doesn't explicitly advertise multi-version support. + +Specifying the version of a component instance is done analogously to specifying the version of a base component or single-instance component. +The component version is specified in `parameters.components.`. The content is merged into `parameters.components.`. + +The version of the base component (`parameters.components.`) must always be specified explicitly, even if the component is only used with instance names. + +.tenant/common.yml +[source,yaml] +---- +applications: + - nfs-subdir-external-provisioner + - nfs-subdir-external-provisioner as nfs-2 + - nfs-subdir-external-provisioner as nfs-3 +parameters: + components: + nfs-subdir-external-provisioner: <1> + url: https://github.com/projectsyn/nfs-subdir-external-provisioner.git + version: v1.0.0 + nfs-2: + version: v1.1.0 <2> + nfs-3: + url: https://github.com/projectsyn/nfs-subdir-external-provisioner-fork.git <3> + version: v1.1.0 +---- +<1> The URL and version of the base component must always be specified. +Component instance version configurations will be merged into this base configuration. +<2> If only a version is specified for an instance, then the same URL as the base component will be used. +<3> It's possible to specify a different URL, for example, to use a fork of a component for this particular instance. + +[NOTE] +==== +If a component requires Jsonnet dependencies, those are always provided from the base (non-instantiated) version of the component. +In other words, if a component instance overrides the version, its Jsonnet dependencies are provided from a different component version. + +Similarly, if other components include references to a multi-version component's `defaults.yml` or to Jsonnet libraries provided by the multi-version component, then those files are always taken from the base (non-instantiated) components. +==== + === Component dependencies Components can specify their dependencies in a `jsonnetfile.json`. @@ -303,51 +346,6 @@ Commodore currently doesn't provide support for component authors to specify lib It's the responsibility of component authors to agree on an interface and to ensure that their implementations adhere to the interface. ==== -=== Component instance versions - -With https://syn.tools/syn/SDDs/0033-commodore-component-instance-versioning.html[SDD0033], we've introduced support for instantiating components with different versions, meaning that different instances of the same component may use a different dependency or different versions of the same dependency. -Component authors must explicitly declare that their component supports multi-version instantiation. -Components advertise that they support multi-version instantiation by setting the field `multi_version` in `parameters.._metadata` to `true`. -Commodore will exit with an error if a hierarchy tries to override the version of a component instance where the component doesn't explicitly advertise multi-version support. - -Specifying the version of a component instance is done analogously to specifying the version of a base component or single-instance component. -The component version is specified in `parameters.components.`. The content is merged into `parameters.components.`. - -The version of the base component (`parameters.components.`) must always be specified explicitly, even if the component is only used with instance names. - -.tenant/common.yml -[source,yaml] ----- -applications: - - nfs-subdir-external-provisioner - - nfs-subdir-external-provisioner as nfs-2 - - nfs-subdir-external-provisioner as nfs-3 -parameters: - components: - nfs-subdir-external-provisioner: <1> - url: https://github.com/projectsyn/nfs-subdir-external-provisioner.git - version: v1.0.0 - nfs-2: - version: v1.1.0 <2> - nfs-3: - url: https://github.com/projectsyn/nfs-subdir-external-provisioner-fork.git <3> - version: v1.1.0 ----- -<1> The URL and version of the base component must always be specified. -Component instance version configurations will be merged into this base configuration. -<2> If only a version is specified for an instance, then the same URL as the base component will be used. -<3> It's possible to specify a different URL, for example, to use a fork of a component for this particular instance. - -[NOTE] -==== -If a component requires Jsonnet dependencies, those are always provided from the base (non-instantiated) version of the component. -In other words, if a component instance overrides the version, its Jsonnet dependencies are provided from a different component version. - -Similarly, if other components include references to a multi-version component's `defaults.yml` or to Jsonnet libraries provided by the multi-version component, then those files are always taken from the base (non-instantiated) -==== - - - == Catalog Compilation