Add a WithValueTranslator option to Reconciller.

[porridge]

This lets the user inject code which produces helm values based on the fetched object itself (unlike Mapper which can only see Values).
This way the custom code can convert the object to a typed one and work with proper structs rather than a tree of maps to interface{}.

One wrinkle I'm not proud of is the conversion between chartutil.Values and
internal/values.Values but I don't think it's ugly enough to put it on the
critical path.

[SimonBaeumer]

Could you mention the difference to the Mapper? It could get confusing as they have similar use-cases.

[misberner]

+1, reading this PR I don't really understand why a custom translator is required in addition to the mapper. Things that come to mind:

• access to metadata (name, ...) and status fields
• ability to use Go types by converting the *unstructured.Unstructured into the Go type for a CR.

However, having translator + mapper seems confusing. Perspectively, I think it would make more sense to introduce translator as the first-class citizen, and drop the valueMapper field. WithValueMapper could then return a function that sets the valueTranslator accordingly (relying on FromUnstructured).

Or am I completely off, and the difference is actually whether it gets executed pre/post the application of overrides?

[porridge]

@SimonBaeumer added a couple of sentences.

@misberner the rationale is in the PR description:

This lets the user inject code which produces helm values based on the fetched object itself (unlike Mapper which can only see Values).
This way the custom code can convert the object to a typed one and work with proper structs rather than a tree of maps to interface{}.

Basically it is so that we can have write type-safe code at least to read the CRs.

I'd agree about dropping the Mapper API eventually, but:

• it's out of scope of this PR, I'm trying to diverge from upstream as little as possible,
• in general it would need more discussion with upstream, for example there are also these ApplyOverrides and CoalesceValues steps in getValues which I don't fully understand, so there might be use cases where Mapper is a better tool

[msugakov]

Is FromUnstructured() still used somewhere? If not, maybe it should be deleted.

Alternatively, I don't think you need ChartValuesFromUnstructured() function at all. Its only difference from FromUnstructured() is that it doesn't wrap map[string]interface{} into Values. And that's only for DefaultTranslator.

You can do something like

var DefaultTranslator = values.TranslatorFunc(func(u *unstructured.Unstructured) (chartutil.Values, error) {
	blah, err = FromUnstructured(u)
	if err != nil {
		return chartutil.Values{}, err
	}
    return blah.Map(), nil
}

and then ChartValuesFromUnstructured() isn't used anywhere.

I think that's simpler than finding the proper name for ChartValuesFromUnstructured() and adjusting tests for it (which I otherwise would ask).

---

Maybe authors have an idea to extend Values struct with some other filed. Right now, I think this struct is redundant and the code will gain more clarity if it goes away eventually.

I see that with ChartValuesFromUnstructured() you avoid creating a garbage object but I don't think it is worthwhile goal. Even though the object is returned as pointer, I believe it should be placed on stack in Reconciler.getValues() (due to escape analysis), and so freeing it is cheap.

[porridge]

I guess I was trying to avoid the back-and-forth of New->Map->New, not for performance reasons but for simplicity. But getting rid of ChartValuesFromUnstructured seems worth it. Changed.

[misberner]

Minimizing divergence makes sense to me to make our lives easier for future rebases. When we pitch this to upstream, however, I think it would be nice to move applyOverrides + mapper to a "legacy translator", as imho neither makes sense if you have something as flexible as a translator (CoalesceValues seems to be required as it pulls in values defined in the chart itself)

[msugakov]

+1 to Malte's point about retiring Mapper&Co. in favor of Translator.
What could be a good way for us to not forget mention it when we eventually merge this upstream. Should we add a // TODO: comment on Mapper for this?

[porridge]

TODO added, merging.