feat: Introduce error log snippet line config #2237
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @chmouel, 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 enhances the error logging functionality by introducing a new configurable setting for the number of lines displayed in error log snippets. It also establishes a character limit for these snippets to ensure compatibility with GitHub's Check Run API, providing more control and reliability for error reporting within pull requests.
Highlights
- New Configuration Option: Introduced a new configuration option
error-log-snippet-number-of-linesto control the number of lines displayed in error log snippets. - Default Value and Character Limit: Set the default value for
error-log-snippet-number-of-linesto 3 and implemented a maximum character limit of 65536 for all error log snippets to prevent exceeding GitHub Check Run API limits. - Code Refactoring: Replaced a previously hardcoded constant for log snippet lines with the new configurable setting, improving flexibility.
- Documentation Update: Updated the documentation to describe the new configuration and the character limit.
- End-to-End Tests: Added end-to-end tests with golden files to validate the output of the error log snippets with custom line configurations.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR introduces configurable error log snippet line limits and character restrictions for error reporting in pipelines. The changes improve control over error message display while preventing API character limit violations, particularly for GitHub Check Run API.
- Added new configuration option
error-log-snippet-number-of-lineswith default value of 3 - Implemented 65536 character limit for error log snippets to comply with GitHub Check Run API limits
- Added end-to-end tests with golden file matching to verify proper output formatting
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| config/302-pac-configmap.yaml | Adds new configuration setting with documentation |
| pkg/params/settings/config.go | Defines new ErrorLogSnippetNumberOfLines field with default value |
| pkg/reconciler/status.go | Replaces hardcoded constant with configurable value from settings |
| pkg/kubeinteraction/status/task_status.go | Implements character limit enforcement for error snippets |
| docs/content/docs/install/settings.md | Documents new configuration option and character limits |
| test/gitea_test.go | Adds e2e test to verify custom line configuration works |
| test/pkg/gitea/test.go | Adds golden file matching utility function |
| test/testdata/ | Test data and golden files for e2e validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
pkg/reconciler/status.go
Outdated
| @@ -81,7 +80,11 @@ func (r *Reconciler) updateRepoRunStatus(ctx context.Context, logger *zap.Sugare | |||
| } | |||
|
|
|||
| func (r *Reconciler) getFailureSnippet(ctx context.Context, pr *tektonv1.PipelineRun) string { | |||
| taskinfos := kstatus.CollectFailedTasksLogSnippet(ctx, r.run, r.kinteract, pr, logSnippetNumLines) | |||
| var lines int64 = 50 // default value | |||
There was a problem hiding this comment.
The default value of 50 lines doesn't match the configured default of 3 lines defined in the Settings struct. This inconsistency could lead to unexpected behavior when the configuration is not loaded properly.
| var lines int64 = 50 // default value | |
| var lines int64 = 3 // default value, matches Settings struct |
Copilot uses AI. Check for mistakes.
| if len(trimmed) > maxErrorSnippetCharacterLimit { | ||
| trimmed = trimmed[:maxErrorSnippetCharacterLimit] |
There was a problem hiding this comment.
The character truncation could cut off in the middle of a multi-byte UTF-8 character, potentially causing invalid UTF-8 sequences. Consider using a safe string truncation method that respects UTF-8 character boundaries.
| if len(trimmed) > maxErrorSnippetCharacterLimit { | |
| trimmed = trimmed[:maxErrorSnippetCharacterLimit] | |
| runes := []rune(trimmed) | |
| if len(runes) > maxErrorSnippetCharacterLimit { | |
| trimmed = string(runes[:maxErrorSnippetCharacterLimit]) | |
| } else { | |
| trimmed = string(runes) | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Code Review
This pull request introduces a new configuration for error log snippet lines, along with documentation and tests. The changes are mostly good, but I've found a few issues. There's a logic error in pkg/reconciler/status.go where an incorrect default value is used. In the tests, there's a compilation error in a new e2e test and a new, unused helper function in test/pkg/gitea/test.go has a misleading name and buggy logic. Please see my detailed comments.
28aeabe to
1b2caec
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new configuration option, error-log-snippet-number-of-lines, to control the number of lines in error log snippets, along with a character limit to prevent issues with API limits. The changes include updates to the configuration, documentation, and implementation, supported by new unit and end-to-end tests. My review identified a few areas for improvement: a minor logic simplification, using a more standard library function in a test, and correcting a fallback value that appears to be a copy-paste error. Overall, this is a solid contribution that enhances configurability.
1b2caec to
868f8c1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable number of lines for error log snippets and a character limit to prevent issues with the GitHub API. The changes are well-implemented and include documentation updates and e2e tests. My main feedback is to correct the value of the character limit, which is currently off by one, to align with GitHub's documentation.
868f8c1 to
2849bda
Compare
* Added a new configuration option
`error-log-snippet-number-of-lines`. This provides control over the
number of lines displayed in error log snippets.
* Set the default value for `error-log-snippet-number-of-lines` to 3.
* Implemented a maximum character limit of 65536 for all error log
snippets. This addressed potential issues with exceeding the GitHub
Check Run API character limit for logs.
* Removed a previously hardcoded constant for log snippet lines,
replacing it with the new configurable setting.
* Updated the documentation to describe the new configuration and the
character limit.
Jira: https://issues.redhat.com/browse/SRVKP-8165
Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
2849bda to
e4d6b15
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new configuration option error-log-snippet-number-of-lines to control the number of lines in error log snippets, defaulting to 3. It also enforces a 65535 character limit on these snippets to prevent issues with provider API limits, with UTF-8 safe truncation. The changes are well-implemented, including updates to documentation, configuration, and comprehensive unit and end-to-end tests. My feedback includes a suggestion to simplify the log truncation logic for better readability.
| if len(trimmed) > maxErrorSnippetCharacterLimit { | ||
| runes := []rune(trimmed) | ||
| if len(runes) > maxErrorSnippetCharacterLimit { | ||
| trimmed = string(runes[:maxErrorSnippetCharacterLimit]) | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for truncating the log snippet is a bit complex. The outer if statement checking the byte length (len(trimmed)) is a performance optimization, but it makes the code harder to read. Since this is for an error snippet, which is not expected to be excessively large, we can simplify the logic for better readability and maintainability.
A simpler approach would be to directly work with runes, which is safer for UTF-8 strings anyway.
runes := []rune(trimmed)
if len(runes) > maxErrorSnippetCharacterLimit {
trimmed = string(runes[:maxErrorSnippetCharacterLimit])
}
error-log-snippet-number-of-lines. This provides control over thenumber of lines displayed in error log snippets.
error-log-snippet-number-of-linesto 3.snippets. This addressed potential issues with exceeding the GitHub
Check Run API character limit for logs.
replacing it with the new configurable setting.
character limit.
Co-authored-by: claude
Jira: https://issues.redhat.com/browse/SRVKP-8165
📝 Description of the Change
🔗 Linked GitHub Issue
Fixes #
👨🏻 Linked Jira
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
✅ 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.