Skip to content

DO NOT MERGE: Test PR to debug cel condition tests#4472

Closed
liamfallon wants to merge 3 commits intokptdev:mainfrom
Nordix:cel-tests
Closed

DO NOT MERGE: Test PR to debug cel condition tests#4472
liamfallon wants to merge 3 commits intokptdev:mainfrom
Nordix:cel-tests

Conversation

@liamfallon
Copy link
Copy Markdown
Contributor

Duplicate PR of #4469, raised to debug the CEL tests on the PR

Signed-off-by: liamfallon <liam.fallon@est.tech>
Signed-off-by: liamfallon <liam.fallon@est.tech>
Copilot AI review requested due to automatic review settings April 9, 2026 05:40
@netlify
Copy link
Copy Markdown

netlify bot commented Apr 9, 2026

Deploy Preview for kptdocs ready!

Name Link
🔨 Latest commit 91c36e2
🔍 Latest deploy log https://app.netlify.com/projects/kptdocs/deploys/69d73e56e22d870008f96e16
😎 Deploy Preview https://deploy-preview-4472--kptdocs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. do-not-merge-work-in-progress Work in progress, do not merge Testing labels Apr 9, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds CEL-based conditional execution for kpt functions and extends results/status reporting so skipped functions are reflected in CLI output and pipeline status, along with unit/E2E coverage.

Changes:

  • Introduces a shared CEL environment (RunnerOptions.CELEnvironment) and condition evaluation logic to skip function execution when a CEL expression evaluates to false.
  • Propagates condition through Kptfile schema/types, CLI (fn eval --condition), and status/result types via a skipped flag.
  • Adds unit tests and E2E golden testdata for condition-met / condition-not-met scenarios; adjusts a couple of E2E timeouts.

Reviewed changes

Copilot reviewed 32 out of 33 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
thirdparty/kyaml/runfn/runfn.go Adds Condition to RunFns and wires it into the function runner.
thirdparty/cmdconfig/commands/cmdeval/cmdeval.go Adds --condition flag and passes condition through to runfn.RunFns / Kptfile function object.
thirdparty/cmdconfig/commands/cmdeval/cmdeval_test.go Updates eval runner tests for new runner options field.
pkg/lib/runneroptions/runneroptions.go Adds CELEnvironment to RunnerOptions and InitCELEnvironment() initializer.
pkg/lib/runneroptions/celenv.go Implements CEL environment creation + condition evaluation against KRM resources.
pkg/lib/kptops/render_test.go Initializes CEL environment in render tests.
pkg/lib/kptops/fs_test.go Initializes CEL environment in fs-based render tests.
pkg/api/kptfile/v1/types.go Adds Function.Condition and PipelineStepResult.Skipped.
pkg/api/fnresult/v1/types.go Adds Result.Skipped to structured function results.
internal/util/render/executor.go Avoids counting skipped functions as “executed”; surfaces skipped in step capture.
internal/util/get/get.go Initializes CEL environment for hook execution path.
internal/fnruntime/runner.go Evaluates conditions pre-exec, emits [SKIPPED], and records skipped results.
internal/fnruntime/condition_test.go Adds unit tests for conditional execution across runtimes.
internal/fnruntime/celeval_test.go Adds unit tests for CEL environment + evaluation behavior.
go.mod Adds CEL-related dependencies (cel-go, k8s.io/apiserver CEL libs, etc.).
go.sum Adds checksums for new dependencies.
e2e/testdata/live-apply/json-output/config.yaml Increases reconcile timeout used by test scenario.
e2e/testdata/live-apply/apply-depends-on/config.yaml Increases reconcile timeout used by test scenario.
e2e/testdata/fn-render/condition/condition-not-met/resources.yaml Adds E2E inputs for “condition not met” case.
e2e/testdata/fn-render/condition/condition-not-met/Kptfile Adds pipeline with a condition expression.
e2e/testdata/fn-render/condition/condition-not-met/.krmignore Ignores .expected for the new test package.
e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch Expected diff showing condition persistence + skipped status.
e2e/testdata/fn-render/condition/condition-not-met/.expected/config.yaml Expected stderr output including [SKIPPED] and 0 executed functions.
e2e/testdata/fn-render/condition/condition-met/resources.yaml Adds E2E inputs for “condition met” case.
e2e/testdata/fn-render/condition/condition-met/Kptfile Adds pipeline with a condition expression.
e2e/testdata/fn-render/condition/condition-met/.krmignore Ignores .expected for the new test package.
e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch Expected diff for the “condition met” path.
e2e/testdata/fn-render/condition/condition-met/.expected/config.yaml Expected stderr output for normal execution.
documentation/content/en/reference/schema/kptfile/kptfile.yaml Updates schema reference to include condition.
documentation/content/en/book/04-using-functions/_index.md Documents condition usage and adds examples.
documentation/content/en/book/01-getting-started/_index.md Minor wording fixes in getting started guide.
commands/fn/render/cmdrender.go Initializes CEL environment for kpt fn render.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +417 to +421
runner, _ := fnruntime.NewFunctionRunner(r.Ctx, fltr, "", fnResult, r.fnResults, opts)
if r.Condition != "" {
runner.SetCondition(r.Condition, opts.CELEnvironment)
}
return runner, nil
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewFunctionRunner returns an error but it’s being discarded (runner, _ := ...). Handle/propagate the error before using runner.

Also, when r.Condition is set but opts.CELEnvironment is nil, SetCondition will silently disable condition evaluation (because Filter checks fr.celEnv != nil). Consider returning an error when a condition is provided without a CEL environment (consistent with fnruntime.NewRunner).

Copilot uses AI. Check for mistakes.
Comment on lines 86 to 91
func (r *Runner) InitDefaults() {
r.RunnerOptions.InitDefaults(runneroptions.GHCRImagePrefix)
// Initialize CEL environment for condition evaluation
// Ignore error as conditions are optional; if CEL init fails, conditions will error at runtime
_ = r.RunnerOptions.InitCELEnvironment()
}
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitDefaults ignores the InitCELEnvironment() error, but conditions in Kptfiles will fail later if RunnerOptions.CELEnvironment is nil (see fnruntime.NewRunner). Either propagate the init error here or adjust the surrounding logic/comments so a CEL init failure can’t lead to confusing runtime behavior.

Copilot uses AI. Check for mistakes.
Comment on lines 171 to 176
func (r *EvalFnRunner) InitDefaults() {
r.RunnerOptions.InitDefaults(runneroptions.GHCRImagePrefix)
// Initialize CEL environment for condition evaluation
// Ignore error as conditions are optional; if CEL init fails, conditions will error at runtime
_ = r.RunnerOptions.InitCELEnvironment()
}
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitDefaults ignores the InitCELEnvironment() error. With --condition set, a nil CEL environment will cause the condition to be skipped (runfn path) or error later (fnruntime path). Please handle the error (return/print) so conditions can’t be silently ignored.

Copilot uses AI. Check for mistakes.
ResolveToImage ImageResolveFunc

// CELEnvironment is the shared CEL environment used to evaluate function conditions.
// It is initialised by InitDefaults and reused across all function runners.
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says CELEnvironment is initialized by InitDefaults, but initialization is now done by InitCELEnvironment().

Please update this comment to reflect the actual initialization flow (and avoid implying it’s always non-nil after InitDefaults).

Suggested change
// It is initialised by InitDefaults and reused across all function runners.
// It is initialized by InitCELEnvironment and reused across all function runners.
// It may be nil until InitCELEnvironment has been called successfully.

Copilot uses AI. Check for mistakes.

The expression receives a variable called `resources`, which is a list of all KRM resources passed to
this function step (after `selectors` and `exclude` have been applied). Each resource is a map with
the standard fields: `apiVersion`, `kind`, `metadata`, `spec`, `status`.
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs say each resources entry has standard fields apiVersion, kind, metadata, spec, status, but the implementation only guarantees defaults for apiVersion, kind, and metadata (see resourceToMap in pkg/lib/runneroptions/celenv.go). Either update the docs to clarify that spec/status may be absent or update the conversion to default those keys as well.

Suggested change
the standard fields: `apiVersion`, `kind`, `metadata`, `spec`, `status`.
the standard fields `apiVersion`, `kind`, and `metadata`. Depending on the resource, fields such as
`spec` and `status` may also be present.

Copilot uses AI. Check for mistakes.

[SKIPPED] "ghcr.io/kptdev/krm-functions-catalog/set-labels:latest" (condition not met)

Successfully executed 1 function(s) in 1 package(s).
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the “condition not met” example, the output says Successfully executed 1 function(s) even though skipped functions are not counted as executed (and the e2e expectations show 0 function(s)). Please update the example output to match the CLI behavior.

Suggested change
Successfully executed 1 function(s) in 1 package(s).
Successfully executed 0 function(s) in 1 package(s).

Copilot uses AI. Check for mistakes.
Comment on lines +153 to +155
// Initialize CEL environment for condition evaluation
// Ignore error as conditions are optional; if CEL init fails, conditions will error at runtime
_ = hookCmd.RunnerOptions.InitCELEnvironment()
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitCELEnvironment() errors are ignored here, but later code paths require RunnerOptions.CELEnvironment to evaluate conditions. Please handle/propagate this error (or intentionally disable condition support in this flow with an explicit message) to avoid silent misbehavior.

Suggested change
// Initialize CEL environment for condition evaluation
// Ignore error as conditions are optional; if CEL init fails, conditions will error at runtime
_ = hookCmd.RunnerOptions.InitCELEnvironment()
// Initialize CEL environment for condition evaluation.
// Fail early if initialization does not succeed because hook execution
// may require CEL to evaluate conditions.
if err := hookCmd.RunnerOptions.InitCELEnvironment(); err != nil {
return fmt.Errorf("initializing CEL environment for deployment hooks: %w", err)
}

Copilot uses AI. Check for mistakes.
Signed-off-by: liamfallon <liam.fallon@est.tech>
@liamfallon
Copy link
Copy Markdown
Contributor Author

Closed because tests are passing on PR #4469 now

@liamfallon liamfallon closed this Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge-work-in-progress Work in progress, do not merge size:XL This PR changes 500-999 lines, ignoring generated files. Testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants