From ed94338cdefa7e17991cc2866bd70f6ee2be5dde Mon Sep 17 00:00:00 2001 From: Allen Porter Date: Thu, 21 Dec 2023 08:08:04 -0800 Subject: [PATCH] Remove complexity from multi-cluster diff (#447) Remove the unnecessary code from multi-cluster diff. Multi-cluster repos are still supported, just not with a single diff command. --- flux_local/git_repo.py | 5 -- flux_local/tool/diff.py | 46 +++++--------- flux_local/tool/visitor.py | 74 +++++----------------- tests/__snapshots__/test_git_repo.ambr | 10 --- tests/test_git_repo.py | 12 ++-- tests/tool/__snapshots__/test_diff_ks.ambr | 12 ++-- 6 files changed, 42 insertions(+), 117 deletions(-) diff --git a/flux_local/git_repo.py b/flux_local/git_repo.py index 3ef5902b..8c2fdd3b 100644 --- a/flux_local/git_repo.py +++ b/flux_local/git_repo.py @@ -200,7 +200,6 @@ class ResourceVisitor: func: Callable[ [ - Path, Path, Kustomization | HelmRelease | HelmRepository | ClusterPolicy, kustomize.Kustomize | None, @@ -546,7 +545,6 @@ async def build_kustomization( if kustomization_selector.visitor: await kustomization_selector.visitor.func( - cluster_path, Path(kustomization.path), kustomization, cmd, @@ -675,7 +673,6 @@ async def update_kustomization(cluster: Cluster) -> None: for kustomization in cluster.kustomizations: for helm_repo in kustomization.helm_repos: await selector.helm_repo.visitor.func( - Path(cluster.path), Path(kustomization.path), helm_repo, None, @@ -685,7 +682,6 @@ async def update_kustomization(cluster: Cluster) -> None: for kustomization in cluster.kustomizations: for helm_release in kustomization.helm_releases: await selector.helm_release.visitor.func( - Path(cluster.path), Path(kustomization.path), helm_release, None, @@ -695,7 +691,6 @@ async def update_kustomization(cluster: Cluster) -> None: for kustomization in cluster.kustomizations: for cluster_policy in kustomization.cluster_policies: await selector.cluster_policy.visitor.func( - Path(cluster.path), Path(kustomization.path), cluster_policy, None, diff --git a/flux_local/tool/diff.py b/flux_local/tool/diff.py index 6968430b..04b5c972 100644 --- a/flux_local/tool/diff.py +++ b/flux_local/tool/diff.py @@ -162,11 +162,10 @@ def perform_yaml_diff( def get_helm_release_diff_keys( a: ObjectOutput, b: ObjectOutput -) -> dict[str, list[ResourceKey]]: +) -> list[ResourceKey]: """Return HelmRelease resource keys with diffs, by cluster.""" - result: dict[str, list[ResourceKey]] = {} + results: list[ResourceKey] = [] for kustomization_key in _unique_keys(a.content, b.content): - cluster_path = kustomization_key.cluster_path _LOGGER.debug("Diffing results for Kustomization %s", kustomization_key) a_resources = a.content.get(kustomization_key, {}) b_resources = b.content.get(kustomization_key, {}) @@ -174,8 +173,8 @@ def get_helm_release_diff_keys( if resource_key.kind != "HelmRelease": continue if a_resources.get(resource_key) != b_resources.get(resource_key): - result[cluster_path] = result.get(cluster_path, []) + [resource_key] - return result + results.append(resource_key) + return results def add_diff_flags(args: ArgumentParser) -> None: @@ -370,31 +369,20 @@ async def run( # type: ignore[no-untyped-def] # This avoid building unnecessary resources and churn from things like # random secret generation. diff_resource_keys = get_helm_release_diff_keys(orig_content, content) - cluster_paths = { - kustomization_key.cluster_path - for kustomization_key in set(orig_content.content.keys()) - | set(content.content.keys()) + diff_names = { + f"{resource_key.namespace}/{resource_key.name}" + for resource_key in diff_resource_keys } - for cluster_path in cluster_paths: - diff_keys = diff_resource_keys.get(cluster_path, []) - diff_names = { - f"{resource_key.namespace}/{resource_key.name}" - for resource_key in diff_keys - } - if cluster_path in helm_visitor.releases: - releases = [ - release - for release in helm_visitor.releases[cluster_path] - if f"{release.namespace}/{release.name}" in diff_names - ] - helm_visitor.releases[cluster_path] = releases - if cluster_path in orig_helm_visitor.releases: - releases = [ - release - for release in orig_helm_visitor.releases[cluster_path] - if f"{release.namespace}/{release.name}" in diff_names - ] - orig_helm_visitor.releases[cluster_path] = releases + helm_visitor.releases = [ + release + for release in helm_visitor.releases + if release.namespaced_name in diff_names + ] + orig_helm_visitor.releases = [ + release + for release in orig_helm_visitor.releases + if release.namespaced_name in diff_names + ] helm_content = ObjectOutput(strip_attrs) orig_helm_content = ObjectOutput(strip_attrs) diff --git a/flux_local/tool/visitor.py b/flux_local/tool/visitor.py index 03214ced..7a7d6d17 100644 --- a/flux_local/tool/visitor.py +++ b/flux_local/tool/visitor.py @@ -39,17 +39,12 @@ class ResourceKey: """Key for a Kustomization object output.""" - cluster_path: str kustomization_path: str kind: str namespace: str name: str def __post_init__(self) -> None: - if self.cluster_path.startswith("/"): - raise AssertionError( - f"Expected cluster_path as relative: {self.cluster_path}" - ) if self.kustomization_path.startswith("/"): raise AssertionError( f"Expected kustomization_path as relative: {self.kustomization_path}" @@ -63,9 +58,6 @@ def label(self) -> str: if self.kustomization_path and self.kustomization_path != ".": parts.append(self.kustomization_path) parts.append(" ") - elif self.cluster_path: - parts.append(self.cluster_path) - parts.append(" ") parts.append(self.compact_label) return "".join(parts) @@ -95,7 +87,6 @@ def visitor(self) -> git_repo.ResourceVisitor: @abstractmethod async def call_async( self, - cluster_path: pathlib.Path, kustomization_path: pathlib.Path, doc: ResourceType, cmd: Kustomize | None, @@ -104,12 +95,10 @@ async def call_async( def key_func( self, - cluster_path: pathlib.Path, kustomization_path: pathlib.Path, resource: ResourceType, ) -> ResourceKey: return ResourceKey( - cluster_path=str(cluster_path), kustomization_path=str(kustomization_path), kind=resource.__class__.__name__, namespace=resource.namespace or "", @@ -126,7 +115,6 @@ def __init__(self) -> None: async def call_async( self, - cluster_path: pathlib.Path, kustomization_path: pathlib.Path, doc: ResourceType, cmd: Kustomize | None, @@ -137,7 +125,7 @@ async def call_async( lines = content.split("\n") if lines[0] != "---": lines.insert(0, "---") - self.content[self.key_func(cluster_path, kustomization_path, doc)] = lines + self.content[self.key_func(kustomization_path, doc)] = lines def strip_attrs(metadata: dict[str, Any], strip_attributes: list[str]) -> None: @@ -159,14 +147,11 @@ class ImageOutput(ResourceOutput): def __init__(self) -> None: """Initialize ObjectOutput.""" - # Map of kustomizations to the map of built objects as lines of the yaml string - self.content: dict[ResourceKey, dict[ResourceKey, list[str]]] = {} self.image_visitor = image.ImageVisitor() self.repo_visitor = self.image_visitor.repo_visitor() async def call_async( self, - cluster_path: pathlib.Path, kustomization_path: pathlib.Path, doc: ResourceType, cmd: Kustomize | None, @@ -203,7 +188,6 @@ def __init__(self, strip_attributes: list[str] | None) -> None: async def call_async( self, - cluster_path: pathlib.Path, kustomization_path: pathlib.Path, doc: ResourceType, cmd: Kustomize | None, @@ -232,7 +216,6 @@ async def call_async( strip_attrs(meta, self.strip_attributes) resource_key = ResourceKey( kind=kind, - cluster_path=str(cluster_path), kustomization_path=str(kustomization_path), namespace=metadata.get("namespace", doc.namespace), name=metadata.get("name", ""), @@ -242,12 +225,11 @@ async def call_async( lines.insert(0, "---") contents[resource_key] = lines self.content[ - self.key_func(cluster_path, kustomization_path, doc) + self.key_func(kustomization_path, doc) ] = contents async def inflate_release( - cluster_path: pathlib.Path, helm: Helm, release: HelmRelease, visitor: git_repo.ResourceVisitor, @@ -255,7 +237,7 @@ async def inflate_release( ) -> None: cmd = await helm.template(release, options) # We can ignore the Kustomiation path since we're essentially grouping by cluster - await visitor.func(cluster_path, pathlib.Path(""), release, cmd) + await visitor.func(pathlib.Path(""), release, cmd) class HelmVisitor: @@ -263,18 +245,19 @@ class HelmVisitor: def __init__(self) -> None: """Initialize KustomizationContentOutput.""" - self.repos: dict[str, list[HelmRepository]] = {} - self.releases: dict[str, list[HelmRelease]] = {} + self.repos: list[HelmRepository] = [] + self.releases: list[HelmRelease] = [] - def active_repos(self, cluster_path: str) -> list[HelmRepository]: + @property + def active_repos(self) -> list[HelmRepository]: """Return HelpRepositories referenced by a HelmRelease.""" repo_keys: set[str] = { f"{release.chart.repo_namespace}-{release.chart.repo_name}" - for release in self.releases.get(cluster_path, []) + for release in self.releases } return [ repo - for repo in self.repos.get(cluster_path, []) + for repo in self.repos if repo.repo_name in repo_keys ] @@ -282,16 +265,13 @@ def repo_visitor(self) -> git_repo.ResourceVisitor: """Return a git_repo.ResourceVisitor that points to this object.""" async def add_repo( - cluster_path: pathlib.Path, kustomization_path: pathlib.Path, doc: ResourceType, cmd: Kustomize | None, ) -> None: if not isinstance(doc, HelmRepository): raise ValueError(f"Expected HelmRepository: {doc}") - self.repos[str(cluster_path)] = self.repos.get(str(cluster_path), []) + [ - doc - ] + self.repos.append(doc) return git_repo.ResourceVisitor(func=add_repo) @@ -299,16 +279,13 @@ def release_visitor(self) -> git_repo.ResourceVisitor: """Return a git_repo.ResourceVisitor that points to this object.""" async def add_release( - cluster_path: pathlib.Path, kustomization_path: pathlib.Path, doc: ResourceType, cmd: Kustomize | None, ) -> None: if not isinstance(doc, HelmRelease): raise ValueError(f"Expected HelmRelease: {doc}") - self.releases[str(cluster_path)] = self.releases.get( - str(cluster_path), [] - ) + [doc] + self.releases.append(doc) return git_repo.ResourceVisitor(func=add_release) @@ -319,43 +296,22 @@ async def inflate( options: Options, ) -> None: """Expand and notify about HelmReleases discovered.""" - cluster_paths = set(list(self.releases)) | set(list(self.repos)) - tasks = [ - self.inflate_cluster( - helm_cache_dir, - pathlib.Path(cluster_path), - visitor, - options, - ) - for cluster_path in cluster_paths - ] - _LOGGER.debug("Waiting for cluster inflation to complete") - await asyncio.gather(*tasks) - - async def inflate_cluster( - self, - helm_cache_dir: pathlib.Path, - cluster_path: pathlib.Path, - visitor: git_repo.ResourceVisitor, - options: Options, - ) -> None: - _LOGGER.debug("Inflating Helm charts in cluster %s", cluster_path) + _LOGGER.debug("Inflating Helm charts in cluster") if not self.releases: return with tempfile.TemporaryDirectory() as tmp_dir: helm = Helm(pathlib.Path(tmp_dir), helm_cache_dir) - if active_repos := self.active_repos(str(cluster_path)): + if active_repos := self.active_repos: helm.add_repos(active_repos) await helm.update() tasks = [ inflate_release( - cluster_path, helm, release, visitor, options, ) - for release in self.releases.get(str(cluster_path), []) + for release in self.releases ] - _LOGGER.debug("Waiting for tasks to inflate %s", cluster_path) + _LOGGER.debug("Waiting for inflate tasks to complete") await asyncio.gather(*tasks) diff --git a/tests/__snapshots__/test_git_repo.ambr b/tests/__snapshots__/test_git_repo.ambr index 614a4ced..0095f538 100644 --- a/tests/__snapshots__/test_git_repo.ambr +++ b/tests/__snapshots__/test_git_repo.ambr @@ -290,19 +290,16 @@ # name: test_helm_release_visitor.1 list([ tuple( - 'tests/testdata/cluster', 'tests/testdata/cluster/apps/prod', 'podinfo', 'podinfo', ), tuple( - 'tests/testdata/cluster', 'tests/testdata/cluster/infrastructure/controllers', 'flux-system', 'weave-gitops', ), tuple( - 'tests/testdata/cluster', 'tests/testdata/cluster/infrastructure/controllers', 'metallb', 'metallb', @@ -500,19 +497,16 @@ # name: test_helm_repo_visitor.1 list([ tuple( - 'tests/testdata/cluster', 'tests/testdata/cluster/infrastructure/configs', 'flux-system', 'bitnami', ), tuple( - 'tests/testdata/cluster', 'tests/testdata/cluster/infrastructure/configs', 'flux-system', 'podinfo', ), tuple( - 'tests/testdata/cluster', 'tests/testdata/cluster/infrastructure/configs', 'flux-system', 'weave-charts', @@ -980,25 +974,21 @@ # name: test_kustomization_visitor.1 list([ tuple( - 'tests/testdata/cluster', 'tests/testdata/cluster/apps/prod', 'flux-system', 'apps', ), tuple( - 'tests/testdata/cluster', 'tests/testdata/cluster/clusters/prod', 'flux-system', 'flux-system', ), tuple( - 'tests/testdata/cluster', 'tests/testdata/cluster/infrastructure/configs', 'flux-system', 'infra-configs', ), tuple( - 'tests/testdata/cluster', 'tests/testdata/cluster/infrastructure/controllers', 'flux-system', 'infra-controllers', diff --git a/tests/test_git_repo.py b/tests/test_git_repo.py index f1c1bfd3..ee0128f1 100644 --- a/tests/test_git_repo.py +++ b/tests/test_git_repo.py @@ -87,8 +87,8 @@ async def test_kustomization_visitor(snapshot: SnapshotAssertion) -> None: stream = io.StringIO() visits: list[tuple[str, str, str, str]] = [] - async def write(w: Path, x: Path, y: Any, cmd: Kustomize | None) -> None: - visits.append((str(w), str(x), y.namespace, y.name)) + async def write(x: Path, y: Any, cmd: Kustomize | None) -> None: + visits.append((str(x), y.namespace, y.name)) if cmd: stream.write(await cmd.run()) @@ -114,8 +114,8 @@ async def test_helm_repo_visitor(snapshot: SnapshotAssertion) -> None: visits: list[tuple[str, str, str, str]] = [] - async def append(w: Path, x: Path, y: Any, z: Any) -> None: - visits.append((str(w), str(x), y.namespace, y.name)) + async def append(x: Path, y: Any, z: Any) -> None: + visits.append((str(x), y.namespace, y.name)) query.helm_repo.visitor = ResourceVisitor(func=append) @@ -134,8 +134,8 @@ async def test_helm_release_visitor(snapshot: SnapshotAssertion) -> None: visits: list[tuple[str, str, str, str]] = [] - async def append(w: Path, x: Path, y: Any, z: Any) -> None: - visits.append((str(w), str(x), y.namespace, y.name)) + async def append(x: Path, y: Any, z: Any) -> None: + visits.append((str(x), y.namespace, y.name)) query.helm_release.visitor = ResourceVisitor( func=append, diff --git a/tests/tool/__snapshots__/test_diff_ks.ambr b/tests/tool/__snapshots__/test_diff_ks.ambr index 64239e82..33bd579a 100644 --- a/tests/tool/__snapshots__/test_diff_ks.ambr +++ b/tests/tool/__snapshots__/test_diff_ks.ambr @@ -8,14 +8,12 @@ # name: test_diff_ks[yaml-empty-sources] ''' --- - - cluster_path: tests/testdata/cluster - kustomization_path: tests/testdata/cluster/apps/prod + - kustomization_path: tests/testdata/cluster/apps/prod kind: Kustomization namespace: flux-system name: apps diffs: - - cluster_path: tests/testdata/cluster - kustomization_path: tests/testdata/cluster/apps/prod + - kustomization_path: tests/testdata/cluster/apps/prod kind: Namespace namespace: flux-system name: podinfo @@ -35,8 +33,7 @@ - kustomize.toolkit.fluxcd.io/namespace: flux-system - name: podinfo - - - cluster_path: tests/testdata/cluster - kustomization_path: tests/testdata/cluster/apps/prod + - kustomization_path: tests/testdata/cluster/apps/prod kind: HelmRelease namespace: podinfo name: podinfo @@ -84,8 +81,7 @@ - repository: public.ecr.aws/docker/library/redis - tag: 7.0.6 - - - cluster_path: tests/testdata/cluster - kustomization_path: tests/testdata/cluster/apps/prod + - kustomization_path: tests/testdata/cluster/apps/prod kind: ConfigMap namespace: podinfo name: podinfo-config