Make sure FirstPartyHelmDeploymentMapping doesn't get calculated if disabled

[grihabor]

Option [helm-infer].deployment_dependencies was added in #21282. Unfortunately, I missed one place where FirstPartyHelmDeploymentMapping is requested:

@rule(desc="Prepare Helm deployment post-renderer", level=LogLevel.DEBUG)
async def prepare_post_renderer_for_helm_deployment(
    request: HelmDeploymentPostRendererRequest,
    union_membership: UnionMembership,
    docker_options: DockerOptions,
) -> SetupHelmPostRenderer:
    mapping = await Get(
        FirstPartyHelmDeploymentMapping, FirstPartyHelmDeploymentMappingRequest(request.field_set)
    )
    ...

Here I move this logic inside the rule, so that all consumers correctly handle the case when [helm-infer].deployment_dependencies is set to false

[tdyas]

Would this option benefit from a test of any sort?

[grihabor]

Would this option benefit from a test of any sort?

I'm not sure if there is a good test that can check that we're not doing inference, but I can add one if you suggest how it should work exactly

[tgolsson]

I'm not sure if there is a good test that can check that we're not doing inference, but I can add one if you suggest how it should work exactly

Isn't this the exact same test as for doing inference, except that you the dependency shouldn't exist..?

[tdyas]

The reason why I'm asking for a test is primarily because this PR fixes a regression. And I generally want tests for regressions since they are failure modes which actually occurred.

The operative question in my view: How do we know that this PR in fact fixed the regression?

How did you account for tracking down all relevant call sites?

A test can provide proof. Or some additional narrative context on testing methodology could be fine as well.

[benjyw]

@grihabor Are you able to add a test and get this merged today? It is past time to get 2.23.0 out... :)

[grihabor]

Isn't this the exact same test as for doing inference, except that you the dependency shouldn't exist..?

If this test existed, it wouldn't have caught this problem, because it wouldn't have tested that we don't calculate FirstPartyHelmDeploymentMapping

[grihabor]

The operative question in my view: How do we know that this PR in fact fixed the regression?

I can explain how I noticed there is an issue. I ran this

$ pants experimental-deploy --no-experimental-deploy-publish-dependencies --dry-run :my-deployment

and got the error

UnownedDependencyError: Error resolving Docker image dependency of a Helm chart.
                `my-image:0.1.0` was supplied, but Pants cannot determine
                whether this should be a target's address or a 3rd-party dependency.
                If this should be an external image, add `my-image:0.1.0` to `[helm-infer].external_docker_images`
                If this should be a target address, use an absolute path instead (possibly `//my-image:0.1.0`).

This indirectly means that pants is trying to build FirstPartyHelmDeploymentMapping.

I could add the test that sets [helm-infer].unowned_dependency_behavior = "error" and [helm-infer].deployment_dependencies = false and makes sure there is no error, sounds good?

[tdyas]

I could add the test that sets [helm-infer].unowned_dependency_behavior = "error" and [helm-infer].deployment_dependencies = false and makes sure there is no error, sounds good?

That works for me.

[grihabor]

Done: b8b3d83

PS Notes are probably not required, because there is already a note about the option:

pants/docs/notes/2.23.x.md

Line 190 in b399758

Added [option `[helm-infer].deployment_dependencies`](https://www.pantsbuild.org/2.23/reference/subsystems/helm-infer#deployment_dependencies) to disable costly parsing of k8s manifests.

[WorkerPants]

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

✔️ 2.23.x

Successfully opened #21635.

---

Thanks again for your contributions!

🤖 Beep Boop here's my run link