Open originalrkk opened 1 year ago
~Seems we don't use file://...
for dependency inference, we just use the chart name and glue it together ref. I think it makes sense to also resolve those.~
Seems I spoke too soon, it seems like file://...
dependencies are resolved. Let me dig into this some more.
Hm, I'm not able to get a repro. I've committed the files I'm using to a subtree here. you can try with
cp -r testprojects/src/helm/ /tmp/helm19966/
pushd /tmp/helm19966/
pants lint --only=helm actual-project::
Let me know if you spot what I haven't copied correctly
I think the template is too simple here. Try:
metadata:
name: .Values.value_c
labels:
a: {{ .Values.value_a | required "test a" }}
b: {{ .Values.value_b | required "test b" }}
c: {{ .Values.value_c | required "test c" }}
Only the chart's values.yaml seems to be plumbed through. I get
[WARNING] templates/cronJob.yaml: object name does not conform to Kubernetes naming requirements: ".Values.value_c": metadata.name: Invalid value: ".Values.value_c": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
1 chart(s) linted, 0 chart(s) failed
engine.go:168: [INFO] Missing required value: test a
engine.go:168: [INFO] Missing required value: test c
(Thanks for looking into this!)
Thanks for that sample. I'm not sure what to do, though, as I'm not really a Helm power user.
The error pants lint
gives is the same as the error helm lint
gives. I tried adding the --with-subcharts
flag, but that just seems to have also linted the subchart too, rather than the combination. Do you know of a Helm command we could run that would consider these charts together?
The only way I know of combining the charts is to use the helm template
command. Pants does that as part of the pants experimental-deploy
command. So pants experimental-deploy //:actual-project-actual-task-prod
results in "cronjob.batch/hello" with the expected metadata.labels .
Again, I'm not an advanced Helm user. If you know of the Helm commands to run, I can add those to Pants!
So I think a couple points here:
actual-task
isn't a subchart, it's an instance (we prefer to keep information about instances in the same directory, but that directory and its values.yaml could be anywhere).helm lint
would also need -f common/values.yaml -f actual-project/values.yaml -f actual-project/actual-task/values.yaml
to use the base values of the charts as well as the instance values.Not sure beyond that. I'd have to dig more. I realized we also adopted:
exports:
defaults:
value_a: "a"
in common/values.yaml and
dependencies:
- name: common
< ... >
import-values:
- defaults
in actual-project/Chart.lock.
Oh neat, I didn't realise that you could pass vars files to helm lint
. I think we could implement that.
I tried fiddling with exports
and import-values
, but couldn't get it to fix the issue.
I don't think this is a bug. The Helm backend is not capable of resolving chart dependencies (i.e. subcharts) using file://...
references. This is somewhat intentional as Pants works internally with a virtual filesystem and therefore references like file://.../common
are most of the time invalid. It's true that Helm itself does support that but adding that to Pants is non-trivial as Pants is designed to be able to run distributed builds using remote servers (hence the reason it uses a virtual filesystem).
To use chart dependencies you should instead use one of the two approaches documented here: https://www.pantsbuild.org/2.21/docs/helm#managing-chart-dependencies
That way Pants will properly identify the subcharts and their sources, build a compound chart that includes the root one plus all of its dependencies and use that compound chart when running tasks like lint
or publish
.
Besides, a values.yml
file at the root of a chart (sibling to Chart.yml
) is considered to be the default values of a chart. It will be included by default when running a deployment and there is no need to reference them when using helm_deployment
targets.
In the other hand, the location of actual-project/actual-task/values.yaml
is particularly odd. The helm_chart
target will ignore that file since, even though it's under the same file structure of the chart, it doesn't correspond to any of the standard chart file locations documented here: https://helm.sh/docs/topics/charts/#the-chart-file-structure
If you want to pass values to the deployment of a chart (via value files), we recommend to follow what is documented here: https://www.pantsbuild.org/2.21/docs/helm/deployments#value-files. This is, create a specific folder in your repo where to put all the value files that a given deployment may require (outside the structure of any Chart) and then add a BUILD
file to that folder where you put your helm_deployment
target configured as suggested in our docs.
That said, it is quite likely that current Helm implementation is missing some recent features supported by Helm, which could involve the possibility of importing custom value files from other charts or similar.
Happy to discuss those as some sort of feature request though.
We have the following files in a directory:
A BUILD file at the root of this contains:
(It would also be nice if these rules could live in each project directory, but that's a separate issue, i.e. supporting a reference to
../common/values.yaml
the same way Chart.yaml supportsfile://../common/
.)Anyway, when running linting in this setup, such as
pants lint --only=helm actual_project::
, this generates errors such as this:Or this:
If, however, I run the following in
actual-project
, I get the correctly-filled-in result:Pants version: 2.17 OS: Linux