Skip to content

feat: Add CEL expression evaluator command #2151

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chmouel
Copy link
Member

@chmouel chmouel commented Jun 30, 2025

Changes

This commit introduces a new command tkn pac cel that allows users to interactively evaluate CEL (Common Expression Language) expressions. The command is designed to help users test and debug CEL expressions, which are commonly used in Pipelines-as-Code.

Screen.Recording.2025-06-30.at.11.mp4

Key features include:

  • Interactive and non-interactive modes.
  • Support for webhook payloads and headers.
  • Provider auto-detection (GitHub, GitLab, Bitbucket Cloud, Bitbucket Data Center, Gitea).
  • Direct access to variables as per PAC documentation.
  • Cross-platform history with readline experience.
  • Comprehensive help and example expressions.

JIRA: https://issues.redhat.com/browse/SRVKP-8026

Submitter Checklist

  • 📝 Ensure your commit message is clear and informative. Refer to the How to write a git commit message guide. Include the commit message in the PR body rather than linking to an external site (e.g., Jira ticket).

  • ♽ Run make test lint before submitting a PR to avoid unnecessary CI processing. Consider installing pre-commit and running pre-commit install in the repository root for an efficient workflow.

  • ✨ We use linters to maintain clean and consistent code. Run make lint before submitting a PR. Some linters offer a --fix mode, executable with make fix-linters (ensure markdownlint and golangci-lint are installed).

  • 📖 Document any user-facing features or changes in behavior.

  • 🧪 While 100% coverage isn't required, we encourage unit tests for code changes where possible.

  • 🎁 If feasible, add an end-to-end test. See README for details.

  • 🔎 Address any CI test flakiness before merging, or provide a valid reason to bypass it (e.g., token rate limitations).

  • If adding a provider feature, fill in the following details:

    • GitHub App
    • GitHub Webhook
    • Gitea/Forgejo
    • GitLab
    • Bitbucket Cloud
    • Bitbucket Data Center

    (update the provider documentation accordingly)

@Copilot Copilot AI review requested due to automatic review settings June 30, 2025 09:40
@chmouel
Copy link
Member Author

chmouel commented Jun 30, 2025

/jira

Copy link

💫 Jira ticket created: SRVKP-8026

Copy link
Contributor

@Copilot 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

This PR adds a new tkn pac cel command, providing an interactive and non-interactive REPL for evaluating CEL expressions against real webhook payloads and HTTP headers.

  • Introduces the cel subcommand in the CLI root.
  • Implements provider detection, event parsing, and an interactive readline-based REPL.
  • Adds comprehensive tests and updates documentation.

Reviewed Changes

Copilot reviewed 7 out of 41 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pkg/cmd/tknpac/root.go Register the new cel command under tkn pac
pkg/cmd/tknpac/cel/templates/help.tmpl Create help template with available variables & examples
pkg/cmd/tknpac/cel/cel.go Implement cel command logic, provider detection, REPL
pkg/cmd/tknpac/cel/cel_test.go Add extensive unit tests covering parsing, evaluation, providers, flags, files
pkg/cel/cel.go Extend CEL environment to include direct variables
docs/content/docs/guide/cli.md Document usage for tkn pac cel
go.mod Add github.com/chzyer/readline dependency
Comments suppressed due to low confidence (2)

docs/content/docs/guide/cli.md:425

  • [nitpick] The example headers block includes Request method: POST, which is not a standard HTTP header and will be treated as a custom key by the parser. Consider removing or clarifying this line.
Request method: POST

pkg/cel/cel.go:55

  • [nitpick] The body is marshaled then unmarshaled to a map for CEL. If body is already a map or structured data, avoid the double marshal/unmarshal by type asserting or converting directly.
			decls.NewVariable("headers", mapStrDyn),

@chmouel
Copy link
Member Author

chmouel commented Jun 30, 2025

🤖 AI Review provided by gemini:gemini-2.5-pro:

Excellent! This is a substantial and well-thought-out feature commit. It adds a powerful new tkn pac cel command with great features like provider auto-detection and an interactive shell. The test coverage is also impressive.

Here is a careful analysis of the changes, highlighting potential bugs, spelling mistakes, and code improvements.

Summary of Analysis

Overall, this is a high-quality commit. The core logic is solid, and the feature is well-implemented with good test coverage. The identified issues are relatively minor and easy to address.

  • Bugs: A couple of minor bugs were found, mainly related to redundant code and case-sensitive handling of HTTP headers.
  • Spelling: A few minor typos in comments and test data.
  • Improvements: Suggestions to make the code more robust and idiomatic.

🐛 Bugs & Potential Issues

These are items that could cause incorrect behavior under certain conditions.

  1. Redundant JSON Unmarshaling

    In the functions that parse provider payloads, json.Unmarshal is called on the payload body after the provider's SDK has already parsed it. This is unnecessary and can be removed for cleaner code.

    • File: pkg/cmd/tknpac/cel/cel.go
    • In eventFromGitHub(): The line _ = json.Unmarshal(body, &ghEvent) is redundant because github.ParseWebHook already unmarshals the data.
    • In eventFromGitLab(): The block if err := json.Unmarshal(body, &eventInt); err != nil { ... } is redundant because gitlab.ParseWebhook already handles this.

    Recommendation: Remove these redundant json.Unmarshal calls. The preceding ParseWebhook functions have already done the work.

  2. Case-Sensitive GitHub Header Lookup

    HTTP headers are defined as case-insensitive. While most of the new code handles this correctly (e.g., using strings.EqualFold), the GitHub event parsing does not.

    • File: pkg/cmd/tknpac/cel/cel.go
    • In eventFromGitHub(): The line event.EventType = headers["X-GitHub-Event"] performs a case-sensitive lookup. If a user provides headers with x-github-event, this will fail to find the event type.

    Recommendation: Make the lookup case-insensitive to be more robust. You can check for both casings, as is done in the detectProvider function:

    // In eventFromGitHub()
    event.EventType = headers["X-GitHub-Event"]
    if event.EventType == "" {
        event.EventType = headers["x-github-event"]
    }

📝 Spelling and Documentation

These are minor typos and clarifications in comments and documentation.

  1. Typo in Test Error String

    There's a casing typo in an expected error message within the tests.

    • File: pkg/cmd/tknpac/cel/cel_test.go
    • In TestCommandExecution and TestCommandFileHandling: The expectation wantErrContains: "unknown X-Github-Event" should be wantErrContains: "unknown X-GitHub-Event".
  2. Misleading Comment

    A comment in the main command logic is slightly misleading.

    • File: pkg/cmd/tknpac/cel/cel.go, in Command() function.
    • Comment: // Create files data structure (empty for now since we don't have a provider)
    • Issue: At this point in the code, the provider has been determined. The reason the files data is empty is because file change detection is not implemented in the CLI.
    • Recommendation: Change the comment to be more accurate, for example: // Create files data structure (always empty in CLI mode).
  3. Minor Typo in Documentation

    • File: docs/content/docs/guide/cli.md
    • Text: "...content of the webhook requests..."
    • Recommendation: Change to "...content of the webhook request...".

💡 Code Improvements & Idiomatic Go

These are suggestions to make the code cleaner, more robust, or more idiomatic.

  1. Refactor Bitbucket Provider Detection Logic

    The logic to distinguish Bitbucket Cloud from Data Center in detectProvider works, but it contains a slightly strange-looking repeated if block. It can be made more concise.

    • File: pkg/cmd/tknpac/cel/cel.go
    • Original Code:
      // ...
      if actor, ok := payload["actor"].(map[string]any); ok {
          if _, hasAccountID := actor["account_id"]; hasAccountID {
              return "bitbucket-cloud", nil
          }
      }
      // Bitbucket Data Center has different structure
      if actor, ok := payload["actor"].(map[string]any); ok {
          if _, hasID := actor["id"]; hasID {
              return "bitbucket-datacenter", nil
          }
      }
      // ...
    • Recommendation: Combine these into a single if block for better readability.
      if actor, ok := payload["actor"].(map[string]any); ok {
          if _, hasAccountID := actor["account_id"]; hasAccountID {
              return "bitbucket-cloud", nil
          }
          // Heuristic: if it has an `id` but not an `account_id`, assume it's Data Center.
          if _, hasID := actor["id"]; hasID {
              return "bitbucket-datacenter", nil
          }
      }
  2. Explicitly State files.* Limitation in Help Template

    The help template is very good, but the note about files.* functionality could be slightly punchier.

    • File: pkg/cmd/tknpac/cel/templates/help.tmpl
    • Suggestion: The current text is clear but a bit long. Consider combining the two lines about file variables.
      -  📁 Files (files.* - ❌ always empty in CLI):
      -    • files.all, files.added, files.modified, etc.
      -    ⚠️  Note: fileChanged, fileDeleted, fileModified and all files.* variables are not implemented in the CLI and will always be empty.
      +  📁 Files (files.*):
      +    ⚠️  Note: File-related variables (e.g., `files.all`) and functions (e.g., `fileChanged`) are not supported in the CLI and will be empty or false.

    This is a stylistic choice; the original is also perfectly fine.

Final Verdict

This is an excellent commit that adds a very useful feature. The implementation is robust, especially with the fallback from readline to survey and the thoughtful provider auto-detection logic. The identified issues are minor and do not detract from the overall quality of the contribution. Congratulations to the author on a job well done

@chmouel
Copy link
Member Author

chmouel commented Jun 30, 2025

/retest

@zakisk
Copy link
Contributor

zakisk commented Jul 1, 2025

/lgtm

Copy link

@pipelines-as-code pipelines-as-code bot left a comment

Choose a reason for hiding this comment

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

Congrats @chmouel your PR Has been approved 🎉

✅ Pull Request Approved

Approval Status:

  • Required Approvals: 1
  • Current Approvals: 1

👥 Reviewers Who Approved:

Reviewer Permission Level Approval Status
@zakisk admin

📝 Next Steps

  • Ensure all required checks pass
  • Comply with branch protection rules
  • Request a maintainer to merge using the /merge command (or merge it
    directly if you have repository permission).

Automated by the PAC Boussole 🧭

@zakisk
Copy link
Contributor

zakisk commented Jul 1, 2025

/retest

@chmouel chmouel force-pushed the add-tknpac-command-to-evaluate-cel-expressions branch from 17052ff to e3c0d9c Compare July 1, 2025 10:58
This commit introduces a new command `tkn pac cel` that allows
users to interactively evaluate CEL (Common Expression Language)
expressions. The command is designed to help users test and debug
CEL expressions, which are commonly used in Pipelines-as-Code.

Key features include:

- Interactive and non-interactive modes.
- Support for webhook payloads and headers.
- Provider auto-detection (GitHub, GitLab, Bitbucket Cloud,
  Bitbucket Data Center, Gitea).
- Direct access to variables as per PAC documentation.
- Cross-platform history with readline experience.
- Comprehensive help and example expressions.

Signed-off-by: Chmouel Boudjnah <[email protected]>

feat: Support gosmee-generated shell scripts for headers

- Allow the CLI to parse headers from gosmee scripts.
- Add support for JSON, plain text and gosmee scripts header formats.
- Update documentation to reflect the changes in the header file formats.
- Add tests to validate the script and header parsing.

Signed-off-by: Chmouel Boudjnah <[email protected]>
@zakisk zakisk force-pushed the add-tknpac-command-to-evaluate-cel-expressions branch from e3c0d9c to e7347b5 Compare July 2, 2025 08:56
@zakisk
Copy link
Contributor

zakisk commented Jul 3, 2025

it is LGTM, do I merge or you wanna make more changes?

@chmouel
Copy link
Member Author

chmouel commented Jul 3, 2025

@zakisk i think wait for it, there is some edge case that makes it buggy compared to live event... i'll let you know if we keep it "as is"

@chmouel chmouel marked this pull request as draft July 3, 2025 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants