fix: extended detected level for debug and critical level - #18370
Conversation
trevorwhitney
left a comment
trevorwhitney
left a comment
There was a problem hiding this comment.
we've had problems in the past when not including the : in the match. I think we need to keep the : to avoid unexpected matches.
|
Related PR: #17615 |
|
fyi: @trevorwhitney I updated and extended |
trevorwhitney
left a comment
trevorwhitney
left a comment
There was a problem hiding this comment.
approving to unblock as I know a lot of people have been waiting for the bracket functionality, but I'm hesitant about the matches on critical and debug as those will match if they're a substring. I'd prefer explicitly matching on those only if they have whitespace around them in that case, but since there's precedent for other levels doing that we can merge as is and wait to see if anyone files a bug for false positives, as I'm not sure how commonly those will show up as substrings.
| if strings.Contains(log, "CRITICAL:") || strings.Contains(log, "critical:") { | ||
| if strings.Contains(log, "CRITICAL:") || strings.Contains(log, "critical:") || | ||
| strings.Contains(log, "[critical]") || strings.Contains(log, "[CRITICAL]") || | ||
| strings.Contains(log, "critical") || strings.Contains(log, "CRITICAL") { |
There was a problem hiding this comment.
strings like critical without any special characters around them are likely to cause false positives. however, since I see we already have that pattern in here we can merge and wait to see if we get a bug report for false positives, at which point I recommend removing all the ones without special characters, or at least adding whitespace to the matcher.
There was a problem hiding this comment.
Yes, without special characters it might cause false positives especially for critical. Should I remove that lines? even with whitespace could cause false postives.
There was a problem hiding this comment.
my preference would be we only match on critical: and [critical], both lowercase and uppercase variants
There was a problem hiding this comment.
I made the changes for critical and error try to avoid false positive for both.
3 participants
What this PR does / why we need it:
Extend detected level for
debugandcriticallog levelSpecial notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR