crossplane-contrib / function-patch-and-transform

A patch & transform composition function
https://crossplane.io
Apache License 2.0
26 stars 25 forks source link

Add Sprig templating capability to transforms and CombineFromComposite #63

Closed phclark closed 8 months ago

phclark commented 11 months ago

This add the ability to use sprig templates as a transform, or with CombineFromComposite instead of simple fmt. The benefit of this is similar to function-go-template, except that we can benefit from the power of templates within the patch-and-transform context.

negz commented 10 months ago

@phclark I'm a little wary of mixing Go templating into the P&T function. Would it not be reasonable to expect folks to use https://github.com/crossplane-contrib/function-go-templating if they wanted this kind of expressiveness?

phclark commented 9 months ago

@negz Sorry, I just noticed this reply. The use case for having Go templating as a Transform type vs defining the entire manifest as a Go template is that you often only need advanced templating capabilities (compared to Format) when defining a string field in a manifest. For example, converting an array parameter into a Json list of strings to be injected as IAM resources in a policy document.
Additionally, I feel like it makes more sense to include features in function-patch-and-transform for consistency / interoperability sake, as opposed to forcing users to abandon Patches and PatchSets entirely. This limits the usefulness of features like PatchSets and makes it impossible to patch the Composition status with data from MRs.

withnale commented 9 months ago

Whilst this does introduce more complexity, right now our teams are often spending hours trying to implement simple tasks using the existing small sets of transforms and trying to create massively complex regexps. The risks associated with these implementations are much greater, since the implementations are very brittle. A definite :+1: on this from here.

go-templating is another option but it's a significant shift in how we write compositions when all we need is a very simple patch step.

withnale commented 9 months ago

Coming back around... I regard the below as a "simple use case" that is trivial using this PR but not without it.

        - type: CombineFromComposite
          toFieldPath: "spec.forProvider.projectId"
          combine:
            strategy: template
            variables:
              - fromFieldPath: metadata.name
                variableName: name
              - fromFieldPath: metadata.uid
                variableName: uid
            template:
              template: '{{ .name }}-{{ .uid | sha1sum | trunc 8 }}'

How would I approach this using the main branch version of function-patch-and-transform?

This "need" is in a composition file that is 1200 lines long already, so just moving it into function-go-templating isn't a quick option and having everything being a multiline yaml block removes the structure from the file potentially opening us up for a lot more uncaught errors.

phisco commented 9 months ago

@phclark @withnale, I think the point is that with this many other features would become redundant, e.g. most other string transforms could be replaced with just a string transform of type Template, and the rest could probably be replaced with the same + some ToType transform. I can see the value of a function that combines function-patch-and-transform and function-go-templating. The latter is better suited for creating new resources from scratch, with support for loops, ifs and other more powerful constructs, while the former is more effective at patching between resources and handling existing resources. But I think it would be better to think it from scratch rather than trying to fit it into this function.

negz commented 9 months ago

How would I approach this using the main branch version of function-patch-and-transform?

You wouldn't. The goal of this function is not full expressiveness.

The intent behind P&T is to cover the simple "I need to copy data X from resource Y to resource Z" case. It was forced to grow more expressive (and thus hard to reason about) than that because there was no alternative. There are now alternatives in the form of other functions, including one specifically dedicated to Go templating.

We need to cap the complexity of this function somewhere. Introducing a nested templating language with its own set of functions (in addition to the "first-class" transforms we already support) seems like a reasonable place to draw the line.

negz commented 8 months ago

My position here hasn't changed. I appreciate the effort, but I'm going to close this PR.

I think https://github.com/crossplane-contrib/function-go-templating/ is a great alternative to explore if you need this level of expressiveness.