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 support for CEL string 'replace' functions #472

Closed
bitsofinfo opened this issue Mar 5, 2020 · 10 comments · Fixed by #558
Closed

Add support for CEL string 'replace' functions #472

bitsofinfo opened this issue Mar 5, 2020 · 10 comments · Fixed by #558
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@bitsofinfo
Copy link
Contributor

Would be great to have a replace function for strings.

I don't think a custom one would be necessary to develop in triggers itself, but just wanted to note that eventually this should be available in cel-go per google/cel-go#316 requiring a dependency update once released

@dibyom
Copy link
Member

dibyom commented Mar 5, 2020

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 5, 2020
@bigkevmcd
Copy link
Member

This is dependent on google/cel-go#316

@bigkevmcd
Copy link
Member

bigkevmcd commented Mar 9, 2020

I'll wait for a release of cel-go with the new strings code, but this is a preview

master...bigkevmcd:bump-cel-version

This also extracts the Triggers extensions into a library similar to the strings one in cel-go, in doing so, it follows the style there.

This means that split(string, '/') would become string.split('/') and decodeb64(string) becomes string.decodeb64().

@TristonianJones
Copy link
Contributor

@bigkevmcd Just a quick FYI, the cel-go v0.4.1 release is stable and contains the extensions you're looking for.

With respect to encoders, I think a base64 encoding/decode function is also another reasonable CEL extension. One way to go for future custom functions is to file a feature request against cel-go for an extension. I can't promise it will be implemented right away, but I can promise to iterate on the names and type signatures of the functions in order to reduce churn in your project.

Cheers!

@bitsofinfo
Copy link
Contributor Author

@TristonianJones awesome thanks, can't wait for this!

@dibyom dibyom added this to the Triggers v0.5.0 milestone Apr 21, 2020
@bigkevmcd
Copy link
Member

@TristonianJones We're going to go through a "breaking changes" step, to bring in the strings library.

I'm going to switch to align with your "str-ing".split('-') approach, instead of our split("str-ing", "-") version, and the other hook-processing functions will go the same way.

We have an existing decodeb64 function (which is about to become string.decodeb64()).

In order to avoid further breakage, can we agree what this function might look like?

I'm happy to rename it if this isn't what the cel-go project would like, but obviously I'd prefer not to rename more than once since it's a breaking change.

@bigkevmcd
Copy link
Member

@TristonianJones To be clear, I'd happily do the work to move the implementation here into cel-go.

@TristonianJones
Copy link
Contributor

Hi @bigkevmcd!

Since base64 encode / decode can be applied to bytes or string types, I think it's actually reasonable to use base64.encode and base64.decode as the function names since this will also be the same convention I'd follow for json as well: json.encode, json.decode.

In thinking about this request I realized that there is a bug which is addressed in google/cel-go#341, and which I will release into cel-go v0.4.3 later today. The test cases in the PR approximate what you'd want out of this feature, but I'd be looking for it as a cel.Library in the file cel-go/ext/encoders.go

Thanks for being willing to put in all the hard work! I'm happy to review when you're ready.

Cheers,

-Tristan

@bitsofinfo
Copy link
Contributor Author

awesome can't wait!!

@TristonianJones
Copy link
Contributor

Just as a quick update, I'm going to take a bit more time to make the fix in google/cel-go#341 more bulletproof. It's got a rough edge on it at the moment.

savitaashture pushed a commit to savitaashture/triggers that referenced this issue Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants