Repository navigation
Conversation
Uses the $( ) expression engine in kubevela/pkg/cel, with one root, context: the same object CUE filters see, so both read the same paths. Filters: a filter that is a single $( ) is CEL; any other filter is CUE, as before. No CUE filter has that form, so this needs no switch. CEL's && and || skip what cannot change the result, has() and `in` guard fields only some objects carry, and a non-bool result is an error. A CEL filter on a 5 KB Deployment evaluates in about 56 us against about 1.05 ms for the CUE equivalent. Action properties: $( ) expressions over context are rendered per event; one that is a whole property keeps its type, and $$( is a literal $(. The job's ID still comes from the properties as written, so jobs from one trigger keep sharing it. Property expressions are on by default and can be turned off, since a literal $( such as Kubernetes' $(VAR) or a shell's $(date) in a task's command must now be written $$(; the compile error says so. - The worker's --enable-cel-expressions (default true) turns them off. - The manager's --enable-cel-expressions (default true) does so for every TriggerService, and passes its choice to each worker explicitly. - A TriggerService annotated trigger.oam.dev/cel-expressions: "false" opts out; with the manager's --require-cel-expression-opt-in, only those annotated "true" render expressions. Expressions are planned when kube-trigger starts: one that does not compile stops it, naming the trigger and property, instead of failing on every event. The image-update example's task ran `echo ... $(date)`, which would now stop kube-trigger at start; it is written $$(date). A test compiles every shipped example with expressions on, and the README documents the flag in its configuration reference. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGACRqRjRF3TqH6B9vVijh Signed-off-by: Brian Kane <briankane1@gmail.com>
briankane
requested review from
FogDong,
Somefive,
anoop2811,
charlie0129 and
jguionnet
as code owners
October 15, 2026 20:17
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 45.39% 48.38% +2.99%
==========================================
Files 31 32 +1
Lines 1454 1546 +92
==========================================
+ Hits 660 748 +88
- Misses 794 798 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
All reported issues were addressed across 18 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
2 of 5 tasks
Review follow-ups. - A filter that opens with $( but does not parse, such as one never closed, was taken for CUE and failed only when an event arrived. It is now an error at start, like any other CEL filter that does not compile. - The image-update example writes the shell's command substitution with backticks rather than $$(date), so it runs the same with expressions on or off. - The event handler test that a CEL filter keeps an event uses a fake client rather than recovering from any panic, so a panic in evaluation can no longer pass as a kept event. - The README's environment variable for --enable-cel-expressions is ENABLE_CEL_EXPRESSIONS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGACRqRjRF3TqH6B9vVijh Signed-off-by: Brian Kane <briankane1@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Adds CEL to kube-trigger, using the
$( )expression engine inkubevela/pkg/cel(kubevela/pkg#143), with one root,context: the same object CUE filters see, so CEL and CUE read the same paths.Filters
A filter that is a single
$( )is CEL; any other filter is CUE, as before. No working CUE filter has that form, so this needs no switch.CEL's
&&and||skip what cannot change the result, sohas()andincan guard fields only some objects carry, which CUE's non-short-circuiting&&cannot. A result that is not a bool is an error.Expressions in action properties
$( )in action properties is rendered per event. An expression that is a whole property keeps its type; one inside text is joined in;$$(is a literal$(.The job's ID still comes from the properties as written, so jobs from one trigger keep sharing it.
Switches
On by default, and can be turned off:
--enable-cel-expressions=falsetrigger.oam.dev/cel-expressions: "false"--enable-cel-expressions=false--require-cel-expression-opt-in; only TriggerServices annotated"true"The manager passes its choice to each worker explicitly through the default worker template. The annotation is read when a TriggerService's worker is created.
Breaking change: a literal
$(in action properties, such as a shell's$(date)or Kubernetes'$(VAR), is now written$$(. Such a property stops kube-trigger at start with an error naming the trigger and property and saying so. Thetriggerservice-image-updateexample is updated.Validation
Every expression is planned when kube-trigger starts; one that does not compile stops it, naming the trigger and property, instead of failing on every event. Typing is dynamic (any kind can be watched): a typo or a wrong type shows at event time, where the event is dropped and the error logged.
I have:
make reviewableto ensure this PR is ready for review. (Ranmake lint,make checklicenseandgo mod tidy; notgenerate, as the localcontroller-gendiffers from the pinned version.)backport release-x.ylabels to auto-backport this PR if necessary. Not needed: kube-trigger has no release branches.How has this code been tested
Unit tests at each layer, written before the code:
pkg/expression: telling CEL from CUE filters; filter results,has()andinguards, short-circuiting, a missing field and a non-bool result as errors; property rendering (typed whole values, interpolation,$$(); compile errors naming the property with the$$(hint.pkg/eventhandler: CEL filters keep and drop events (including a test that fails if a CEL filter is read as CUE); bad expressions refuse the handler; properties are not read while expressions are off.pkg/config: startup validation reports every bad expression with its trigger; every file inexamples/compiles with expressions on.controllers/triggerservice: the switch and annotation decision table, and the default worker template rendering--enable-cel-expressions=trueand=false.go test ./pkg/... ./controllers/...passes; golangci-lint and staticcheck are clean. A CEL filter on a 5 KB Deployment evaluates in about 0.056 ms against about 1.05 ms for the CUE equivalent. Not yet run against a live cluster.Special notes for your reviewer
context.changedbecomes readable from CEL with no change here; whichever merges second needs a small rebase in the event handler.contextis evaluated dynamically. Typed checking against the watched kind's schema (as ValidatingAdmissionPolicy does) is a possible follow-up; it needs a TriggerService admission webhook, which kube-trigger does not have.Summary by cubic
Adds CEL expressions to kube-trigger using the expression engine from
kubevela/pkg/cel. A filter that is a single$( )is evaluated as CEL; any other filter is CUE as before. Both read the samecontextroot, so CEL and CUE read the same paths.$( )in action properties is rendered per event; an expression that is a whole property keeps its type.&&/||short-circuit, sohas()andincan guard fields only some objects carry; a non-bool filter result is an error.$(but never closes, which previously only failed per event.trigger.oam.dev/cel-expressionsannotation, or for all TriggerServices; the manager can also require per-TriggerService opt-in.Breaking change
$(in action properties, such as a shell's$(date)or Kubernetes'$(VAR), must now be written$$(; an unescaped one stops kube-trigger at startup. Thetriggerservice-image-updateexample now uses backticks for the shell substitution, so it runs the same with expressions on or off.Written for commit f6b173b. Summary will update on new commits.