feat(cel): add direct custom param variable access in expressions #2320

theakshaypant · 2025-11-12T07:47:29Z

📝 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

🧪 Testing Strategy

🤖 AI Assistance

I have not used any AI assistance for this PR.
I have used AI assistance for this PR.

If you have used AI assistance, please provide the following details:

Which LLM was used?

Extent of AI Assistance:

Documentation and research only
Unit tests or E2E tests only
Code generation (parts of the code)
Full code generation (most of the PR)
PR description and comments
Commit message(s)

Important

If the majority of the code in this PR was generated by an AI, please add a Co-authored-by trailer to your commit message.
For example:

Co-authored-by: Gemini [email protected]
Co-authored-by: ChatGPT [email protected]
Co-authored-by: Claude [email protected]
Co-authored-by: Cursor [email protected]
Co-authored-by: Copilot [email protected]

**💡You can use the script ./hack/add-llm-coauthor.sh to automatically add
these co-author trailers to your commits.

✅ Submitter Checklist

pipelines-as-code · 2025-11-12T07:48:14Z

🔍 PR Lint Feedback

Note: This automated check helps ensure your PR follows our contribution guidelines.

⚠️ Items that need attention:

🤖 AI attribution

The following commits lack an explicit AI attribution footer:

354354e feat(cel): add direct custom param variable access in expressions

If no AI assistance was used for a commit, you can ignore this warning.
Otherwise add an Assisted-by: or Co-authored-by: footer referencing the AI used.

ℹ️ Next Steps

Review and address the items above
Push new commits to update this PR
This comment will be automatically updated when issues are resolved

🔧 Admin Tools (click to expand)

Automated Issue/Ticket Creation:

/issue-create - Generate a GitHub issue from this PR content using AI
/jira-create - Create a SRVKP Jira ticket from this PR content using AI

⚠️ Important: Always review and edit generated content before finalizing tickets/issues.
The AI-generated content should be used as a starting point and may need adjustments.

These commands are available to maintainers and will post the generated content as PR comments for review.

_{🤖 This feedback was generated automatically by the PR CI system}

theakshaypant

~~TODO: Docs update~~

theakshaypant · 2025-11-12T07:50:18Z

pkg/matcher/cel.go

-		cel.Lib(celPac{vcx, ctx, event}),
+	conflictingVar := make(map[string]bool)
+	for k, v := range customParams {
+		// Don't overwrite standard params


Want to confirm if we want this behaviour. Modeled it on this

theakshaypant · 2025-11-12T07:50:56Z

pkg/matcher/cel.go

+	for k := range customParams {
+		// Don't overwrite standard params
+		if _, ok := conflictingVar[k]; !ok {
+			customParamDecls = append(customParamDecls, decls.NewVariable(k, types.StringType))


All variables are used as strings, do we want them to be DynType?

theakshaypant · 2025-11-12T08:04:59Z

/gemini review

gemini-code-assist

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.

pkg/matcher/annotation_matcher.go

pkg/matcher/annotation_matcher_test.go

theakshaypant · 2025-11-13T12:35:03Z

/gemini review

gemini-code-assist

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!

pkg/matcher/annotation_matcher.go

docs/content/docs/guide/customparams.md

gemini-code-assist · 2025-11-13T12:37:31Z

pkg/matcher/cel.go

+		} else {
+			conflictingVar[k] = true
+		}


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 logger from MatchPipelinerunByAnnotation into this celEvaluate function.

Add a logging statement inside this else block, like:
logger.Warnf("Custom parameter %q conflicts with a standard CEL variable and will be ignored", k)

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 <[email protected]>

aThorp96

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

aThorp96 · 2025-11-14T16:35:31Z

docs/content/docs/guide/matchingevents.md

 | `headers`         | The full set of headers as passed by the Git provider. Example: `headers['x-github-event']` retrieves the event type on GitHub.  |
 | `.pathChanged`    | A suffix function to a string that can be a glob of a path to check if changed. (Supported only for `GitHub` and `GitLab` providers.) |
 | `files`           | The list of files that changed in the event (`all`, `added`, `deleted`, `modified`, and `renamed`). Example: `files.all` or `files.deleted`. For pull requests, every file belonging to the pull request will be listed. |
+| Custom params     | Any [custom parameters]({{< relref "/docs/guide/customparams" >}}) defined in the Repository CR `spec.params` are available as CEL variables. Example: `enable_ci == "true"`. See [Using custom parameters in CEL matching expressions]({{< relref "/docs/guide/customparams#using-custom-parameters-in-cel-matching-expressions" >}}) for details. |


Since some custom params may not be available based on their filter, "any param defined" could be interpreted to suggest filters may not be applied in this case. I think "any param provided" is less absolute so is more aligned when a param doesn't match its filter. Maybe there is another word which would be clearer though?

Suggested change

| Custom params | Any [custom parameters]({{< relref "/docs/guide/customparams" >}}) defined in the Repository CR `spec.params` are available as CEL variables. Example: `enable_ci == "true"`. See [Using custom parameters in CEL matching expressions]({{< relref "/docs/guide/customparams#using-custom-parameters-in-cel-matching-expressions" >}}) for details. |

| Custom params | Any [custom parameters]({{< relref "/docs/guide/customparams" >}}) provided from the Repository CR `spec.params` are available as CEL variables. Example: `enable_ci == "true"`. See [Using custom parameters in CEL matching expressions]({{< relref "/docs/guide/customparams#using-custom-parameters-in-cel-matching-expressions" >}}) for details. |

aThorp96 · 2025-11-14T16:49:17Z

pkg/matcher/annotation_matcher.go

+			// Determine the effective repository for this PipelineRun
+			effectiveRepo := repo
+			if prMatch.Repo != nil {
+				effectiveRepo = prMatch.Repo
+			}


An event is repository-scoped, so as far as I know the repository should be the same across all pipelineruns in the prMatch. AFAICT prMatch.Repo can never be different within the same event unless the pipelinerun does not annotate a targetNamespace.

I believe the checks above are unnecessary as well, since the Repo is identified in all cases by the event's repository URL, and Repository CRs must all have unique URL.

In any case, I think this can be simplified by using repo instead of using an effectiveRepo which gets updated each loop.

That was my original thought as well. After this comment, I modified my implementation. Would love to get your thoughts on this.

Hmm. I believe Gemini is BSing. AFAIK one event can only create pipelineruns for a single Repository CR, but I'll defer to @chmouel's understanding. I could be wrong

Even Claude backed the comment so I decided to move to this version of getting an effective repo.

aThorp96 · 2025-11-14T16:51:28Z

pkg/matcher/annotation_matcher.go

+				if cached, found := customParamsCache[cacheKey]; found {
+					customParams = cached
+				} else {
+					customParams = resolveCustomParamsForCEL(ctx, effectiveRepo, event, cs, vcx, eventEmitter, logger)


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

aThorp96 · 2025-11-14T16:55:37Z

pkg/matcher/annotation_matcher.go

+// 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.
+// All parameters are returned as strings, including those from secret_ref.
+func resolveCustomParamsForCEL(ctx context.Context, repo *apipac.Repository, event *info.Event, cs *params.Run, vcx provider.Interface, eventEmitter *events.EventEmitter, logger *zap.SugaredLogger) map[string]string {


Any reason we can't use customparams.NewCustomParams() directly?

Again, my thinking was influenced by this comment. Would be nice to get an experienced perspective.

Sorry I think my original comment wasn't clear. I mean to ask: wherever we call resolveCustomParamsForCEL, would we not instead call customparams.NewCustomParams().GetParams()? It seems like these are very similar, but maybe I'm missing the difference

Ah, sorry meant to have this reply on the earlier comment.
As for customparams.NewCustomParams(), it is used by resolveCustomParamsForCEL and then filters the params to only return the ones defined in the repo CR.

pipelines-as-code bot added the repo-cr label Nov 12, 2025

theakshaypant commented Nov 12, 2025

View reviewed changes

gemini-code-assist bot reviewed Nov 12, 2025

View reviewed changes

pkg/matcher/annotation_matcher.go Outdated Show resolved Hide resolved

pkg/matcher/annotation_matcher_test.go Outdated Show resolved Hide resolved

theakshaypant force-pushed the SRVKP-9118-fix-cel-custom-params branch from 3d13ed7 to 6e26664 Compare November 13, 2025 12:34

pipelines-as-code bot added documentation Improvements or additions to documentation feature New feature or request labels Nov 13, 2025

gemini-code-assist bot reviewed Nov 13, 2025

View reviewed changes

theakshaypant force-pushed the SRVKP-9118-fix-cel-custom-params branch from 6e26664 to 52e29ce Compare November 13, 2025 12:53

theakshaypant force-pushed the SRVKP-9118-fix-cel-custom-params branch from 52e29ce to 354354e Compare November 13, 2025 14:36

theakshaypant changed the title ~~feat(cel): add direct custom param variable access~~ feat(cel): add direct custom param variable access in expressions Nov 13, 2025

theakshaypant marked this pull request as ready for review November 14, 2025 02:16

theakshaypant requested a review from aThorp96 November 14, 2025 02:17

aThorp96 reviewed Nov 14, 2025

View reviewed changes

	\| Custom params \| Any [custom parameters]({{< relref "/docs/guide/customparams" >}}) defined in the Repository CR `spec.params` are available as CEL variables. Example: `enable_ci == "true"`. See [Using custom parameters in CEL matching expressions]({{< relref "/docs/guide/customparams#using-custom-parameters-in-cel-matching-expressions" >}}) for details. \|
	\| Custom params \| Any [custom parameters]({{< relref "/docs/guide/customparams" >}}) provided from the Repository CR `spec.params` are available as CEL variables. Example: `enable_ci == "true"`. See [Using custom parameters in CEL matching expressions]({{< relref "/docs/guide/customparams#using-custom-parameters-in-cel-matching-expressions" >}}) for details. \|

feat(cel): add direct custom param variable access in expressions #2320

Are you sure you want to change the base?

feat(cel): add direct custom param variable access in expressions #2320

Conversation

theakshaypant commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Description of the Change

👨🏻‍ Linked Jira

🔗 Linked GitHub Issue

🚀 Type of Change

🧪 Testing Strategy

🤖 AI Assistance

✅ Submitter Checklist

Uh oh!

pipelines-as-code bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 PR Lint Feedback

⚠️ Items that need attention:

🤖 AI attribution

ℹ️ Next Steps

Uh oh!

theakshaypant left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theakshaypant commented Nov 12, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

theakshaypant commented Nov 13, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

aThorp96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theakshaypant Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theakshaypant Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

theakshaypant commented Nov 12, 2025 •

edited

Loading

pipelines-as-code bot commented Nov 12, 2025 •

edited

Loading

theakshaypant left a comment •

edited

Loading

theakshaypant Nov 14, 2025 •

edited

Loading

theakshaypant Nov 17, 2025 •

edited

Loading