From acd6e15ff0284ec3bc1dc09ec740eb53b22f51a5 Mon Sep 17 00:00:00 2001 From: Borodin Gregory <grihabor@gmail.com> Date: Tue, 12 Nov 2024 14:59:25 +0100 Subject: [PATCH 1/5] Make sure FirstPartyHelmDeploymentMapping doesn't get calculated if disabled --- .../helm/dependency_inference/deployment.py | 83 +++++++++++++------ 1 file changed, 58 insertions(+), 25 deletions(-) diff --git a/src/python/pants/backend/helm/dependency_inference/deployment.py b/src/python/pants/backend/helm/dependency_inference/deployment.py index 4c5a3c19bd1..7bfdd1b28a5 100644 --- a/src/python/pants/backend/helm/dependency_inference/deployment.py +++ b/src/python/pants/backend/helm/dependency_inference/deployment.py @@ -18,7 +18,10 @@ UnownedHelmDependencyUsage, ) from pants.backend.helm.subsystems import k8s_parser -from pants.backend.helm.subsystems.k8s_parser import ParsedKubeManifest, ParseKubeManifestRequest +from pants.backend.helm.subsystems.k8s_parser import ( + ParsedKubeManifest, + ParseKubeManifestRequest, +) from pants.backend.helm.target_types import HelmDeploymentFieldSet from pants.backend.helm.target_types import rules as helm_target_types_rules from pants.backend.helm.util_rules import renderer @@ -73,7 +76,9 @@ def metadata(self) -> dict[str, Any] | None: @rule(desc="Analyse Helm deployment", level=LogLevel.DEBUG) -async def analyse_deployment(request: AnalyseHelmDeploymentRequest) -> HelmDeploymentReport: +async def analyse_deployment( + request: AnalyseHelmDeploymentRequest, +) -> HelmDeploymentReport: rendered_deployment = await Get( RenderedHelmFiles, HelmDeploymentRequest( @@ -83,7 +88,9 @@ async def analyse_deployment(request: AnalyseHelmDeploymentRequest) -> HelmDeplo ), ) - rendered_entries = await Get(DigestEntries, Digest, rendered_deployment.snapshot.digest) + rendered_entries = await Get( + DigestEntries, Digest, rendered_deployment.snapshot.digest + ) parsed_manifests = await MultiGet( Get( ParsedKubeManifest, @@ -133,6 +140,29 @@ class FirstPartyHelmDeploymentMapping: @rule async def first_party_helm_deployment_mapping( request: FirstPartyHelmDeploymentMappingRequest, + helm_infer: HelmInferSubsystem, +) -> FirstPartyHelmDeploymentMapping: + if not helm_infer.deployment_dependencies: + return FirstPartyHelmDeploymentMapping( + request.field_set.address, + FrozenYamlIndex.empty(), + ) + # Use a small proxy rule to make sure we don't calculate AllDockerImageTargets + # if `[helm-infer].deployment_dependencies` is set to true. + return await Get( + FirstPartyHelmDeploymentMapping, + _FirstPartyHelmDeploymentMappingRequest(field_set=request.field_set), + ) + + +@dataclass(frozen=True) +class _FirstPartyHelmDeploymentMappingRequest(EngineAwareParameter): + field_set: HelmDeploymentFieldSet + + +@rule +async def _first_party_helm_deployment_mapping( + request: _FirstPartyHelmDeploymentMappingRequest, docker_targets: AllDockerImageTargets, helm_infer: HelmInferSubsystem, ) -> FirstPartyHelmDeploymentMapping: @@ -160,10 +190,14 @@ def image_ref_to_address_input(image_ref: str) -> tuple[str, AddressInput] | Non docker_target_addresses = {tgt.address for tgt in docker_targets} maybe_addresses_by_ref = { ref: maybe_addr - for ((ref, _), maybe_addr) in zip(indexed_address_inputs.values(), maybe_addresses) + for ((ref, _), maybe_addr) in zip( + indexed_address_inputs.values(), maybe_addresses + ) } - resolver = ImageReferenceResolver(helm_infer, maybe_addresses_by_ref, docker_target_addresses) + resolver = ImageReferenceResolver( + helm_infer, maybe_addresses_by_ref, docker_target_addresses + ) indexed_docker_addresses = indexed_address_inputs.transform_values( lambda image_ref_ai: resolver.image_ref_to_actual_address(image_ref_ai[0]) @@ -263,9 +297,15 @@ def report_errors(self): f"The behavior for unowned imports can also be set with the `[{HelmInferSubsystem.options_scope}].unowned_dependency_behavior`", ] ) - if self.helm_infer.unowned_dependency_behavior == UnownedHelmDependencyUsage.RaiseError: + if ( + self.helm_infer.unowned_dependency_behavior + == UnownedHelmDependencyUsage.RaiseError + ): raise UnownedDependencyError(message) - elif self.helm_infer.unowned_dependency_behavior == UnownedHelmDependencyUsage.LogWarning: + elif ( + self.helm_infer.unowned_dependency_behavior + == UnownedHelmDependencyUsage.LogWarning + ): logging.warning(message) else: return @@ -286,29 +326,22 @@ async def inject_deployment_dependencies( DependenciesRequest(request.field_set.dependencies), ) - if infer_subsystem.deployment_dependencies: - chart_address, explicitly_provided_deps, mapping = await MultiGet( - get_address, - get_explicit_deps, - Get( - FirstPartyHelmDeploymentMapping, - FirstPartyHelmDeploymentMappingRequest(request.field_set), - ), - ) - else: - (chart_address, explicitly_provided_deps), mapping = ( - await MultiGet(get_address, get_explicit_deps), - FirstPartyHelmDeploymentMapping( - request.field_set.address, - FrozenYamlIndex.empty(), - ), - ) + chart_address, explicitly_provided_deps, mapping = await MultiGet( + get_address, + get_explicit_deps, + Get( + FirstPartyHelmDeploymentMapping, + FirstPartyHelmDeploymentMappingRequest(request.field_set), + ), + ) dependencies: OrderedSet[Address] = OrderedSet() dependencies.add(chart_address) for imager_ref, candidate_address in mapping.indexed_docker_addresses.values(): - matches = frozenset([candidate_address]).difference(explicitly_provided_deps.includes) + matches = frozenset([candidate_address]).difference( + explicitly_provided_deps.includes + ) explicitly_provided_deps.maybe_warn_of_ambiguous_dependency_inference( matches, request.field_set.address, From ddb9d428df310d4b6c9c99cd4aeebe89c94529bb Mon Sep 17 00:00:00 2001 From: Borodin Gregory <grihabor@gmail.com> Date: Tue, 12 Nov 2024 15:06:34 +0100 Subject: [PATCH 2/5] Format file --- .../helm/dependency_inference/deployment.py | 27 +++++-------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/src/python/pants/backend/helm/dependency_inference/deployment.py b/src/python/pants/backend/helm/dependency_inference/deployment.py index 7bfdd1b28a5..cef201448bc 100644 --- a/src/python/pants/backend/helm/dependency_inference/deployment.py +++ b/src/python/pants/backend/helm/dependency_inference/deployment.py @@ -18,10 +18,7 @@ UnownedHelmDependencyUsage, ) from pants.backend.helm.subsystems import k8s_parser -from pants.backend.helm.subsystems.k8s_parser import ( - ParsedKubeManifest, - ParseKubeManifestRequest, -) +from pants.backend.helm.subsystems.k8s_parser import ParsedKubeManifest, ParseKubeManifestRequest from pants.backend.helm.target_types import HelmDeploymentFieldSet from pants.backend.helm.target_types import rules as helm_target_types_rules from pants.backend.helm.util_rules import renderer @@ -76,9 +73,7 @@ def metadata(self) -> dict[str, Any] | None: @rule(desc="Analyse Helm deployment", level=LogLevel.DEBUG) -async def analyse_deployment( - request: AnalyseHelmDeploymentRequest, -) -> HelmDeploymentReport: +async def analyse_deployment(request: AnalyseHelmDeploymentRequest) -> HelmDeploymentReport: rendered_deployment = await Get( RenderedHelmFiles, HelmDeploymentRequest( @@ -88,9 +83,7 @@ async def analyse_deployment( ), ) - rendered_entries = await Get( - DigestEntries, Digest, rendered_deployment.snapshot.digest - ) + rendered_entries = await Get(DigestEntries, Digest, rendered_deployment.snapshot.digest) parsed_manifests = await MultiGet( Get( ParsedKubeManifest, @@ -190,9 +183,7 @@ def image_ref_to_address_input(image_ref: str) -> tuple[str, AddressInput] | Non docker_target_addresses = {tgt.address for tgt in docker_targets} maybe_addresses_by_ref = { ref: maybe_addr - for ((ref, _), maybe_addr) in zip( - indexed_address_inputs.values(), maybe_addresses - ) + for ((ref, _), maybe_addr) in zip(indexed_address_inputs.values(), maybe_addresses) } resolver = ImageReferenceResolver( @@ -297,15 +288,9 @@ def report_errors(self): f"The behavior for unowned imports can also be set with the `[{HelmInferSubsystem.options_scope}].unowned_dependency_behavior`", ] ) - if ( - self.helm_infer.unowned_dependency_behavior - == UnownedHelmDependencyUsage.RaiseError - ): + if self.helm_infer.unowned_dependency_behavior == UnownedHelmDependencyUsage.RaiseError: raise UnownedDependencyError(message) - elif ( - self.helm_infer.unowned_dependency_behavior - == UnownedHelmDependencyUsage.LogWarning - ): + elif self.helm_infer.unowned_dependency_behavior == UnownedHelmDependencyUsage.LogWarning: logging.warning(message) else: return From 11a35ca47772d5f165b387004d80bd0df028bf1f Mon Sep 17 00:00:00 2001 From: Borodin Gregory <grihabor@gmail.com> Date: Tue, 12 Nov 2024 15:08:01 +0100 Subject: [PATCH 3/5] Format file --- .../pants/backend/helm/dependency_inference/deployment.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/python/pants/backend/helm/dependency_inference/deployment.py b/src/python/pants/backend/helm/dependency_inference/deployment.py index cef201448bc..8f6cc7a8e33 100644 --- a/src/python/pants/backend/helm/dependency_inference/deployment.py +++ b/src/python/pants/backend/helm/dependency_inference/deployment.py @@ -186,9 +186,7 @@ def image_ref_to_address_input(image_ref: str) -> tuple[str, AddressInput] | Non for ((ref, _), maybe_addr) in zip(indexed_address_inputs.values(), maybe_addresses) } - resolver = ImageReferenceResolver( - helm_infer, maybe_addresses_by_ref, docker_target_addresses - ) + resolver = ImageReferenceResolver(helm_infer, maybe_addresses_by_ref, docker_target_addresses) indexed_docker_addresses = indexed_address_inputs.transform_values( lambda image_ref_ai: resolver.image_ref_to_actual_address(image_ref_ai[0]) @@ -324,9 +322,7 @@ async def inject_deployment_dependencies( dependencies.add(chart_address) for imager_ref, candidate_address in mapping.indexed_docker_addresses.values(): - matches = frozenset([candidate_address]).difference( - explicitly_provided_deps.includes - ) + matches = frozenset([candidate_address]).difference(explicitly_provided_deps.includes) explicitly_provided_deps.maybe_warn_of_ambiguous_dependency_inference( matches, request.field_set.address, From c11db1bd2de20bca6372a53b24b4113771546646 Mon Sep 17 00:00:00 2001 From: Borodin Gregory <grihabor@gmail.com> Date: Tue, 12 Nov 2024 15:22:38 +0100 Subject: [PATCH 4/5] Fix test --- .../backend/helm/dependency_inference/deployment.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/python/pants/backend/helm/dependency_inference/deployment.py b/src/python/pants/backend/helm/dependency_inference/deployment.py index 8f6cc7a8e33..f1972ff5987 100644 --- a/src/python/pants/backend/helm/dependency_inference/deployment.py +++ b/src/python/pants/backend/helm/dependency_inference/deployment.py @@ -130,6 +130,11 @@ class FirstPartyHelmDeploymentMapping: indexed_docker_addresses: FrozenYamlIndex[tuple[str, Address]] +@dataclass(frozen=True) +class _FirstPartyHelmDeploymentMappingRequest(EngineAwareParameter): + field_set: HelmDeploymentFieldSet + + @rule async def first_party_helm_deployment_mapping( request: FirstPartyHelmDeploymentMappingRequest, @@ -148,11 +153,6 @@ async def first_party_helm_deployment_mapping( ) -@dataclass(frozen=True) -class _FirstPartyHelmDeploymentMappingRequest(EngineAwareParameter): - field_set: HelmDeploymentFieldSet - - @rule async def _first_party_helm_deployment_mapping( request: _FirstPartyHelmDeploymentMappingRequest, From b8b3d834c37adffc354243965f660e37838520ca Mon Sep 17 00:00:00 2001 From: Borodin Gregory <grihabor@gmail.com> Date: Tue, 12 Nov 2024 20:10:55 +0100 Subject: [PATCH 5/5] Add a test --- .../dependency_inference/deployment_test.py | 76 +++++++++++++++++-- 1 file changed, 71 insertions(+), 5 deletions(-) diff --git a/src/python/pants/backend/helm/dependency_inference/deployment_test.py b/src/python/pants/backend/helm/dependency_inference/deployment_test.py index 3689370dd02..a2bba802937 100644 --- a/src/python/pants/backend/helm/dependency_inference/deployment_test.py +++ b/src/python/pants/backend/helm/dependency_inference/deployment_test.py @@ -62,7 +62,10 @@ def rule_runner() -> RuleRunner: *process.rules(), *stripped_source_files.rules(), *tool.rules(), - QueryRule(FirstPartyHelmDeploymentMapping, (FirstPartyHelmDeploymentMappingRequest,)), + QueryRule( + FirstPartyHelmDeploymentMapping, + (FirstPartyHelmDeploymentMappingRequest,), + ), QueryRule(HelmDeploymentReport, (AnalyseHelmDeploymentRequest,)), QueryRule(InferredDependencies, (InferHelmDeploymentDependenciesRequest,)), ], @@ -102,7 +105,8 @@ def test_deployment_dependencies_report(rule_runner: RuleRunner) -> None: source_root_patterns = ("/src/*",) rule_runner.set_options( - [f"--source-root-patterns={repr(source_root_patterns)}"], env_inherit=PYTHON_BOOTSTRAP_ENV + [f"--source-root-patterns={repr(source_root_patterns)}"], + env_inherit=PYTHON_BOOTSTRAP_ENV, ) target = rule_runner.get_target(Address("src/deployment", target_name="foo")) @@ -230,13 +234,17 @@ def test_resolve_relative_docker_addresses_to_deployment( def make_request(): return rule_runner.request( - FirstPartyHelmDeploymentMapping, [FirstPartyHelmDeploymentMappingRequest(field_set)] + FirstPartyHelmDeploymentMapping, + [FirstPartyHelmDeploymentMappingRequest(field_set)], ) if correct_target_name: expected = [ (":myapp0", Address("src/deployment", target_name="myapp0")), - ("//src/deployment:myapp1", Address("src/deployment", target_name="myapp1")), + ( + "//src/deployment:myapp1", + Address("src/deployment", target_name="myapp1"), + ), ("./subdir:myapp2", Address("src/deployment/subdir", target_name="myapp2")), ] assert list(make_request().indexed_docker_addresses.values()) == expected @@ -377,7 +385,8 @@ def test_inject_deployment_dependencies(rule_runner: RuleRunner) -> None: expected_dependency_addr = Address("src/image", target_name="myapp") mapping = rule_runner.request( - FirstPartyHelmDeploymentMapping, [FirstPartyHelmDeploymentMappingRequest(field_set)] + FirstPartyHelmDeploymentMapping, + [FirstPartyHelmDeploymentMappingRequest(field_set)], ) assert list(mapping.indexed_docker_addresses.values()) == [ (expected_image_ref, expected_dependency_addr) @@ -447,3 +456,60 @@ def test_disambiguate_docker_dependency(rule_runner: RuleRunner) -> None: # Assert only the Helm chart dependency has been inferred assert len(inferred_dependencies.include) == 1 assert set(inferred_dependencies.include) == {Address("src/mychart")} + + +def test_dont_infer_docker_dependency(rule_runner: RuleRunner) -> None: + rule_runner.write_files( + { + "src/mychart/BUILD": "helm_chart()", + "src/mychart/Chart.yaml": HELM_CHART_FILE, + "src/mychart/values.yaml": HELM_VALUES_FILE, + "src/mychart/templates/_helpers.tpl": HELM_TEMPLATE_HELPERS_FILE, + "src/mychart/templates/pod.yaml": dedent( + """\ + apiVersion: v1 + kind: Pod + metadata: + name: {{ template "fullname" . }} + labels: + chart: "{{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}" + spec: + containers: + - name: myapp-container + image: registry/image:latest + """ + ), + "src/deployment/BUILD": dedent( + """\ + helm_deployment( + name="foo", + chart="//src/mychart", + ) + """ + ), + "src/docker/BUILD": "docker_image(name='latest')", + "src/docker/Dockerfile": "FROM busybox:1.28", + } + ) + + source_root_patterns = ("/", "src/*") + rule_runner.set_options( + [ + f"--source-root-patterns={repr(source_root_patterns)}", + "--no-helm-infer-deployment-dependencies", + "--helm-infer-unowned-dependency-behavior=error", + ], + env_inherit=PYTHON_BOOTSTRAP_ENV, + ) + + deployment_addr = Address("src/deployment", target_name="foo") + tgt = rule_runner.get_target(deployment_addr) + + inferred_dependencies = rule_runner.request( + InferredDependencies, + [InferHelmDeploymentDependenciesRequest(HelmDeploymentFieldSet.create(tgt))], + ) + + # Assert only the Helm chart dependency has been inferred + assert len(inferred_dependencies.include) == 1 + assert set(inferred_dependencies.include) == {Address("src/mychart")}