Address review comments

src/python/pants/backend/helm/dependency_inference/deployment.py

@@ -23,7 +23,6 @@
     HelmDeploymentRequest,
     RenderedHelmFiles,
 )
-from pants.backend.helm.utils.docker import ImageRef
 from pants.backend.helm.utils.yaml import FrozenYamlIndex, MutableYamlIndex
 from pants.engine.addresses import Address
 from pants.engine.fs import Digest, DigestEntries, FileEntry
@@ -47,10 +46,10 @@
 @dataclass(frozen=True)
 class HelmDeploymentReport:
     address: Address
-    image_refs: FrozenYamlIndex[ImageRef]
+    image_refs: FrozenYamlIndex[str]
 
     @property
-    def all_image_refs(self) -> FrozenOrderedSet[ImageRef]:
+    def all_image_refs(self) -> FrozenOrderedSet[str]:
         return FrozenOrderedSet(self.image_refs.values())
 
 
@@ -75,25 +74,32 @@ async def analyse_deployment(field_set: HelmDeploymentFieldSet) -> HelmDeploymen
         if isinstance(entry, FileEntry)
     )
 
-    # Build YAML index of `ImageRef`s for future processing during depedendecy inference or post-rendering.
-    image_refs_index: MutableYamlIndex[ImageRef] = MutableYamlIndex()
+    # Build YAML index of Docker image refs for future processing during depedendecy inference or post-rendering.
+    image_refs_index: MutableYamlIndex[str] = MutableYamlIndex()
     for manifest in parsed_manifests:
         for (idx, path, image_ref) in manifest.found_image_refs:
             image_refs_index.insert(
                 file_path=PurePath(manifest.filename),
                 document_index=idx,
                 yaml_path=path,
-                item=ImageRef.parse(image_ref),
+                item=image_ref,
             )
 
     return HelmDeploymentReport(address=field_set.address, image_refs=image_refs_index.frozen())
 
 
 @dataclass(frozen=True)
 class FirstPartyHelmDeploymentMappings:
-    deployment_to_docker_addresses: FrozenDict[Address, FrozenYamlIndex[tuple[ImageRef, Address]]]
+    """A mapping between `helm_deployment` target addresses and tuples made up of a Docker image
+    reference and a `docker_image` target address.
 
-    def referenced_by(self, address: Address) -> list[tuple[ImageRef, Address]]:
+    The tuples of Docker image references and addresses are stored in a YAML index so we can track
+    the locations in which the Docker image refs appear in the deployment files.
+    """
+
+    deployment_to_docker_addresses: FrozenDict[Address, FrozenYamlIndex[tuple[str, Address]]]
+
+    def docker_addresses_referenced_by(self, address: Address) -> list[tuple[str, Address]]:
         if address not in self.deployment_to_docker_addresses:
             return []
         return list(self.deployment_to_docker_addresses[address].values())
@@ -110,7 +116,7 @@ async def first_party_helm_deployment_mappings(
 
     docker_target_addresses = {tgt.address.spec: tgt.address for tgt in docker_targets}
 
-    def lookup_docker_addreses(image_ref: ImageRef) -> tuple[ImageRef, Address] | None:
+    def lookup_docker_addreses(image_ref: str) -> tuple[str, Address] | None:
         addr = docker_target_addresses.get(str(image_ref), None)
         if addr:
             return image_ref, addr
@@ -144,7 +150,7 @@ async def inject_deployment_dependencies(
     explicitly_provided_deps = await Get(
         ExplicitlyProvidedDependencies, DependenciesRequest(request.field_set.dependencies)
     )
-    candidate_docker_addresses = mapping.referenced_by(request.field_set.address)
+    candidate_docker_addresses = mapping.docker_addresses_referenced_by(request.field_set.address)
 
     dependencies: OrderedSet[Address] = OrderedSet()
     for imager_ref, candidate_address in candidate_docker_addresses:
@@ -166,7 +172,6 @@ async def inject_deployment_dependencies(
     logging.debug(
         f"Found {pluralize(len(dependencies), 'dependency')} for target {request.field_set.address}"
     )
-
     return InferredDependencies(dependencies)
 
 

src/python/pants/backend/helm/dependency_inference/deployment_test.py

@@ -27,7 +27,6 @@
     K8S_SERVICE_TEMPLATE,
 )
 from pants.backend.helm.util_rules import chart, tool
-from pants.backend.helm.utils.docker import ImageRef
 from pants.backend.python.util_rules import pex
 from pants.core.util_rules import config_files, external_tool, stripped_source_files
 from pants.engine import process
@@ -101,9 +100,9 @@ def test_deployment_dependencies_report(rule_runner: RuleRunner) -> None:
     dependencies_report = rule_runner.request(HelmDeploymentReport, [field_set])
 
     expected_container_refs = [
-        ImageRef(registry=None, repository="busybox", tag="1.28"),
-        ImageRef(registry=None, repository="busybox", tag="1.29"),
-        ImageRef(registry="example.com", repository="containers/busybox", tag="1.28"),
+        "busybox:1.28",
+        "busybox:1.29",
+        "example.com/containers/busybox:1.28",
     ]
 
     assert len(dependencies_report.all_image_refs) == 3
@@ -146,11 +145,11 @@ def test_inject_deployment_dependencies(rule_runner: RuleRunner) -> None:
     deployment_addr = Address("src/deployment", target_name="foo")
     tgt = rule_runner.get_target(deployment_addr)
 
-    expected_image_ref = ImageRef.parse("src/image:myapp")
+    expected_image_ref = "src/image:myapp"
     expected_dependency_addr = Address("src/image", target_name="myapp")
 
     mappings = rule_runner.request(FirstPartyHelmDeploymentMappings, [])
-    assert mappings.referenced_by(deployment_addr) == [
+    assert mappings.docker_addresses_referenced_by(deployment_addr) == [
         (expected_image_ref, expected_dependency_addr)
     ]
 

src/python/pants/backend/helm/subsystems/post_renderer.py

@@ -113,6 +113,9 @@ async def setup_post_renderer_tool(
 
 @dataclass(frozen=True)
 class SetupHelmPostRenderer(EngineAwareParameter):
+    """Request for a post-renderer process that will perform a series of replacements in the
+    generated files."""
+
     replacements: FrozenYamlIndex[str]
     description_of_origin: str
 

src/python/pants/backend/helm/util_rules/post_renderer.py

@@ -24,7 +24,6 @@
 from pants.backend.helm.subsystems import post_renderer
 from pants.backend.helm.subsystems.post_renderer import HelmPostRenderer, SetupHelmPostRenderer
 from pants.backend.helm.target_types import HelmDeploymentFieldSet
-from pants.backend.helm.utils.docker import ImageRef
 from pants.engine.addresses import Address, Addresses
 from pants.engine.engine_aware import EngineAwareParameter
 from pants.engine.rules import Get, MultiGet, collect_rules, rule
@@ -110,7 +109,7 @@ def resolve_docker_image_ref(address: Address, context: DockerBuildContext) -> s
         for addr, ctx in zip(docker_addresses, docker_contexts)
     }
 
-    def find_replacement(value: tuple[ImageRef, Address]) -> str | None:
+    def find_replacement(value: tuple[str, Address]) -> str | None:
         _, addr = value
         return docker_addr_ref_mapping.get(addr)
 

src/python/pants/backend/helm/util_rules/renderer.py

@@ -92,13 +92,13 @@ def metadata(self) -> dict[str, Any] | None:
 
 
 @dataclass(frozen=True)
-class _HelmDeploymentAction(EngineAwareParameter, EngineAwareReturnType):
+class _HelmDeploymentProcessWrapper(EngineAwareParameter, EngineAwareReturnType):
     """Intermediate representation of a `HelmProcess` that will produce a fully rendered set of
     manifests from a given chart.
 
-    The encapsulated `process` will correspond to the `cmd` that was originally requested.
+    The encapsulated `process` will be side-effecting dependening on the `cmd` that was originally requested.
 
-    This is meant only to be used internally by this module
+    This is meant to only be used internally by this module.
     """
 
     chart: HelmChart
@@ -126,7 +126,7 @@ def level(self) -> LogLevel | None:
     def message(self) -> str | None:
         msg = softwrap(
             f"""
-            Built renderer for {self.address} using chart {self.chart.address}
+            Built deployment process for {self.address} using chart {self.chart.address}
             with{'out' if not self.output_directory else ''} a post-renderer stage
             """
         )
@@ -158,7 +158,7 @@ def message(self) -> str | None:
         return softwrap(
             f"""
             Generated {pluralize(len(self.snapshot.files), 'file')} from deployment {self.address}
-            using chart {self.chart}.
+            using chart {self.chart.address}.
             """
         )
 
@@ -218,10 +218,10 @@ def minimise_and_sort_subset(input_subset: set[str]) -> list[str]:
     return result
 
 
-@rule(desc="Prepare Helm deployment renderer", level=LogLevel.DEBUG)
+@rule(desc="Prepare Helm deployment renderer")
 async def setup_render_helm_deployment_process(
     request: HelmDeploymentRequest,
-) -> _HelmDeploymentAction:
+) -> _HelmDeploymentProcessWrapper:
     chart, value_files = await MultiGet(
         Get(HelmChart, FindHelmDeploymentChart(request.field_set)),
         Get(
@@ -322,7 +322,7 @@ def maybe_escape_string_value(value: str) -> str:
         output_directories=output_directories,
     )
 
-    return _HelmDeploymentAction(
+    return _HelmDeploymentProcessWrapper(
         cmd=request.cmd,
         chart=chart,
         process=process,
@@ -335,9 +335,9 @@ def maybe_escape_string_value(value: str) -> str:
 _HELM_OUTPUT_FILE_MARKER = "# Source: "
 
 
-@rule(desc="Run Helm deployment renderer", level=LogLevel.DEBUG)
-async def run_renderer(action: _HelmDeploymentAction) -> RenderedHelmFiles:
-    assert not action.is_side_effect
+@rule(desc="Render Helm deployment", level=LogLevel.DEBUG)
+async def run_renderer(process_wrapper: _HelmDeploymentProcessWrapper) -> RenderedHelmFiles:
+    assert not process_wrapper.is_side_effect
 
     def file_content(file_name: str, lines: Iterable[str]) -> FileContent:
         sanitised_lines = list(lines)
@@ -370,29 +370,35 @@ def parse_renderer_output(result: ProcessResult) -> list[FileContent]:
 
         return [file_content(file_name, lines) for file_name, lines in rendered_files.items()]
 
-    logger.debug(f"Running Helm renderer process for {action.address}")
-    result = await Get(ProcessResult, HelmProcess, action.process)
+    logger.debug(f"Rendering Helm files for {process_wrapper.address}")
+    result = await Get(ProcessResult, HelmProcess, process_wrapper.process)
 
     output_snapshot = EMPTY_SNAPSHOT
-    if not action.output_directory:
-        logger.debug(f"Parsing Helm renderer files from the process' output of {action.address}.")
+    if not process_wrapper.output_directory:
+        logger.debug(
+            f"Parsing Helm rendered files from the process' output of {process_wrapper.address}."
+        )
         output_snapshot = await Get(Snapshot, CreateDigest(parse_renderer_output(result)))
     else:
         logger.debug(
-            f"Obtaining Helm renderer files from the process' output directory of {action.address}."
+            f"Obtaining Helm rendered files from the process' output directory of {process_wrapper.address}."
         )
         output_snapshot = await Get(
-            Snapshot, RemovePrefix(result.output_digest, action.output_directory)
+            Snapshot, RemovePrefix(result.output_digest, process_wrapper.output_directory)
         )
 
-    return RenderedHelmFiles(address=action.address, chart=action.chart, snapshot=output_snapshot)
+    return RenderedHelmFiles(
+        address=process_wrapper.address, chart=process_wrapper.chart, snapshot=output_snapshot
+    )
 
 
 @rule
-async def materialize_deployment_action(action: _HelmDeploymentAction) -> InteractiveProcess:
-    assert action.is_side_effect
+async def materialize_deployment_process_wrapper_into_interactive_process(
+    process_wrapper: _HelmDeploymentProcessWrapper,
+) -> InteractiveProcess:
+    assert process_wrapper.is_side_effect
 
-    process = await Get(Process, HelmProcess, action.process)
+    process = await Get(Process, HelmProcess, process_wrapper.process)
     return InteractiveProcess.from_process(process)