feat(error-detection): add multiple pattern support #2325
Conversation
|
/gemini review |
![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 enhancement by adding support for multiple error detection patterns at both the global and repository levels. The implementation is well-structured, with corresponding updates to the CRD, configuration parsing, and documentation. The code is clean and the new tests cover the added functionality. I've found one logic issue where the documented behavior for disabling error detection at the repository level is not correctly implemented. My review includes a suggestion to fix this.
| if repoErrorDetection.Patterns != nil { | ||
| patterns = repoErrorDetection.Patterns | ||
| } |
There was a problem hiding this comment.
There's a logic issue here that contradicts the feature's documentation. According to the CRD documentation, providing an empty patterns array (patterns: []) should effectively disable error detection for the repository. However, the current implementation appends the global patterns even when the repository-specific patterns list is empty, causing it to fall back to global patterns instead of disabling detection. This could lead to unexpected behavior for users who want to opt-out of error detection for a specific repository.
To align with the documentation, you should check if repoErrorDetection.Patterns is an empty slice and, if so, stop further pattern processing for this repository.
| if repoErrorDetection.Patterns != nil { | |
| patterns = repoErrorDetection.Patterns | |
| } | |
| if repoErrorDetection.Patterns != nil { | |
| // As per documentation, an empty list of patterns disables error detection for the repo. | |
| if len(repoErrorDetection.Patterns) == 0 { | |
| return annotations | |
| } | |
| patterns = repoErrorDetection.Patterns | |
| } |
There was a problem hiding this comment.
Have made it more explicit in the docs that empty pattern list WOULD NOT disable error detection for the repo.
theakshaypant
force-pushed
the
SRVKP-7237-feat-repo-error-detection-setting
branch
from
feefb68 to
f922a79
Compare
theakshaypant
force-pushed
the
SRVKP-7237-feat-repo-error-detection-setting
branch
from
f922a79 to
a0046de
Compare
pipelines-as-code
bot
added
documentation
| // parseErrorDetectionPatterns parses the error-detection-simple-regexp from ConfigMap | ||
| // and populates ErrorDetectionSimpleRegexp as []string. It supports: | ||
| // 1. Single string pattern (backward compatible) | ||
| // 2. JSON array format: ["pattern1", "pattern2"] | ||
| // 3. Newline-separated patterns (YAML multi-line). | ||
| func parseErrorDetectionPatterns(setting *Settings, config map[string]string) error { |
There was a problem hiding this comment.
Would love some inputs on this. This approach came to my mind to ensure backward compatibility while allowing multiple patterns as input.
| {{< hint info >}} | ||
| **Global Override:** The global `error-detection-from-container-logs` setting | ||
| must be enabled (default: `true`) for error detection to work. If disabled | ||
| globally, repository-level settings cannot override it. | ||
| {{< /hint >}} |
There was a problem hiding this comment.
Have kept a global override so admins could disable error detection at an org level (for whatever (?) reason).
theakshaypant
force-pushed
the
SRVKP-7237-feat-repo-error-detection-setting
branch
from
a0046de to
905496c
Compare
theakshaypant
force-pushed
the
SRVKP-7237-feat-repo-error-detection-setting
branch
from
905496c to
49b7a7c
Compare
Add support for configuring multiple error detection regex patterns at both global (ConfigMap) and repository (CR) levels. This allows users to match different error formats from various linters and tools in a single pipeline. Global Configuration (ConfigMap): - Changed error-detection-simple-regexp to support arrays - Supports 3 formats: single pattern (backward compatible), multi-line YAML, and JSON array Repository CR: - Added ErrorDetectionSettings with patterns array and max_number_of_lines - Patterns are additive with global patterns - Per-repository max_number_of_lines override Jira: https://issues.redhat.com/browse/SRVKP-7237 Signed-off-by: Akshay Pant <[email protected]> Assisted-by: Cursor <[email protected]>
theakshaypant
force-pushed
the
SRVKP-7237-feat-repo-error-detection-setting
branch
from
49b7a7c to
b4099d4
Compare
1 participant
📝 Description of the Change
Add support for configuring multiple error detection regex patterns at both global (ConfigMap) and repository (CR) levels. This allows users to match different error formats from various linters and tools in a single pipeline.
Global Configuration (ConfigMap):
Repository CR:
👨🏻 Linked Jira
Jira: https://issues.redhat.com/browse/SRVKP-7237
🔗 Linked GitHub Issue
N/A
🚀 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.