-
Notifications
You must be signed in to change notification settings - Fork 289
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
Kubernetes source plugin #985
Kubernetes source plugin #985
Conversation
4533ae7
to
7ef0031
Compare
8ac2cd9
to
867b9dd
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.
I still need to check 5 more files:
- internal/source/dispatcher.go
- internal/source/kubernetes/registration.go
- internal/source/kubernetes/router.go
- internal/source/kubernetes/source.go
- internal/source/scheduler.go
And run some manual tests.
You need to move botkube/helm/botkube/values.yaml Lines 411 to 420 in 09e4a26
|
As a part of this PR, please also document all breaking under: #972. Thanks! 🙇 |
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.
Great work! Looks like the basic functionality works OK, I tested a few configurations and they worked well. Telemetry also OK 👍
I agree with all @mszostok comments, and here are just a few ones from my side 🙂
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.
Overall good work 🚀
Once all comments will be resolved I will play with it locally as Paweł did.
Please also track all work that you would like to do in the follow-ups. Thanks!
LGTM with minor comments 🚀
I addressed all the feedbacks. |
df8ed4d
to
56bd5d3
Compare
types: | ||
- create | ||
- delete | ||
- error |
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.
NOTE: after this merging this PR we won't merge configuration anymore. As a result, enabling both k8s-err-events
and 'k8s-all-events' results in duplicated error
messages. Probably will be good to mention that in the release notes/docs.
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.
Ok, I will be stating that in both release docs and main doc
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 👍 but before merge please address my last comments, and this one too: #985 (comment)
Also please wait for @pkosiec feedback and approve. Thanks!
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.
🚀 Awesome work!
Addressed all the feedbacks, will check locally and focus on docs changes. |
Description
Changes proposed in this pull request:
Testing
Build Kubernetes Plugin
Following command will build kubernetes plugin
Run Plugin Server
Run Botkube With the Following Configurations
and then update configmap to see diff is shown in slack
Related issue(s)
Relates #840