Skip to content
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

sync 'error-detection-simple-regexp' default value #1683

Merged

Conversation

jkandasa
Copy link
Member

@jkandasa jkandasa commented May 14, 2024

Changes

Submitter Checklist

  • πŸ“ Please ensure your commit message is clear and informative. For guidance on crafting effective commit messages, refer to the How to write a git commit message guide. We prefer the commit message to be included in the PR body itself rather than a link to an external website (ie: Jira ticket).

  • β™½ Before submitting a PR, run make test lint to avoid unnecessary CI processing. For an even more efficient workflow, consider installing pre-commit and running pre-commit install in the root of this repository.

  • ✨ We use linters to maintain clean and consistent code. Please ensure you've run make lint before submitting a PR. Some linters offer a --fix mode, which can be executed with the command make fix-linters (ensure markdownlint and golangci-lint tools are installed first).

  • πŸ“– If you're introducing a user-facing feature or changing existing behavior, please ensure it's properly documented.

  • πŸ§ͺ While 100% coverage isn't a requirement, we encourage unit tests for any code changes where possible.

  • 🎁 If feasible, please check if an end-to-end test can be added. See README for more details.

  • πŸ”Ž If there's any flakiness in the CI tests, don't necessarily ignore it. It's better to address the issue before merging, or provide a valid reason to bypass it if fixing isn't possible (e.g., token rate limitations).

@jkandasa jkandasa changed the title sync 'error-detection-simple-regexp' and lint on tags sync 'error-detection-simple-regexp' default value and lint on tags May 14, 2024
@savitaashture
Copy link
Member

/test

@chmouel
Copy link
Member

chmouel commented May 14, 2024

image

ErrorLogSnippet bool `default:"true" json:"error-log-snippet"`
ErrorDetection bool `default:"true" json:"error-detection-from-container-logs"`
ErrorDetectionNumberOfLines int `default:"50" json:"error-detection-max-number-of-lines"`
ErrorDetectionSimpleRegexp string `default:"^(?P<filename>[^:]*):(?P<line>[0-9]+):(?P<column>[0-9]+)?([ ]*)?(?P<error>.*)" json:"error-detection-simple-regexp"`
Copy link
Member

Choose a reason for hiding this comment

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

why the change between ? and :

Copy link
Member Author

@jkandasa jkandasa May 14, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

i see yeah, we need to figure out a way to have a CI job to sync or check the two because this becomes a pain

@jkandasa jkandasa force-pushed the sync_error_detection_regexp branch from 9c3270b to 7d67720 Compare May 14, 2024 08:48
Copy link

codecov bot commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 64.41%. Comparing base (16596b4) to head (ef987bb).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1683   +/-   ##
=======================================
  Coverage   64.41%   64.41%           
=======================================
  Files         143      143           
  Lines       11091    11091           
=======================================
  Hits         7144     7144           
  Misses       3421     3421           
  Partials      526      526           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@jkandasa
Copy link
Member Author

jkandasa commented May 14, 2024

@chmouel sorry for the test failures, please ignore. I will test it completely in local and comeback.

Ready

Signed-off-by: Jeeva Kandasamy <jkandasa@redhat.com>
@jkandasa jkandasa force-pushed the sync_error_detection_regexp branch from 7d67720 to ef987bb Compare May 14, 2024 10:59
@jkandasa jkandasa changed the title sync 'error-detection-simple-regexp' default value and lint on tags sync 'error-detection-simple-regexp' default value May 14, 2024
@chmouel
Copy link
Member

chmouel commented May 17, 2024

I am a bit confuse about this commit i'll need to spend more time to test it manually since we don't have e2e for this error reporting (since we can't see the checks output from an API)

@chmouel
Copy link
Member

chmouel commented May 27, 2024

tested as working!

@chmouel chmouel merged commit 6d4be45 into openshift-pipelines:main May 27, 2024
3 checks passed
@jkandasa jkandasa deleted the sync_error_detection_regexp branch May 27, 2024 17:02
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.

None yet

3 participants