fix: add helm

[afterthought]

functional, but could use improvements.

Off the top of my head:

I run dependency update inside a genrule.
This requires env vars as passthrough to login to helm because
otherwise, the genrule cannot see the context from a previous login.

Documentation for ENV vars.

[Tatskaari]

Nice one! Thanks for the PR.

I don't know too much about helm, but I had a few questions. Hopefully they're not too silly.

[Tatskaari]

nit: if you're passing both the named outputs, you can just use :_replaced_files

[afterthought]

I can tweak/test this. I was learning plz when doing this...

[Tatskaari]

From your comment here:

This requires env vars as passthrough to login to helm because
otherwise, the genrule cannot see the context from a previous login.

I'm just wondering what this context is. Why does it need to log in to access it?

I'm also wondering if we can get away with only doing the update when we run one of the script targets below?

[afterthought]

Helm allows you define sub charts, which is roughly akin to dependencies in a language framework package manager. These do not point to a central repo automatically. You can use a local relative file path, e.g. file://../ which points to other charts in the same parent folder. Or you can point to an OCI compatible registry like ACR, ECR, GCR, etc.

Running the update command creates/updates a lock file. For starters, this part of the PR does smell a bit. I tried a few other variations of this and landed here. The ideal use case (I think) would be for the user to maintain this lock file in source they way one might maintain package-lock.json, etc. I believe helm supports semantic ranges, etc and has a similar process for bumping dependencies within a rule/pattern. I used the pleasings/docker build_def as a guide when writing this and as a result, the Chart.yaml file includes variable markers, e.g. ${VERSION} and targets e.g. //charts/subchart:mychart. This is similar to how the docker def implements a "base image". When the source file is doctored with plz syntax like this it breaks the file. docker build with a Dockerfile that uses a plz target as a base image will break unless it is run using the special plz run target. In the same way, if I run helm update with these changes, it breaks.

Hopefully this explains more why it is this way. This led me to dynamically build this lock file inside the please context each time. Happy to help talk through a better approach.

[Tatskaari]

Would we be able to avoid logging in if we could download or build all the charts we depend on in other build rules and then use local file:// based URLs instead of remote registries?

If so, we could label sub-charts with something like helm-chart:src/my_service/k8s/my_chart.tar.gz, and pick these up in a pre_build function. The pre-build function can then update your command to set up these file paths. We do something similar for linker flags:

cc_library(
    ...
    linker_flags = "--pthread",
)

Adds cc:ld:--pthread as a label to the target. The cc_binary() rule then picks these up in a pre-build via get_labels(name, "cc:"), and updates it's command in accordingly. It's a little grungy but works quite well.

https://github.com/thought-machine/please/blob/master/rules/cc_rules.build_defs#L742

[afterthought]

I think that makes sense. The other rule would still need to login though if I understand correctly. For our use case it doesn't make sense to switch to file:// since we want to publish the new chart with the correct remote dependency.

[Tatskaari]

If this works, I'm reasonably happy to merge this and iterate as we go forward. I'm planning on moving rules out of the pleasings repo into their own repo (using the new plugin API) as they become mature enough. Ideally we'd not need to set up env variables just to build this stuff by then.

[Tatskaari]

Given that we're doing helm dependency update here, do we need to run the update at build time? It would simplify things if we don't.

[afterthought]

I think the reason I ended up like this (running it in two places) is because for awhile I was attempting a process that let me run plz to maintain the lock file in source. I think you're right though. I should simplify it. Assuming the design doesn't change around managing the lock file.

[Tatskaari]

Why did this need to be forked? Are there changes here?

[afterthought]

There are two changes. I needed to set the pass_env to params.keys() to get the substitutions working with ENV vars. This probably wouldn't force forking. I also altered the list of fqn's used for templating docker image tags to also template charts. Notice there is a sub_charts param now and the code now generates fqns for these as well and concats with containers.

I forked it because I realize how super awkward that is for a k8s build_def. Makes no sense imho. But I wanted to progress.

Looking at the code now, I think I'm realizing that I could avoid this by concatenating the containers/subcharts before calling this build_def and passing both to the containers variable. Still a little awkward, but not as bad as this.

[afterthought]

I'm not in a rush to merge this. I am slowly making some improvements. I did manage to find a nice clean way to remove the login code (using system library to point to ~/.config/helm.....). I'll post an update once I hit a decent stopping point.