-
Notifications
You must be signed in to change notification settings - Fork 132
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 Alert .spec.eventMetadata behavior #529
Conversation
03b1c9a
to
7491394
Compare
Does the CI run all the tests? Also, how can I change the log to warning rather than an error? |
Doesn't fail locally for me.
CI just runs
I don't think we have a warning level. The logger we use prefers verbosity levels over named levels. We do have some custom wrapper for warning level log somewhere, but that doesn't work well with the underlying zap logging levels and results in weird logs. |
I see. Should I keep it as is then? (error log) |
Looking at the existing usage of error log, I'm more inclined towards an info log as it'd be beneficial if it's readily visible, without the need to change debug levels. |
bbf82c7
to
684113f
Compare
@stefanprodan @darkowlzz we should probably also merge this for rc4? |
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 good to me, but requires a rebase.
Signed-off-by: Matheus Pimenta <matheuscscp@gmail.com>
684113f
to
8c11d8a
Compare
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
Thanks @matheuscscp
Hi @darkowlzz, @hiddeco and @somtochiama, this fixes the behavior we discussed and updates the documentation. Still need to write tests, but please feel free to give an initial pass 👌
Follow up on #519 (comment), and discussed in #528 (review)