-
Notifications
You must be signed in to change notification settings - Fork 135
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
Refactor events and introduce v1beta3 API for Alert and Provider #540
Conversation
47cda90
to
6bec2c2
Compare
f650b00
to
ec0e793
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.
first pass LGTM
f85c4d1
to
ae9a55d
Compare
ae9a55d
to
3b77966
Compare
3b77966
to
9562888
Compare
These changes depended on the custom metrics that was release in Flux v2.1.0 for the static Alert and Provider metrics. It's no longer blocked now. I've rebased and tried my best to go through all the recent changes to make sure I haven't missed anything that was added as these changes completely change the alert and provider related code. But still, it'd be great if others look out for things I may have missed. I'll also continue looking for anything missing. As it has been a few months since these changes were written, I'll be testing more and reevaluating if everything is working as they are supposed to be. Marking the PR ready for review. ❔ ❓ I need some help in deciding what to do with the deprecated |
@darkowlzz I'm for dropping |
We had a discussion about this with all the maintainers and it was decided to keep this support for now and drop it in v1 API of the Provider. I'll add a TODO in the code to remove it in v1. |
9562888
to
ee3b54f
Compare
a2d4a39
to
9037fdd
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.
Please mark the v1beta2 APIs as deprecated.
From docs:
A beta version API becomes deprecated once a subsequent beta or stable API version is released. A deprecated beta version is subject to removal after a six-months period.
2c97a14
to
cc25bd2
Compare
Marked both v1beta1 and v1beta2 Alert and Provider APIs as deprecated. |
cc25bd2
to
d194886
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 @darkowlzz 🥇
PS. Please create a PR in website to move the API docs to v1beta3
and update all docs to the new version.
@darkowlzz please integrate #639 and make the correction in docs to
|
d194886
to
7ea5bea
Compare
Pushed a fix for log in the new event handler. A post request failure was logged without any context before: {
"level": "error",
"ts": "2023-11-21T16:50:24.582Z",
"msg": "failed to send notification",
"error": "postMessage failed: request failed with status code 403, invalid_token"
} This was because of using the wrong context in the logger. Using the proper context fixes it with rich contextual log: {
"level": "error",
"ts": "2023-11-21T16:58:07.483Z",
"logger": "event-server",
"msg": "failed to send notification",
"eventInvolvedObject": {
"kind": "HelmRepository",
"namespace": "default",
"name": "podinfo",
"uid": "4892228c-eed5-4dc7-8969-94e51d0df5e7",
"apiVersion": "source.toolkit.fluxcd.io/v1beta2",
"resourceVersion": "991119"
},
"Alert": {
"name": "default-alerts",
"namespace": "default"
},
"error": "postMessage failed: request failed with status code 403, invalid_token"
} |
Rebased and updated the v1beta3 docs with Bitbucket server changes and correction. |
dd47b7a
to
7c1b417
Compare
v1beta3 API for Alert and Provider makes them static objects, removing the status subresource and spec fields that are relevant to dynamic objects with reconcilers. Signed-off-by: Sunny <darkowlzz@protonmail.com>
In v1beta3 API, Alert and Provider are static objects and don't need reconcilers. Signed-off-by: Sunny <darkowlzz@protonmail.com>
- Break down the EventServer.handleEvent() implementation into multiple smaller functions which are extensively tested on their own. - New implementation of filter Alerts for Event - New implementation of Event matches Alert - Remove any readiness check on Alert or Provider. - Add kubebuilder marker for generating RBAC permissions to create and patch events, and query Alert and Provider objects. - Convert the event handler test from controllers/ dir to work with just EventServer without any reconciler, keeping all the test cases and slightly modified test set up code. Signed-off-by: Sunny <darkowlzz@protonmail.com>
Emit events in the event handler along with logs on the respective alert to make the message visible on the alert it belongs to. Signed-off-by: Sunny <darkowlzz@protonmail.com>
Add new Alert and Provider reconcilers to perform migration to static objects. The new Alert and Provider APIs don't contain any status. When the existing Alerts and Providers are queries using the new API client, the status would be dropped. A subsequent write of the object to update the object in api-server will migrate the objects to the new version and drop the status. For the stale finalizers on the objects, the new reconcilers ensure that the finalizers get removed. Signed-off-by: Sunny <darkowlzz@protonmail.com>
The new alert and provider don't have any status. Remove any status related checks from workflows/e2e tests. Signed-off-by: Sunny <darkowlzz@protonmail.com>
Signed-off-by: Sunny <darkowlzz@protonmail.com>
Signed-off-by: Sunny <darkowlzz@protonmail.com>
Use the context containing proper information about the event for logging. Previously, the logged error didn't contain any information about the event, alert or the involved object. Signed-off-by: Sunny <darkowlzz@protonmail.com>
7c1b417
to
6df2c74
Compare
Refer #626 for more details.
Based on a lot of observations from v1beta2 API discussion in #435, it became apparent that Alert and Provider can be static data objects without reconcilers and status subresource.
These changes are based on the experiments from last November, refer https://github.com/fluxcd/notification-controller/commits/refactor-event-handler, updated to include all the latest changes.