Add a WithValueTranslator option to Reconciller. #114

porridge · 2021-11-09T12:24:37Z

A Translator is a way to produces helm values based on the fetched custom
resource itself (unlike Mapper which can only see Values).

This way the code which converts the custom resource to Helm values can first
convert an Unstructured into a regular struct, and then rely on Go type
safety rather than work with a tree of maps from string to interface{}.

Thanks to having access to a Context, the code can also safely access the
network, for example in order to retrieve other resources from the k8s cluster,
when they are referenced by the custom resource.

Fixes: #113

porridge

/assign @varshaprasad96
cc @fabianvf

porridge · 2021-11-09T12:26:25Z

pkg/values/values.go

 )

+// TODO: Consider deprecating Mapper and overrides in favour of Translator.


Please let me know what do you think about this. Since Translator is a strictly more powerful interface, perhaps we should get rid of Mapper?

+1 from me for getting rid of Mapper (pending a conversation about the timing of applying overrides)

coveralls · 2021-11-09T12:35:07Z

Pull Request Test Coverage Report for Build 1530128681

33 of 33 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 62.149%

Totals
Change from base Build 1476478121:	0.2%
Covered Lines:	1683
Relevant Lines:	2708

💛 - Coveralls

varshaprasad96 · 2021-11-11T19:25:18Z

pkg/reconciler/reconciler.go

 	if err := crVals.ApplyOverrides(r.overrideValues); err != nil {
 		return chartutil.Values{}, err
 	}
-	vals := r.valueMapper.Map(crVals.Map())
+	vals = r.valueMapper.Map(crVals.Map())


agreed, having valueMapper is not of much use here. Instead of using both valueTranslator and valueMapper on the same values can we prioritize the translatorfunc if its not nil by the user?

@varshaprasad96 can you please clarify what you mean by "prioritize the translatorfunc if its not nil by the user"?
Do you mean passing both WithValueMapper and WithValueTranslator to New() should be rejected?

If Translator is passed, we can ignore the mapper (in case both are non-nil) instead of applying both. This would make it easier to deprecate and remove mapper completely later. This is just so that we don't have a breaking change immediately. Thoughts?

I'm afraid it would be detrimental to developer experience if the WithValueMapper option was silently ignored when the WithValueTranslator option would be passed. IMHO it would violate the principle of least astonishment.

Completely agree. I had logging a warning and docs along with it in mind so that we don't have to make a breaking change of removing WithValueMapper immediately. It would be nice to discuss the PR in tomorrow's community meeting. This is a really helpful addition, would definetely want to have this PR merged soon :)

Unfortunately I'll be in a place with poor network coverage at that time so I will not be able to join the meeting. But to summarise my thoughts:

I don't think we need to remove value mappers in this PR, or even the same release, but it would make sense to add deprecation warnings to prepare for removal if we want to do it eventually,

a warning and docs alone are in my experience insufficient countermeasure to a surprise in the API unless there is no other option, and I think we have a good alternative:

IF we want to make mappers and translators exclusive, then I'd strongly recommend returning and error from New in case both WithValueMapper and WithValueTranslator are passed. This is not going to harm existing users, unless they try using a translator, in which case such error is a useful hint.

Hi, any updates on this after the meeting? How do we proceed on this?

Also, would it be possible to add me to the operator-framework org? Just so that I can configure GitHub to send PR notifications to my work email..

joelanford · 2021-11-18T18:11:35Z

There's a similar PR to this one (#93).

I mostly summarized my thoughts on this subject here.

My main area of concern is the timing of applying value overrides:

One notable semantic change would be that all of the translation and mapping would happen before overrides are applied. This would make setting the overrides less ergonomic since mapped values may otherwise be an internal implementation detail of the operator.

If a translator is in use, someone configuring overrides will need to be aware of how the CR is mapped to chart values because the overrides apply to the translator's output. This means potentially exposing internal chart implementation details to users applying overrides at runtime.

porridge · 2021-11-24T10:32:09Z

Thanks for pointing me at the overrides documentation, @joelanford - I guess I didn't really understand its purpose before.
Here's my stab at applying overrides first. PTAL

Rebased to take advantage of #112 but otherwise the first commit should be unchanged.

pkg/reconciler/internal/values/values_test.go

pkg/reconciler/reconciler.go

joelanford · 2021-11-30T17:14:21Z

pkg/values/values.go

+// TODO: Consider deprecating Mapper in favour of Translator.
+


Let's go ahead and deprecate it IMO.

joelanford · 2021-11-30T17:14:53Z

pkg/reconciler/reconciler.go

-	crVals, err := internalvalues.FromUnstructured(obj)
-	if err != nil {
+func (r *Reconciler) getValues(ctx context.Context, obj *unstructured.Unstructured) (chartutil.Values, error) {
+	if err := internalvalues.ApplyOverrides(r.overrideValues, obj); err != nil {


👍 This looks great!

A Translator is a way to produces helm values based on the fetched custom resource itself (unlike `Mapper` which can only see `Values`). This way the code which converts the custom resource to Helm values can first convert an `Unstructured` into a regular struct, and then rely on Go type safety rather than work with a tree of maps from `string` to `interface{}`. Thanks to having access to a `Context`, the code can also safely access the network, for example in order to retrieve other resources from the k8s cluster, when they are referenced by the custom resource.

porridge · 2021-12-02T10:31:39Z

Applied the suggestions and squashed the commits.
I also tried to improve the docstrings a little and make sure they look sane in godoc.
Also added a nolint pragma for the use of Mapper to make our lint pass.
PTAL @joelanford

joelanford

/approve

github-actions bot added area/sdk area/testing labels Nov 9, 2021

porridge commented Nov 9, 2021

View reviewed changes

openshift-ci bot assigned varshaprasad96 Nov 9, 2021

varshaprasad96 reviewed Nov 11, 2021

View reviewed changes

porridge force-pushed the porridge/upstream-translator branch from eaf6277 to bc7e888 Compare November 24, 2021 10:30

porridge requested a review from joelanford November 24, 2021 10:33

jmrodri mentioned this pull request Nov 30, 2021

Pass watched Custom Resource to values mapper #93

Closed

joelanford reviewed Nov 30, 2021

View reviewed changes

porridge force-pushed the porridge/upstream-translator branch from bc7e888 to 09a6eb4 Compare December 2, 2021 10:29

porridge requested a review from joelanford December 2, 2021 10:31

joelanford approved these changes Dec 2, 2021

View reviewed changes

porridge requested a review from varshaprasad96 December 3, 2021 07:12

varshaprasad96 merged commit a423fef into operator-framework:main Dec 14, 2021

porridge deleted the porridge/upstream-translator branch December 15, 2021 06:06

This was referenced Dec 15, 2021

Replace WithValueTranslator implementation with upstream one stackrox/helm-operator#23

Merged

REQUEST: New membership for porridge operator-framework/community#57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a WithValueTranslator option to Reconciller. #114

Add a WithValueTranslator option to Reconciller. #114

porridge commented Nov 9, 2021

porridge left a comment

porridge Nov 9, 2021

joelanford Nov 18, 2021 •

edited

Loading

coveralls commented Nov 9, 2021 •

edited

Loading

varshaprasad96 Nov 11, 2021

porridge Nov 12, 2021

varshaprasad96 Nov 12, 2021 •

edited

Loading

porridge Nov 15, 2021

varshaprasad96 Nov 15, 2021 •

edited

Loading

porridge Nov 16, 2021

porridge Nov 18, 2021

joelanford commented Nov 18, 2021

porridge commented Nov 24, 2021

joelanford Nov 30, 2021

porridge Dec 2, 2021

joelanford Nov 30, 2021

porridge commented Dec 2, 2021

joelanford left a comment

		)

		// TODO: Consider deprecating Mapper and overrides in favour of Translator.

		// TODO: Consider deprecating Mapper in favour of Translator.

Add a WithValueTranslator option to Reconciller. #114

Add a WithValueTranslator option to Reconciller. #114

Conversation

porridge commented Nov 9, 2021

porridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 9, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1530128681

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford commented Nov 18, 2021

porridge commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porridge commented Dec 2, 2021

joelanford left a comment

Choose a reason for hiding this comment

joelanford Nov 18, 2021 •

edited

Loading

coveralls commented Nov 9, 2021 •

edited

Loading

varshaprasad96 Nov 12, 2021 •

edited

Loading

varshaprasad96 Nov 15, 2021 •

edited

Loading