From 62d7637b08cd3278278f01d581f2b1fa014d2e56 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Fri, 31 Jan 2025 11:45:39 +0100 Subject: [PATCH] Support different versions per instance of a component (#559) This PR changes Commodore to * 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 symlinked from the alias worktree * Read instance versions from `parameters.components.` Note that per-instance versions only work correctly only for components which use `${_base_directory}` in their config when specifying Jsonnet files or Helm chart/YAML locations in `kapitan.compile` and `kapitan.dependencies`. Note, that components should use `${_base_directory}` anyway, and new components created from the template use `${_base_directory}` out of the box. Components must signal that they support per-instance versions by setting component parameter `_metadata.multi_version=true`. Resolves #563 Co-authored-by: Aline Abler Co-authored-by: Aline Abler --- README.md | 6 +- commodore/cluster.py | 18 +- commodore/compile.py | 2 +- commodore/component/__init__.py | 72 ++++++- commodore/config.py | 32 ++- commodore/dependency_mgmt/__init__.py | 75 ++++++- commodore/dependency_mgmt/version_parsing.py | 57 ++++- commodore/helpers.py | 1 - commodore/inventory/__init__.py | 4 - commodore/postprocess/__init__.py | 4 - commodore/postprocess/jsonnet.py | 8 +- .../ROOT/pages/reference/architecture.adoc | 48 +++++ tests/conftest.py | 27 ++- tests/test_config.py | 92 +++++++++ tests/test_dependency_mgmt.py | 195 ++++++++++++++++-- tests/test_dependency_mgmt_version_parsing.py | 36 +++- tests/test_postprocess.py | 3 + tests/test_render_inventory.py | 6 +- tests/test_target.py | 44 +++- 19 files changed, 636 insertions(+), 94 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/cluster.py b/commodore/cluster.py index 8a6cbb33e..71934e41e 100644 --- a/commodore/cluster.py +++ b/commodore/cluster.py @@ -169,7 +169,9 @@ def generate_target( "_instance": target, } if not bootstrap: - parameters["_base_directory"] = str(components[component].target_directory) + parameters["_base_directory"] = str( + components[component].alias_directory(target) + ) parameters["_kustomize_wrapper"] = str(__kustomize_wrapper__) parameters["kapitan"] = { "vars": { @@ -206,24 +208,28 @@ def render_target( classes = [f"params.{inv.bootstrap_target}"] for c in sorted(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}") return generate_target(inv, target, components, classes, component) -# 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/compile.py b/commodore/compile.py index ddc865b04..45f7e1a1a 100644 --- a/commodore/compile.py +++ b/commodore/compile.py @@ -240,7 +240,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 1fac662d4..b0a214a04 100644 --- a/commodore/component/__init__.py +++ b/commodore/component/__init__.py @@ -19,6 +19,8 @@ class Component: _version: Optional[str] = None _dir: P _sub_path: str + _aliases: dict[str, tuple[str, str]] + _work_dir: Optional[P] @classmethod def clone(cls, cfg, clone_url: str, name: str, version: str = "master"): @@ -57,6 +59,8 @@ def __init__( self.version = version self._sub_path = sub_path self._repo = None + self._aliases = {self.name: (self.version or "", self.sub_path or "")} + self._work_dir = work_dir @property def name(self) -> str: @@ -67,8 +71,12 @@ def repo(self) -> GitRepo: if not self._repo: if self._dependency: dep_repo = self._dependency.bare_repo - author_name = dep_repo.author.name - author_email = dep_repo.author.email + author_name = ( + dep_repo.author.name if hasattr(dep_repo, "author") else None + ) + author_email = ( + dep_repo.author.email if hasattr(dep_repo, "author") else None + ) else: # Fall back to author detection if we don't have a dependency author_name = None @@ -126,9 +134,13 @@ def sub_path(self) -> str: def repo_directory(self) -> P: return self._dir + @property + def work_directory(self) -> Optional[P]: + return self._work_dir + @property def target_directory(self) -> P: - return self._dir / self._sub_path + return self.alias_directory(self.name) @property def target_dir(self) -> P: @@ -136,11 +148,32 @@ def target_dir(self) -> P: @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: + if not self._dependency: + return self._dir / self._sub_path + apath = self._dependency.get_component(alias) + if not apath: + raise ValueError(f"unknown alias {alias} for component {self.name}") + 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" + + 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]: @@ -177,6 +210,35 @@ def checkout(self): ) self._dependency.checkout_component(self.name, self.version) + 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} " + + "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, sub_path) + if self._dependency: + self._dependency.register_component( + alias, component_dir(self._work_dir, alias) + ) + + 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}" + ) + + if alias_dependency: + alias_dependency.checkout_component(alias, self._aliases[alias][0]) + elif self._dependency: + 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/config.py b/commodore/config.py index cb2e2608a..839ae6d7f 100644 --- a/commodore/config.py +++ b/commodore/config.py @@ -377,14 +377,26 @@ 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): 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") 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." + ) def get_component_alias_versioninfos(self) -> dict[str, InstanceVersionInfo]: return { @@ -453,6 +465,18 @@ def _component_is_aliasable(cluster_parameters: dict, component_name: str): 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_version", False) and ameta.get("multi_version", 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/commodore/dependency_mgmt/__init__.py b/commodore/dependency_mgmt/__init__.py index 50822d783..566477638 100644 --- a/commodore/dependency_mgmt/__init__.py +++ b/commodore/dependency_mgmt/__init__.py @@ -40,6 +40,24 @@ 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, + ) + + def create_package_symlink(cfg, pname: str, package: Package): """ Create package symlink in the inventory. @@ -69,7 +87,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) deps: dict[str, list] = {} @@ -93,6 +111,25 @@ def fetch_components(cfg: Config): deps.setdefault(cdep.url, []).append(c) fetch_parallel(fetch_component, cfg, deps.values()) + components = cfg.get_components() + + for alias, component in component_aliases.items(): + if alias == component: + # Nothing to setup for identity alias + continue + + c = components[component] + aspec = cspecs[alias] + adep = None + 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) + c.checkout_alias(alias, adep) + + create_alias_symlinks(cfg, c, alias) + def fetch_component(cfg, dependencies): """ @@ -126,7 +163,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) @@ -152,9 +189,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: @@ -163,6 +200,24 @@ 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] + aspec = cspecs[alias] + + 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}'") + + create_alias_symlinks(cfg, c, alias) + def fetch_packages(cfg: Config): """ @@ -235,10 +290,18 @@ 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 "url" not in cspec: + if cname in aliases: + # 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})" + ) + elif "url" not in cspec: errors.append(f"component '{cname}'") for pname, pspec in cluster_parameters.get("packages", {}).items(): diff --git a/commodore/dependency_mgmt/version_parsing.py b/commodore/dependency_mgmt/version_parsing.py index ed475322e..5318a901f 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 not path: + 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,23 @@ 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) + 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 +120,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( diff --git a/commodore/helpers.py b/commodore/helpers.py index 71226dce5..9801a89e1 100644 --- a/commodore/helpers.py +++ b/commodore/helpers.py @@ -338,7 +338,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 925ad1d68..23e07c820 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 563ec0883..1e48a1cbc 100644 --- a/commodore/postprocess/__init__.py +++ b/commodore/postprocess/__init__.py @@ -35,18 +35,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): diff --git a/commodore/postprocess/jsonnet.py b/commodore/postprocess/jsonnet.py index 8a9b917db..fea45acad 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") diff --git a/docs/modules/ROOT/pages/reference/architecture.adoc b/docs/modules/ROOT/pages/reference/architecture.adoc index 69a094eb7..9267da87c 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. @@ -188,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`. @@ -299,6 +346,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. ==== + == Catalog Compilation Commodore uses https://kapitan.dev[Kapitan] to compile the cluster catalog. diff --git a/tests/conftest.py b/tests/conftest.py index 3b1e4645c..a5b895e2e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -109,33 +109,38 @@ 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] @property def bare_repo(self) -> GitRepo: - return GitRepo(None, self._target_dir) + return self._repo @pytest.fixture diff --git a/tests/test_config.py b/tests/test_config.py index 9f636df40..eba9dd629 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -40,6 +40,98 @@ def test_verify_component_aliases_explicit_no_instance(config): config.verify_component_aliases(params) +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_version": False}, + "namespace": "syn-bar", + }, + "baz": { + "_metadata": {"multi_instance": True, "multi_version": 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_version": True}, + "namespace": "syn-bar", + }, + "baz": { + "_metadata": {"multi_instance": True, "multi_version": 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_version": True}, + "namespace": "syn-bar", + }, + "baz": {"_metadata": {"multi_version": True}, "namespace": "syn-baz"}, + } + + config.verify_component_aliases(params) + + def test_verify_component_aliases_metadata(config): alias_data = {"baz": "bar"} config.register_component_aliases(alias_data) diff --git a/tests/test_dependency_mgmt.py b/tests/test_dependency_mgmt.py index 36320b8d5..585eeaa8a 100644 --- a/tests/test_dependency_mgmt.py +++ b/tests/test_dependency_mgmt.py @@ -27,28 +27,55 @@ 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) + + for alias in aliases.keys(): + if aliases[alias] == component: + component_specs[alias] = component_specs[component] + + return component_specs - 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) - repo.index.add(["class/defaults.yml", f"class/{component}.yml"]) - repo.index.commit("component defaults") - component_specs[component] = DependencySpec(url, version, "") +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, sub_path + ) return component_specs +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 / 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) + + 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, sub_path) + + 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: @@ -168,6 +195,54 @@ def test_fetch_components(patch_discover, patch_read, config: Config, tmp_path: 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", + "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 + + 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") def test_fetch_components_raises( @@ -247,7 +322,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"] @@ -263,6 +338,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 @@ -292,13 +375,16 @@ 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", "") for cn in component_dirs } + patch_read.return_value["fooer"] = patch_read.return_value["foo"] dependency_mgmt.register_components(config) @@ -317,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( @@ -343,19 +449,22 @@ 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 = { 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) @@ -393,7 +502,7 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: @pytest.mark.parametrize( - "cluster_params,expected", + "cluster_params,aliases,expected", [ ( { @@ -412,6 +521,7 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: }, }, }, + {}, "", ), ( @@ -432,6 +542,7 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: }, }, }, + {}, "Version override specified for component 'component_1' which has no URL", ), ( @@ -453,6 +564,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", ), ( @@ -469,6 +581,7 @@ def test_validate_component_library_name(tmp_path: Path, libname: str, expected: } }, }, + {}, "Version override specified for package 'package-1' which has no URL", ), ( @@ -484,6 +597,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", ), ( @@ -502,17 +616,60 @@ 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) diff --git a/tests/test_dependency_mgmt_version_parsing.py b/tests/test_dependency_mgmt_version_parsing.py index ca2adf511..bfc340649 100644 --- a/tests/test_dependency_mgmt_version_parsing.py +++ b/tests/test_dependency_mgmt_version_parsing.py @@ -23,19 +23,43 @@ @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"]) + 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", + "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") 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()) @@ -81,7 +105,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 diff --git a/tests/test_postprocess.py b/tests/test_postprocess.py index 9ac70963e..a65b8cccb 100644 --- a/tests/test_postprocess.py +++ b/tests/test_postprocess.py @@ -144,6 +144,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 40074d181..54fc90235 100644 --- a/tests/test_target.py +++ b/tests/test_target.py @@ -2,6 +2,8 @@ Unit-tests for target generation """ +from __future__ import annotations + import os import click import pytest @@ -17,18 +19,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 +48,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 +122,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.bar", - "defaults.foo", + "defaults.fooer", "global.commodore", - "components.foo", + "components.fooer", ] assert target != "" print(target) @@ -130,13 +150,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 +164,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.bar", - "defaults.foo-comp", + "defaults.foo-1", "global.commodore", - "components.foo-comp", + "components.foo-1", ] assert target != "" print(target) @@ -163,7 +185,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):