Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sprig templating capability to transforms and CombineFromComposite #63

Closed
wants to merge 5 commits into from

Conversation

phclark
Copy link

@phclark phclark commented Dec 8, 2023

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.

Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
@phclark phclark mentioned this pull request Dec 9, 2023
@negz
Copy link
Member

negz commented Jan 2, 2024

@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
Copy link
Author

phclark commented Jan 21, 2024

@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
Copy link

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 👍 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.

Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry folks, right now I am leaning toward not approving this.

I understand that this enables things you can't do today with P&T. However, the point of P&T was to be for simpler use cases. It intentionally does not do everything someone could need it to do.

My suggestion would be that if you want or need sprig templating, you would be better served by migrating to https://github.com/crossplane-contrib/function-go-templating/.

@@ -257,6 +258,11 @@ type StringTransform struct {
// Extract a match from the input using a regular expression.
// +optional
Regexp *StringTransformRegexp `json:"regexp,omitempty"`

// Render a template using the input as data.
// See https://pkg.go.dev/github.com/pschlump/sprig for details.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Masterminds/sprig?

@withnale
Copy link

withnale commented Jan 23, 2024

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
Copy link
Collaborator

phisco commented Jan 23, 2024

@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
Copy link
Member

negz commented Jan 23, 2024

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
Copy link
Member

negz commented Feb 28, 2024

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.

@negz negz closed this Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants