feat: implement export-values

[ilya-lesikov]

What this PR does / why we need it:

This PR implements export-values directive which is similar to the import-values directive, but instead of importing the values of the subchart into the current chart it exports the values of the current chart into subchart.

Initially the change was discussed here #3242. There were several attempts to implement this: #7477, #3243.

Usage:

In the full form of export-values you specify which value tree to export (with parent:) and where it should be exported in the subchart (with child:).

In the short form of export-values you only specify a relative path inside of the exports: key of the parent chart values: specified values will be exported to the root of the subchart values (same as with the import-values). This allows to export/import values defined in exports: both to the parent chart or to the child chart depending on the directive used (import-values or export-values).

Unlike import-values, export-values can export not only the trees of values (maps), but other types of values too (strings, ints, etc).

Example:

# values.yaml
parent-map:
  key1: parent-value1
  key2: parent-value2
  
exports:
  parent-map:
    key3: parent-value3
  
subchart1:
  subchart-map:
    key1: subchart-value1

# Chart.yaml:
dependencies:
  - name: subchart1
    version: 1.0.0
    export-values:
    - parent: "parent-map"
      child: "subchart-map"
    - "parent-map.key3"

# charts/subchart1/dump-values-in-subchart.yaml
dump-values-in-subchart: {{ .Values | toYaml | nindent 2 }}

Result of helm template:

# charts/subchart1/dump-values-in-subchart.yaml
dump-values-in-subchart:
  subchart-map:
    key1: parent-value1
    key2: parent-value2
    key3: parent-value3

If applicable:

• this PR contains documentation
• this PR contains unit tests
• this PR has been tested for backwards compatibility

[bacongobbler]

Hi @ilya-lesikov. I'm going to re-express the same concerns I had with the last PR (#7477) that there was not enough supporting documentation provided to better understand

• how it works
• why it's useful
• how it addresses backwards compatibility
• why existing features can't handle the same use case
• how are we supposed to teach this to new users
• etc.

With that said, I believe this proposal should go through the HIP process BEFORE any further code review. The process helps answer some basic questions like "how does this address backwards compatibility?" and "how is this feature expected to be useful?" which I haven't seen answered in either #3242, #7477, or this PR. And I think that would help alleviate a lot of the concerns I expressed earlier.

Let me know if you have any questions about the process. Thanks!

[ilya-lesikov]

I'll do the docs, although I thought I might get some hints about the overall design before writing them. The usage could've been found in tests and the first post.
I'll do the HIP/docs soon then.

[bacongobbler]

The usage could've been found in tests and the first post.

While I understand the sentiment, we (the maintainers) have a very limited amount of time we can spend each week to review code. Documenting your feature means we can spend more time reviewing your code. If I have to read your unit tests and attempt to make assumptions about how your feature is supposed to work, I'm going to spend my time looking at the other 150+ PRs in the queue that are ready for testing until I can find the time to schedule a longer review - and that time is getting harder to find.

although I thought I might get some hints about the overall design before writing them

Honestly, that's up to you. This is open source - you the author have the authority to choose how it works. Though that also comes with the burden to document and teach others how it works. Especially if you want to get it merged :)

[MajorBreakfast]

Use case: forwarding of imagePullSecret name value (string) to various subcharts.
Many public helm charts expose setting the image pull secret differently in their values.yaml, e.g. imagePullSecret: string, imagePullSecrets: string[] or imagePullSecrets: { name: string }[] (typescript type syntax). If this feature was implemented I think it would let me write a parent chart that accepts the pull secret value top level and forwards it to the child charts

[loewenstein]

May I politely ask about the state of this PR? Has a HIP been created already? Any idea as to when this might be available in a released version of Helm (like are we talking weeks, months, years or even "we don't know if this gets merged at all")?

If I've followed the breadcrumbs correctly, this and similar though more generic proposals have been asked for and discusses for years.

This approach seems to follow a compatibility oriented approach while fixing a from my point of view significant deficiency with regards to compositionality of Helm charts / abstraction around parent and child charts.

It would be really great to get export-mapping and if there is anything that would help to drive this forward I'd be interested to hear.

[ilya-lesikov]

Hello @loewenstein
Frankly, looking at the HIP repo I was not sure how much additional time will all of this take and will it have any chance of landing in upstream. Also, it was not immediately obvious how much value this feature adds on top of passing these same values via --set cli flags (if values come from an external system) or using yaml aliases in values.yaml (if values are already present in values.yaml). Surely, this might look better than passing a handful of --set flags on each invocation, and yaml aliases have their fair share of limitations (e.g. inability to reference anchors from another values file). Is this enough to get it merged is hard to tell. Is this what you need? Maybe you can come up with some additional uses of this feature?

In werf this feature was really useful since we inject in Helm some values at runtime and the user does not have direct control of these values. Because of this, we needed something truly dynamic to be able to actually map these values from one place to another. For us it worked well.

[loewenstein]

FYI we've created a HIP for export-values. helm/community#242

[loewenstein]

I think we did figure out that this PR would not yet readily provide an implementation though.

[jdolitsky]

Lets resolve the issue with old PR but otherwise lgtm

[loewenstein]

Lets resolve the issue with old PR but otherwise lgtm

I understood from @c0d1ngm0nk3y and others that this PR would not be a full implementation of the HIP we provided. I’d rather put this on hold (at least until they had a chance to comment). We might just provide a separate PR with a proper implementation - potentially based of of this proposal.

cc @phil9909

[jdolitsky]

@loewenstein - is this a partial implementation? Would you be able to push your changes for HIP 16 directly onto this PR?

cc @ilya-lesikov

[loewenstein]

We'll discuss how to continue tomorrow 9 CET. We are not related to @ilya-lesikov at all, just need the feature, found the existing approaches and decided to pick up the request for a HIP as prerequisite for am implementation.

[jdolitsky]

revoking approve until the HIP is fully implemented

see above

[ilya-lesikov]

revoking approve until the HIP is fully implemented

@jdolitsky Now I believe the HIP is implemented in this PR. I had to rewrite it from scratch basically, but now it should be fine.

The only thing that was implemented differently is this:

It should be able to use both, export-values and import-values independently and in combination. It should also be possible to export a value which was imported from a third chart.

In current implementation you can use both export-values and import-values, but export-values are evaluated before import-values.

The reason for this is that import-values implementation is not very good and import-values breaks whatever runs after it (export-values in our case). I believe it's due to messing up Values tree of charts, maybe something wrong with Coalesce*() functions, it looks like they do not perform deep copy when merging src and dest, so they might link some reference type values (maps, lists) from src to dest, but I might be wrong. Anyways, fixing import-values is out of scope of this PR so I tried not to touch it.

Other than that should work as expected. Chaining of export-values through multiple nested charts supported too.

[joriatyBen]

What is the state of play here?

[loewenstein]

The other PR (#10804) is waiting to be reviewed I think.

[joejulian]

@ilya-lesikov Can you rebase this?

[dtomasi]

Is there any progress? This would be sooo useful and I was looking forward to make use of this feature since 2021 ...

[loewenstein]

#10804 is being moved from milestone to milestone,..., I am curious if it will eventually get merged.

All in all, quite a frustrating experience.