Add plugin manager for sources #857
Conversation
mszostok
force-pushed
the
plugin-mgmt
branch
3 times, most recently
from
3ad94b1 to
4695584
Compare
mszostok
changed the title
mszostok
force-pushed
the
plugin-mgmt
branch
2 times, most recently
from
d1a7f8a to
aefe8fb
Compare
mszostok
changed the title
mszostok
force-pushed
the
plugin-mgmt
branch
from
aefe8fb to
0da015a
Compare
| @@ -1,4 +1,4 @@ | |||
| package e2e | |||
| package fake | |||
There was a problem hiding this comment.
I cannot put the fake pkg inside e2e as then logs streaming for test doesn't work because of this issue: golang/go#46959
mszostok
force-pushed
the
plugin-mgmt
branch
from
9bf7ab8 to
92da379
Compare
mszostok
force-pushed
the
plugin-mgmt
branch
from
92da379 to
fb20fdc
Compare
| > **Note** | ||
| > If Botkube runs inside the k3d cluster, export the `PLUGIN_SERVER_HOST=http://host.k3d.internal` environment variable. | ||
|
|
There was a problem hiding this comment.
| > **Note** | |
| > If Botkube runs inside the k3d cluster, export the `PLUGIN_SERVER_HOST=http://host.k3d.internal` environment variable. | |
| > **Note** | |
| > If Botkube runs inside the k3d cluster, export the `PLUGIN_SERVER_HOST=http://host.k3d.internal` environment variable. | |
| ) | ||
|
|
||
| // CMWatcher implements Botkube source plugin. | ||
| type CMWatcher struct{} |
There was a problem hiding this comment.
Code style consistency - type block above combined with single line declaration.
There was a problem hiding this comment.
Actually, I use the type block to "group" related entities, so the code block above is about types related to the configuration. This one is about the "service" that implements the source plugin functionality, so it's close to its methods.
Are you OK with such approach?
internal/source/source.go
Outdated
| // As a result our key is e.g. ['source-name1;source-name2'] | ||
| startProcesses := map[string]struct{}{} | ||
|
|
||
| startPlugin := func(bindSources []string) error { |
There was a problem hiding this comment.
Isn't this function complex enough to be named / defined separately?
There was a problem hiding this comment.
yes, you are right. I changed it to become a dedicated method on this struct 👍
In the next PR, I will change it even more to make it more straight forward when it comes to the responsibilities. Because I see that it is to complex to maintain and test all those pieces together.
|
Hi @josefkarasek! Thanks for your review 🙇 |
Description
Changes proposed in this pull request:
Related issue(s)