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

Fix Alert .spec.eventMetadata behavior #529

Merged
merged 1 commit into from
May 24, 2023

Conversation

matheuscscp
Copy link
Collaborator

@matheuscscp matheuscscp commented May 18, 2023

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)

@matheuscscp matheuscscp force-pushed the fix-alert-metadata branch 7 times, most recently from 03b1c9a to 7491394 Compare May 18, 2023 15:41
@matheuscscp matheuscscp marked this pull request as ready for review May 18, 2023 19:11
@matheuscscp
Copy link
Collaborator Author

make test is failing locally for me for some reason but I see:

--- PASS: TestAlertReconciler_EventHandler (10.68s)

Does the CI run all the tests?

Also, how can I change the log to warning rather than an error?

internal/server/event_handlers.go Outdated Show resolved Hide resolved
internal/controllers/alert_controller_test.go Outdated Show resolved Hide resolved
@darkowlzz
Copy link
Contributor

make test is failing locally for me for some reason but I see:

Doesn't fail locally for me.

Does the CI run all the tests?

CI just runs make test, refer

.

Also, how can I change the log to warning rather than an error?

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.

@matheuscscp
Copy link
Collaborator Author

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)

@darkowlzz
Copy link
Contributor

darkowlzz commented May 19, 2023

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.

@matheuscscp matheuscscp force-pushed the fix-alert-metadata branch 7 times, most recently from bbf82c7 to 684113f Compare May 19, 2023 21:41
@matheuscscp
Copy link
Collaborator Author

@stefanprodan @darkowlzz we should probably also merge this for rc4?

Copy link
Member

@hiddeco hiddeco left a 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.

@hiddeco hiddeco added the bug Something isn't working label May 24, 2023
Signed-off-by: Matheus Pimenta <matheuscscp@gmail.com>
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @matheuscscp

@stefanprodan stefanprodan merged commit 639d66d into fluxcd:main May 24, 2023
@matheuscscp matheuscscp deleted the fix-alert-metadata branch May 24, 2023 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants