-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
fix monitor to only start the monitor in json format when requested #10358
Conversation
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.
LGTM!
case ret := <-exitCode: | ||
if ret != 0 { |
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.
There are two goroutines sending to this chan, but this only checks the return value once, which seems like it could easily pass even when one of the goroutines fails.
Instead of running the assertion in a goroutine, could we use a bufio.Scanner
in the main thread? We could then read lines, and assert some number of log lines is json encoded. Once it successfully we can shutdown the agent and goroutines.
Maybe we'll need to run another goroutine to perform a couple operations to ensure that the agent is producing logs.
|
||
// Read the logs and try to json marshall it | ||
go func() { | ||
time.Sleep(1 * time.Second) |
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.
Generally I try to avoid any sleeps in a test, and 1 second is a pretty long sleep. I think with the suggestion below we can remove this sleep.
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/381703. |
🍒✅ Cherry pick of commit dda3e68 onto |
…10358) * fix monitor to only start the monitor in json format when requested * add release notes * add test to validate json format when requested
…10358) * fix monitor to only start the monitor in json format when requested * add release notes * add test to validate json format when requested
🍒✅ Cherry pick of commit dda3e68 onto |
🍒✅ Cherry pick of commit dda3e68 onto |
…10358) * fix monitor to only start the monitor in json format when requested * add release notes * add test to validate json format when requested
when starting a new monitor we were starting (in case of json monitor) a json monitor and then overwrite it with a line monitor.
This will start one or the other.
Fix #10357