-
Notifications
You must be signed in to change notification settings - Fork 117
feat(cel): add direct custom param variable access in expressions #2320
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -9,9 +9,11 @@ import ( | |||||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode" | ||||||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" | ||||||
| apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" | ||||||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/customparams" | ||||||
| pacerrors "github.com/openshift-pipelines/pipelines-as-code/pkg/errors" | ||||||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/events" | ||||||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" | ||||||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction" | ||||||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" | ||||||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/params" | ||||||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" | ||||||
|
|
@@ -210,6 +212,12 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger | |||||
| } | ||||||
| logger.Info(infomsg) | ||||||
|
|
||||||
| // 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)) | ||||||
| } | ||||||
|
|
||||||
| celValidationErrors := []*pacerrors.PacYamlValidations{} | ||||||
| for _, prun := range pruns { | ||||||
| prMatch := Match{ | ||||||
|
|
@@ -280,7 +288,7 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger | |||||
| if celExpr, ok := prun.GetObjectMeta().GetAnnotations()[keys.OnCelExpression]; ok { | ||||||
| checkPipelineRunAnnotation(prun, eventEmitter, repo) | ||||||
|
|
||||||
| out, err := celEvaluate(ctx, celExpr, event, vcx) | ||||||
| out, err := celEvaluate(ctx, celExpr, event, vcx, customParams) | ||||||
| if err != nil { | ||||||
| logger.Errorf("there was an error evaluating the CEL expression, skipping: %v", err) | ||||||
| if checkIfCELEvaluateError(err) { | ||||||
|
|
@@ -508,3 +516,38 @@ func MatchRunningPipelineRunForIncomingWebhook(eventType, incomingPipelineRun st | |||||
| } | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is slightly inaccurate. The function does not exclude reserved keywords; that logic is handled in
Suggested change
|
||||||
| // 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 { | ||||||
aThorp96 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| if repo == nil || repo.Spec.Params == nil { | ||||||
| return map[string]string{} | ||||||
| } | ||||||
|
|
||||||
| // Create kubeinteraction interface | ||||||
| kinteract, err := kubeinteraction.NewKubernetesInteraction(cs) | ||||||
| if err != nil { | ||||||
| logger.Warnf("failed to create kubernetes interaction for custom params: %s", err.Error()) | ||||||
| return map[string]string{} | ||||||
| } | ||||||
|
Comment on lines
+529
to
+533
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't you have kubeinteraction in params.Run? |
||||||
|
|
||||||
| // Use existing customparams package to resolve all params | ||||||
| cp := customparams.NewCustomParams(event, repo, cs, kinteract, eventEmitter, vcx) | ||||||
| allParams, _, err := cp.GetParams(ctx) | ||||||
| if err != nil { | ||||||
| eventEmitter.EmitMessage(repo, zap.WarnLevel, "CustomParamsCELError", | ||||||
| fmt.Sprintf("failed to resolve custom params for CEL: %s", err.Error())) | ||||||
| return map[string]string{} | ||||||
| } | ||||||
|
|
||||||
| // Filter to only include params defined in repo.Spec.Params (not standard PAC params) | ||||||
| result := make(map[string]string) | ||||||
| for _, param := range *repo.Spec.Params { | ||||||
| if value, ok := allParams[param.Name]; ok { | ||||||
| result[param.Name] = value | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return result | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.:
IIUC on a push event, the above pipelinerun's CEL expression won't be false, it will be an error.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_targetand eventarget_branchThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more details in docs in 7afc027