feat: support glob in incoming webhook targets #2294
Conversation
Summary of ChangesHello @theakshaypant, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of incoming webhook configurations by introducing support for regular expressions in target matching. It ensures that users can define more sophisticated rules for triggering pipelines based on branch names, while also incorporating robust security validations to prevent common misconfigurations that could lead to unintended access or behavior. The 'first-match-wins' logic provides clear precedence for rule resolution. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
pipelines-as-code
bot
added
documentation
![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 valuable feature by adding regex support for incoming webhook targets, along with important security validations. The implementation is solid, and the documentation and tests are comprehensive.
My review focuses on a few areas for improvement:
- Correctness and Duplication: There's a small bug in the regex detection logic, and this logic is duplicated in two places. Consolidating it into a single, corrected helper function would improve maintainability.
- Performance: Regex patterns are compiled on every request, which could be optimized by caching the compiled expressions.
- Usability: The security warnings for unanchored patterns, while well-intentioned, might be too aggressive for common use cases, potentially leading to excessive noise.
Overall, this is a great addition. Addressing these points will make the feature even more robust and efficient.
There was a problem hiding this comment.
There is another discussion that can be potentially be had regarding the usage of glob patterns instead of regex here.
My thoughts on this are
- Regex provides stricter, auditable control over branch naming conventions.
For example, enforce rules like^PROJECT-\d+/feature/[a-z0-9._-]+$or^release-(\d+\.\d+)$ - Fine grained control - anchors, groups, etc
- Glob doesn’t offer the same level of flexibility and is harder to define for advanced use cases
- While using glob patterns may be easier for casual users, regex provides more control to the users defining the pipeline.
There was a problem hiding this comment.
We can potentially also link some regex best practices or link a tool which can help the users with their patterns.
Did not want to endorse any random blog so skipped it for now.
There was a problem hiding this comment.
Go's regex uses the RE2 engine, which intentionally excludes features that can cause catastrophic backtracking protecting from ReDoS but have still added them some tests for them as I wanted to confirm the process wouldn't be slowed down in case someone does add an evil regex (maliciously or mistakenly)!
theakshaypant
force-pushed
the
SRVKP-7364-feat-support-regexp-incoming-webhook-targets
branch
2 times, most recently
from
d24f0f8 to
e423be4
Compare
|
I think we need multiple scenarios for E2E Tests for that, there is already a e2e capability in the tests for incoming tests that could be extended i believe. |
theakshaypant
force-pushed
the
SRVKP-7364-feat-support-regexp-incoming-webhook-targets
branch
from
e423be4 to
4b8d8ae
Compare
theakshaypant
force-pushed
the
SRVKP-7364-feat-support-regexp-incoming-webhook-targets
branch
3 times, most recently
from
6c65541 to
b63f0ea
Compare
pkg/matcher/repo_runinfo_matcher.go
Outdated
| } | ||
|
|
||
| // Auto-detect regex patterns by checking for metacharacters. | ||
| if strings.ContainsAny(target, "^$*+|[](){}") || |
There was a problem hiding this comment.
I don't really like doing strings.Contains to try to detect regexp, this feels fragile, but i can live with it if we are sure there is no bug,
are we sure that git branch cannot contains those chars (for ex if i a user called their branch foo-.+-bar) ?
There was a problem hiding this comment.
I see your point, did not consider users having "unconventional" branch names.
Was trying to add break early mechanism in case of a non-regex target.
IMO wouldn't make a difference removing this condition completely, any invalid regex would be rejected by the Compile.
Will get rid of the condition and add test case(s) for such creative branch names.
There was a problem hiding this comment.
@chmouel Spent some time thinking about this and trying out a few things.
We cannot expect users to give conventional branch names and targets. Due to this, one fundamental regex problem will prevail - dots. For example, target v1.2.0 would match with v1.230 and so on.
Some alternate options I considered
1. Adding a new regexTarget in the CR (dont want to do this would cause confusion on which target should be used in case of multiple matches in 2 different fields)
2. Have a regex:/re: prefix for targets specifying a regex but here again we would be asking users to put restrictions on branch naming.
3. Use glob patterns instead, I know there is a comment on why we should use regex over glob but having seen more examples and our ther options, I feel this is the way forward
There was a problem hiding this comment.
yeah i think globbing is good enough for me and we don't need to do crazy things like regexp detection.. it's rare that people needs more than that...
the other options are good option too, I was thinking about that same alternative options as well 👏🏻 this may be something for later if some customers/users somehow really really want regexp instead
Add glob pattern matching support for incoming webhook targets
using shell-style wildcards for intuitive branch filtering.
- Add matchTarget() helper to match glob patterns
- Implement IncomingWebhookRule() with first-match-wins semantics
- Support glob wildcards: * (any chars), ? (single char),
[0-9] (ranges), {a,b} (alternation)
- Add comprehensive unit and e2e tests
- Update documentation with glob syntax, examples, and best
practices
- Update sample repository with glob pattern examples
Jira: https://issues.redhat.com/browse/SRVKP-7364
Signed-off-by: Akshay Pant <[email protected]>
Assisted-by: Claude-Sonnet-4.5 (via Cursor)
theakshaypant
force-pushed
the
SRVKP-7364-feat-support-regexp-incoming-webhook-targets
branch
from
b63f0ea to
9958898
Compare
theakshaypant
changed the title
theakshaypant
changed the title
2 participants
📝 Description of the Change
Add gloab pattern matching support for incoming webhook targets.
Implements first-match-wins strategy when multiple targets match.
👨🏻 Linked Jira
Jira: https://issues.redhat.com/browse/SRVKP-7364
🔗 Linked GitHub Issue
Addresses a comment in #1398
🚀 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 [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.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.