feat(cel): add direct custom param variable access in expressions #2320
Conversation
🔍 PR Lint Feedback
|
pkg/matcher/cel.go
Outdated
| cel.Lib(celPac{vcx, ctx, event}), | ||
| conflictingVar := make(map[string]bool) | ||
| for k, v := range customParams { | ||
| // Don't overwrite standard params |
There was a problem hiding this comment.
Want to confirm if we want this behaviour. Modeled it on this
pkg/matcher/cel.go
Outdated
| for k := range customParams { | ||
| // Don't overwrite standard params | ||
| if _, ok := conflictingVar[k]; !ok { | ||
| customParamDecls = append(customParamDecls, decls.NewVariable(k, types.StringType)) |
There was a problem hiding this comment.
All variables are used as strings, do we want them to be DynType?
There was a problem hiding this comment.
IMO string is preferable since the values are strictly strings in the input, but I don't feel super strongly in that conviction
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature allowing custom parameters from the Repository CR to be accessed directly as CEL variables, which improves the usability of CEL expressions. The implementation looks solid, reusing existing logic for parameter resolution and safely adding the custom parameters to the CEL context. The test coverage for the new functionality is also comprehensive.
I have a couple of minor suggestions for code simplification and to improve clarity in one of the new tests.
theakshaypant
force-pushed
the
SRVKP-9118-fix-cel-custom-params
branch
from
3d13ed7 to
6e26664
Compare
pipelines-as-code
bot
added
documentation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a great feature allowing direct access to custom parameters as CEL variables. The implementation is solid, and the documentation updates are clear. I've identified one important bug related to how parameters are resolved when target-namespace is used, and I've suggested a fix. I also have a couple of suggestions to improve logging for conflicting parameters and to enhance documentation consistency. Overall, great work!
| // Resolve custom params once for all PipelineRuns (for use in CEL expressions) | ||
| customParams := resolveCustomParamsForCEL(ctx, repo, event, cs, vcx, eventEmitter, logger) | ||
| if len(customParams) > 0 { | ||
| logger.Debugf("resolved %d custom params from repo for CEL", len(customParams)) | ||
| } |
There was a problem hiding this comment.
There is a bug in how custom parameters are resolved for CEL expressions. They are resolved only once at the beginning of MatchPipelinerunByAnnotation, using the initial repository. This doesn't account for PipelineRuns that use the pipelinesascode.tekton.dev/target-namespace annotation to target a different repository, which may have its own set of custom parameters. This can lead to CEL expressions being evaluated with incorrect parameter values.
To fix this, parameter resolution should be performed inside the for...range pruns loop for each PipelineRun, after its effective repository has been determined. To avoid performance degradation from resolving parameters repeatedly for the same repository, the results should be cached.
Suggested Refactoring:
- Remove this block of code (lines 215-219).
- Before the
for...range prunsloop, initialize a cache, e.g.,customParamsCache := make(map[string]map[string]string). - Inside the loop, within the
if celExpr, ok := ...block, implement logic to get parameters from the cache or resolve and cache them, using the correct repository for the currentPipelineRun.
Example logic for inside the if celExpr, ok := ... block:
effectiveRepo := repo // Use the default repo passed to the function
if prMatch.Repo != nil {
effectiveRepo = prMatch.Repo // A different repo was found via target-namespace
}
var customParams map[string]string
if effectiveRepo != nil {
cacheKey := fmt.Sprintf("%s/%s", effectiveRepo.Namespace, effectiveRepo.Name)
var ok bool
if customParams, ok = customParamsCache[cacheKey]; !ok {
// Not in cache, resolve and add it
customParams = resolveCustomParamsForCEL(ctx, effectiveRepo, event, cs, vcx, eventEmitter, logger)
customParamsCache[cacheKey] = customParams
}
}
// Now evaluate with the correct customParams
out, err := celEvaluate(ctx, celExpr, event, vcx, customParams)
// ...
pkg/matcher/cel.go
Outdated
| } else { | ||
| conflictingVar[k] = true | ||
| } |
There was a problem hiding this comment.
When a custom parameter conflicts with a standard CEL variable (e.g., a user defines a parameter named event), it is silently ignored. This can be confusing for users and make debugging difficult.
It would be more user-friendly to log a warning when this happens. To do this, you would need to:
- Pass the
loggerfromMatchPipelinerunByAnnotationinto thiscelEvaluatefunction. - Add a logging statement inside this
elseblock, like:
logger.Warnf("Custom parameter %q conflicts with a standard CEL variable and will be ignored", k)
theakshaypant
force-pushed
the
SRVKP-9118-fix-cel-custom-params
branch
2 times, most recently
from
52e29ce to
354354e
Compare
theakshaypant
changed the title
aThorp96
left a comment
aThorp96
left a comment
There was a problem hiding this comment.
This is looking good so far. Thanks! I haven't gone through it entirely yet; I had a couple questions/suggestions which might change the implementation approach
pkg/matcher/annotation_matcher.go
Outdated
| if cached, found := customParamsCache[cacheKey]; found { | ||
| customParams = cached | ||
| } else { | ||
| customParams = resolveCustomParamsForCEL(ctx, effectiveRepo, event, cs, vcx, eventEmitter, logger) |
There was a problem hiding this comment.
Since the Repository CR is specific to an event, we can evaluate the customParams at the top of the loop so we shouldn't need caching logic. Good thought to have the caching though, had the Repository CRs been dependent on the specific pipelinerun
There was a problem hiding this comment.
No caching anymore having removed the effective repo block :)
theakshaypant
force-pushed
the
SRVKP-9118-fix-cel-custom-params
branch
from
354354e to
d328cc6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature that allows custom parameters from the Repository CR to be used directly in CEL expressions, enhancing the flexibility of pipeline matching. The implementation is well-structured, effectively reusing existing parameter resolution logic, and is accompanied by a comprehensive set of unit tests. I've identified a couple of areas for improvement: an inaccurate code comment and a newly introduced CEL variable that needs to be documented. Addressing these points will improve code maintainability and user experience.
| } | ||
|
|
||
| // resolveCustomParamsForCEL resolves custom parameters from the Repository CR for use in CEL expressions. | ||
| // It returns a map of parameter names to values, excluding reserved keywords. |
There was a problem hiding this comment.
This comment is slightly inaccurate. The function does not exclude reserved keywords; that logic is handled in celEvaluate to prevent overwriting standard variables. To improve clarity, I suggest updating the comment to more accurately describe what the function does.
| // It returns a map of parameter names to values, excluding reserved keywords. | |
| // It returns a map of parameter names to values. |
|
|
||
| data := map[string]any{ | ||
| "event": event.TriggerTarget.String(), | ||
| "event_type": event.EventType, |
There was a problem hiding this comment.
The addition of the event_type variable is a great enhancement for more specific event matching in CEL expressions. However, this new variable is not documented in docs/content/docs/guide/matchingevents.md. Please update the documentation to include event_type in the table of available CEL variables. It would be helpful to clarify that event refers to the trigger type (e.g., pull_request, push), while event_type provides the more specific provider event string (e.g., pull_request:labeled).
There was a problem hiding this comment.
+1 event_type should be documented as well
theakshaypant
force-pushed
the
SRVKP-9118-fix-cel-custom-params
branch
3 times, most recently
from
317e497 to
78e3f5a
Compare
aThorp96
left a comment
aThorp96
left a comment
There was a problem hiding this comment.
This looks a lot better, thanks! I have a few smaller requests
| For example, with this Repository CR configuration: | ||
|
|
||
| ```yaml | ||
| apiVersion: "pipelinesascode.tekton.dev/v1alpha1" |
There was a problem hiding this comment.
Nitpick, for consistency
| apiVersion: "pipelinesascode.tekton.dev/v1alpha1" | |
| apiVersion: pipelinesascode.tekton.dev/v1alpha1 |
| Custom parameters from secrets are also available: | ||
|
|
||
| ```yaml | ||
| apiVersion: "pipelinesascode.tekton.dev/v1alpha1" |
There was a problem hiding this comment.
| apiVersion: "pipelinesascode.tekton.dev/v1alpha1" | |
| apiVersion: pipelinesascode.tekton.dev/v1alpha1 |
| # ... pipeline spec | ||
| ``` | ||
|
|
||
| For more information on CEL expressions and event matching, see the [Advanced event matching using CEL]({{< relref "/docs/guide/matchingevents#advanced-event-matching-using-cel" >}}) documentation. |
There was a problem hiding this comment.
It would be good to document that referencing a param with a filter will cause a CEL error if the filter is false on the event. E.g.:
kind: Repository
metadata:
name: my-repo
spec:
url: "https://github.com/owner/repo"
params:
- name: meaning_of_life
value: "42"
filter: pac.event_type == "pull_request"apiVersion: tekton.dev/v1
kind: PipelineRun
metadata:
name: my-pipeline
annotations:
pipelinesascode.tekton.dev/on-cel-expression: |
meaning_of_life == "ohana"
spec:
# ... pipeline specIIUC on a push event, the above pipelinerun's CEL expression won't be false, it will be an error.
There was a problem hiding this comment.
Also we should document that custom parameters here do not override standard cel variables- I have been asked several times if a custom-param could override standard variables like trigger_target and even target_branch
There was a problem hiding this comment.
Added some more details in docs in 7afc027
|
|
||
| data := map[string]any{ | ||
| "event": event.TriggerTarget.String(), | ||
| "event_type": event.EventType, |
There was a problem hiding this comment.
+1 event_type should be documented as well
pkg/matcher/cel.go
Outdated
| } | ||
| env, err := cel.NewEnv( | ||
| cel.Lib(celPac{vcx, ctx, event}), | ||
| conflictingVar := make(map[string]bool) |
There was a problem hiding this comment.
I think this can be simplified a bit:
varDecls := []cel.EnvOption{
cel.Lib(celPac{vcx, ctx, event}),
cel.VariableDecls(
decls.NewVariable("event", types.StringType),
decls.NewVariable("event_type", types.StringType),
decls.NewVariable("headers", types.NewMapType(types.StringType, types.DynType)),
decls.NewVariable("body", types.NewMapType(types.StringType, types.DynType)),
decls.NewVariable("event_title", types.StringType),
decls.NewVariable("target_branch", types.StringType),
decls.NewVariable("source_branch", types.StringType),
decls.NewVariable("target_url", types.StringType),
decls.NewVariable("source_url", types.StringType),
decls.NewVariable("files", types.NewMapType(types.StringType, types.DynType)),
),
}
// Add declarations for custom params (all as strings)
for k, v := range customParams {
// Don't overwrite standard params
if _, ok := data[k]; !ok {
data[k] = v
varDecls = append(varDecls, cel.VariableDecls(decls.NewVariable(k, types.StringType)))
}
}
env, err := cel.NewEnv(varDecls...)
if err != nil {
return nil, err
}There was a problem hiding this comment.
Thanks, this is good simplification, updated the logic in 7afc027
pkg/matcher/cel.go
Outdated
| for k := range customParams { | ||
| // Don't overwrite standard params | ||
| if _, ok := conflictingVar[k]; !ok { | ||
| customParamDecls = append(customParamDecls, decls.NewVariable(k, types.StringType)) |
There was a problem hiding this comment.
IMO string is preferable since the values are strictly strings in the input, but I don't feel super strongly in that conviction
Allow custom parameters from Repository CR to be accessed directly as
CEL variables without template expansion. Parameters can now be used
as: param_name == "value" on top of "{{param_name}}" == "value".
Jira: https://issues.redhat.com/browse/SRVKP-9118
Signed-off-by: Akshay Pant <akshay.akshaypant@gmail.com>
theakshaypant
force-pushed
the
SRVKP-9118-fix-cel-custom-params
branch
from
78e3f5a to
7afc027
Compare
2 participants
📝 Description of the Change
Allow custom parameters from Repository CR to be accessed directly as CEL variables without template expansion. Parameters can now be used as: param_name == "value" on top of "{{param_name}}" == "value".
👨🏻 Linked Jira
https://issues.redhat.com/browse/SRVKP-9118
🔗 Linked GitHub Issue
N/A
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.