Simplify Helm package build and remove special case for `value_or_def…

…ault` in `output_path`

src/python/pants/backend/helm/goals/package.py

@@ -11,32 +11,25 @@
 from pants.backend.helm.util_rules.chart import HelmChart, HelmChartMetadata, HelmChartRequest
 from pants.backend.helm.util_rules.tool import HelmProcess
 from pants.core.goals.package import BuiltPackage, BuiltPackageArtifact, PackageFieldSet
-from pants.engine.fs import AddPrefix, CreateDigest, Digest, Directory, MergeDigests, RemovePrefix
+from pants.engine.fs import (
+    AddPrefix,
+    CreateDigest,
+    Digest,
+    Directory,
+    MergeDigests,
+    RemovePrefix,
+    Snapshot,
+)
 from pants.engine.process import ProcessResult
 from pants.engine.rules import Get, MultiGet, collect_rules, rule
+from pants.engine.unions import UnionRule
 from pants.util.logging import LogLevel
 
 logger = logging.getLogger(__name__)
 
 
-@dataclass(frozen=True)
-class BuiltHelmArtifact(BuiltPackageArtifact):
-    name: str | None = None
-    metadata: HelmChartMetadata | None = None
-
-    @classmethod
-    def create(cls, path_to_dir: str, chart_metadata: HelmChartMetadata) -> BuiltHelmArtifact:
-        path = os.path.join(path_to_dir, _helm_artifact_filename(chart_metadata))
-        return cls(
-            name=chart_metadata.artifact_name,
-            metadata=chart_metadata,
-            relpath=path,
-            extra_log_lines=(f"Built Helm chart artifact: {path}",),
-        )
-
-
 def _helm_artifact_filename(chart_metadata: HelmChartMetadata) -> str:
-    return f"{chart_metadata.artifact_name}.tgz"
+    return f"{chart_metadata.name}-{chart_metadata.version}.tgz"
 
 
 @dataclass(frozen=True)
@@ -70,10 +63,18 @@ async def run_helm_package(field_set: HelmPackageFieldSet) -> BuiltPackage:
         Digest, RemovePrefix(process_result.output_digest, result_dir)
     )
 
-    artifact_path = field_set.output_path.value_or_default(file_ending=None)
-    dest_digest = await Get(Digest, AddPrefix(stripped_output_digest, artifact_path))
-    return BuiltPackage(dest_digest, (BuiltHelmArtifact.create(artifact_path, chart.metadata),))
+    final_snapshot = await Get(
+        Snapshot,
+        AddPrefix(stripped_output_digest, field_set.output_path.value_or_default(file_ending=None)),
+    )
+    return BuiltPackage(
+        final_snapshot.digest,
+        artifacts=tuple(
+            BuiltPackageArtifact(file, extra_log_lines=(f"Built Helm chart artifact: {file}",))
+            for file in final_snapshot.files
+        ),
+    )
 
 
 def rules():
-    return collect_rules()
+    return [*collect_rules(), UnionRule(PackageFieldSet, HelmPackageFieldSet)]

src/python/pants/backend/helm/goals/package_test.py

@@ -8,7 +8,7 @@
 import pytest
 
 from pants.backend.helm.goals import package
-from pants.backend.helm.goals.package import BuiltHelmArtifact, HelmPackageFieldSet
+from pants.backend.helm.goals.package import HelmPackageFieldSet
 from pants.backend.helm.subsystems.helm import HelmSubsystem
 from pants.backend.helm.target_types import HelmChartTarget
 from pants.backend.helm.testutil import (
@@ -18,11 +18,9 @@
     gen_chart_file,
 )
 from pants.backend.helm.util_rules import chart, sources, tool
-from pants.backend.helm.util_rules.chart import HelmChartMetadata
 from pants.build_graph.address import Address
 from pants.core.goals.package import BuiltPackage
 from pants.core.util_rules import config_files, external_tool, stripped_source_files
-from pants.engine.fs import Snapshot
 from pants.engine.rules import QueryRule, SubsystemRule
 from pants.source.source_root import rules as source_root_rules
 from pants.testutil.rule_runner import RuleRunner
@@ -47,27 +45,19 @@ def rule_runner() -> RuleRunner:
     )
 
 
-def _assert_build_package(
-    rule_runner: RuleRunner, *, chart_name: str, chart_version: str, output_path: str = ""
-) -> None:
-    target = rule_runner.get_target(Address("", target_name=chart_name))
-    field_set = HelmPackageFieldSet.create(target)
+def _assert_build_package(rule_runner: RuleRunner, *, chart_name: str, chart_version: str) -> None:
+    rule_runner.set_options(["--source-root-patterns=['src/*']"])
 
-    expected_metadata = HelmChartMetadata(
-        name=chart_name,
-        version=chart_version,
-    )
-    expected_built_package = BuiltHelmArtifact.create(output_path, expected_metadata)
+    target = rule_runner.get_target(Address(f"src/{chart_name}", target_name=chart_name))
+    field_set = HelmPackageFieldSet.create(target)
 
+    dest_dir = field_set.output_path.value_or_default(file_ending=None)
     result = rule_runner.request(BuiltPackage, [field_set])
-    chart_snapshot = rule_runner.request(Snapshot, [result.digest])
 
     assert len(result.artifacts) == 1
-    assert result.artifacts[0] == expected_built_package
-
-    if output_path != "":
-        assert chart_snapshot.dirs == (output_path,)
-    assert chart_snapshot.files == (os.path.join(output_path, f"{chart_name}-{chart_version}.tgz"),)
+    assert result.artifacts[0].relpath == os.path.join(
+        dest_dir, f"{chart_name}-{chart_version}.tgz"
+    )
 
 
 def test_helm_package(rule_runner: RuleRunner) -> None:
@@ -76,11 +66,11 @@ def test_helm_package(rule_runner: RuleRunner) -> None:
 
     rule_runner.write_files(
         {
-            "BUILD": f"helm_chart(name='{chart_name}')",
-            "Chart.yaml": gen_chart_file(chart_name, version=chart_version),
-            "values.yaml": HELM_VALUES_FILE,
-            "templates/_helpers.tpl": HELM_TEMPLATE_HELPERS_FILE,
-            "templates/service.yaml": K8S_SERVICE_FILE,
+            f"src/{chart_name}/BUILD": f"helm_chart(name='{chart_name}')",
+            f"src/{chart_name}/Chart.yaml": gen_chart_file(chart_name, version=chart_version),
+            f"src/{chart_name}/values.yaml": HELM_VALUES_FILE,
+            f"src/{chart_name}/templates/_helpers.tpl": HELM_TEMPLATE_HELPERS_FILE,
+            f"src/{chart_name}/templates/service.yaml": K8S_SERVICE_FILE,
         }
     )
 
@@ -95,14 +85,12 @@ def test_helm_package_with_custom_output_path(rule_runner: RuleRunner) -> None:
 
     rule_runner.write_files(
         {
-            "BUILD": f"""helm_chart(name="{chart_name}", output_path="{output_path}")""",
-            "Chart.yaml": gen_chart_file(chart_name, version=chart_version),
-            "values.yaml": HELM_VALUES_FILE,
-            "templates/_helpers.tpl": HELM_TEMPLATE_HELPERS_FILE,
-            "templates/service.yaml": K8S_SERVICE_FILE,
+            f"src/{chart_name}/BUILD": f"""helm_chart(name="{chart_name}", output_path="{output_path}")""",
+            f"src/{chart_name}/Chart.yaml": gen_chart_file(chart_name, version=chart_version),
+            f"src/{chart_name}/values.yaml": HELM_VALUES_FILE,
+            f"src/{chart_name}/templates/_helpers.tpl": HELM_TEMPLATE_HELPERS_FILE,
+            f"src/{chart_name}/templates/service.yaml": K8S_SERVICE_FILE,
         }
     )
 
-    _assert_build_package(
-        rule_runner, chart_name=chart_name, chart_version=chart_version, output_path=output_path
-    )
+    _assert_build_package(rule_runner, chart_name=chart_name, chart_version=chart_version)

src/python/pants/backend/helm/target_types.py

@@ -3,9 +3,7 @@
 
 from __future__ import annotations
 
-import os
 from dataclasses import dataclass
-from textwrap import dedent
 
 from pants.core.goals.package import OutputPathField
 from pants.engine.target import (
@@ -60,18 +58,23 @@ class HelmChartDependenciesField(Dependencies):
 
 
 class HelmChartOutputPathField(OutputPathField):
-    help = dedent(
-        """\
-        The destination folder where the final packaged chart will be located.\n
-        The final package name will still follow Helm convention, this output path will only affect the destination folder where can be found.
-        """
+    help = (
+        "Where the built directory tree should be located.\n\n"
+        "If undefined, this will use the path to the BUILD file, "
+        "For example, `src/charts/mychart:mychart` would be "
+        "`src.charts.mychart/mychart/`.\n\n"
+        "Regardless of whether you use the default or set this field, the path will end with "
+        "Helms's file format of `<chart_name>-<chart_version>.tgz`, where "
+        "`chart_name` and `chart_version` are the values extracted from the Chart.yaml file. "
+        "So, using the default for this field, the target "
+        "`src/charts/mychart` might have a final path like "
+        "`src.charts.mychart/mychart-0.1.0.tgz`.\n\n"
+        f"When running `{bin_name()} package`, this path will be prefixed by `--distdir` (e.g. "
+        "`dist/`).\n\n"
+        "Warning: setting this value risks naming collisions with other package targets you may "
+        "have."
     )
 
-    def value_or_default(self, *, file_ending: str | None) -> str:
-        if self.value:
-            return self.value
-        return os.path.join(self.address.spec_path.replace(os.sep, "."))
-
 
 class HelmChartLintStrictField(TriBoolField):
     alias = "lint_strict"

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

@@ -191,10 +191,6 @@ def from_dict(cls, d: dict[str, Any]) -> HelmChartMetadata:
     def from_bytes(cls, content: bytes) -> HelmChartMetadata:
         return cls.from_dict(yaml.safe_load(content))
 
-    @property
-    def artifact_name(self) -> str:
-        return f"{self.name}-{self.version}"
-
     def to_dict(self) -> dict[str, Any]:
         d: dict[str, Any] = {
             "apiVersion": self.api_version,