-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Filebeat] fix minor unreachable code #32659
Conversation
💚 CLA has been signed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syslog change LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Abirdcfly Please add an error check to the os.MkdirAll call. Otherwise LGTM
Signed-off-by: Abirdcfly <fp544037857@gmail.com>
Done for make golangci-lint happy. |
@@ -793,7 +793,6 @@ func TestPriority(t *testing.T) { | |||
ParserRFC3164([]byte(log), l) | |||
assert.Equal(t, d, l.Priority()) | |||
}) | |||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks suspicious and it changes how the test ran. Before it was running only once and now for 120 iterations. I think it's safer to remove the loop in order to preserve the existing logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks suspicious and it changes how the test run. Before it was running only once and now for 120 iterations.
Yes. IMO, that was the intent of this test, only the return
statement was added by mistake
I think it's safer to remove the loop in order to preserve the existing logic.
I'm keeping an open mind on this.
I'll wait a few hours, and if no other revier has a problem with it, I'll delete the loop.
@efd6 @ShourieG @cmacknz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running the loop only once is not correct IMO. The point of the test is to check that a variety of syslog priorities are properly parsed into the destination value. Testing only a single value (1) is not interesting. I'm not entirely sure why it starts at 1 and stops at 120 since it would not cost significantly more to test the exhaustive range of valid priority values.
This pull request does not have a backport label.
|
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
@cmacknz Are you able to take a look at this? |
Didn't realize I was holding this up. Approved, and merged. Thanks! |
Signed-off-by: Abirdcfly <fp544037857@gmail.com>
Signed-off-by: Abirdcfly fp544037857@gmail.com
What does this PR do?
Why is it important?
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs