Allow transforms to omit objects; ignore Helm lifecycle hooks

[hausdorff]

Resolves half of #665 (still missing docs).
Fixes #486.

[lukehoban]

Should we make this opt-in or opt-out?

[hausdorff]

We have ~no hope (at least in Q3) of actually supporting the full behavior of lifecycle hooks, so we should make it opt-in.

[ellismg]

I could be misreading the whole thin but I am a little worried that you and Luke have different thoughts as to what "opt-in" means here.

I think Luke's point is "should we be passing this ignoreHelmTesthook thing by default or just export ignoreHelmTesthook and have folks who need to explicitly pass it in as one of the cfg.transforms (i.e. opt-into removing them) and I think you are saying "to a first approximation, if any of these annotations are on the resource, you should have to do something explicit to not have them removed automatically (i.e. opt-into keeping them).

What is unclear to me is in your world how I opt into keeping them. What does that code look like?

[hausdorff]

I meant: we should get rid of them by default.

I will add a little option in the chartopts that causes us to keep these resources, but I don't think it will be used very often.

[hausdorff]

Ok, I've added the option, so you'd do the following. I do think we should pause and make sure that we believe that this is a good thing to add, though. We are not going to be able to support this really well, but it is what we advertise—helm template plus Pulumi.

new helm.v2.Chart("wordpress", { includeTestHookResources: true })

[hausdorff]

@lukehoban I think a bunch of your comments are from when I accidentally opened the PR without committing all my changes. We're preparing to merge, so lmk if you have any other issues here.

[hausdorff]

Forgot about python. one minute.

[ellismg]

FWIW, I do wonder if it is better to force folks to opt into this behavior. I believe that we had been saying that the model we have for our helm support is "it behaves as if you ran helm template and then did kubectl apply" which makes me think that this is behavior you should be opting into. It sounds like now our story is "it behaves as if you ran helm template, followed by kubectl apply, except that you strip out any resources with these test lifecycle annotations". I don't have the expertise to say which of these is correct, I just care about providing easy to understand guidance to folks so they can predict the behavior of the system.

[jasminen]

Do we know when this will be supported ? (on which version)

[nesl247]

Curious what the status of this is? It's actually a pain point with the helm charts.

Regarding which direction to go, I would absolutely say that the hooks should be removed by default. While the idea of simply making pulumi act as if the user ran helm template, and then kubectl apply sounds nice and simple, it doesn't mean it's the right thing to do.

I would venture to say that 90% of people using pulumi would expect to be to deploy any helm chart without issues. Having these hook based resources causes this to not work in many charts, as they have test pods. The few people who want to have these tests run, could manually opt into it in some way (I think there is already a feature in this PR for it).

While I imagine having pulumi may want to keep things simple in some regards, having a better experience that doesn't require 90% of users to remove resources to be able to deploy a chart makes more sense. I think the goal has to be to see what 90% of users want and expect.

[lblackstone]

I do think it would make more sense to disable the test hooks by default. As long as we document the behavior and provide a flag to enable them, I think that's closer to what users would expect. Pulumi enables much more robust testing anyway, so I don't see a lot of value in deploying test workloads by default.

[ameier38]

Just checking if there is any update on the status of this? Would really like this feature as I am having difficultly deploying charts such as stable/ambassador and stable/grafana which include these test pods.

[arnediekmann]

Are there any news on this? This still seems to be an issue with helm charts that contain tests (so most prominent ones, really). I use the following as a workaround based on #486 (comment) for now:

transformations: [
  (o: any) => {
    const annotations = (o.metadata && o.metadata.annotations) || {};
    const hook = annotations["helm.sh/hook"];
    if (hook === "test-success" || hook === "test-failure") {
      o.apiVersion = "v1";
      o.kind = "List";
      o.items = [];
    }
    return o;
  },
]

[arnediekmann]

Due to a recent regression (fixed by #1157) the workaround I mentioned from #486 is not working with version 2.3.0. Therefore I again stumbled across this issue here. Since the pr looks quite finished I still wonder: Is there a reason why this has not been merged yet? And is there an estimate when it will be?

[lblackstone]

I'm cutting the v2.3.1 release this morning which will fix that regression. I think the reason this never got merged was because it was only for NodeJS; we still need to implement support for the other three SDKs.

It looks like this fell through the cracks since the PR never merged, so I've added the issue to our backlog to take another look.

[arnediekmann]

@lblackstone thanks for releasing that fix! Will test it tomorrow morning (in my timezone 😉). Also thank you for clearing up why this PR is still ongoing. I'll keep my fingers crossed that it gets implemented across SDKs and merged soon.

[lblackstone]

#1467 fixes the test hook part of things, and we have a documented workaround for omitting resources with transformations, so I'm going to close this out now.