feat: implement slack notifier for demo organization creation event - #2041
Conversation
d3ea6aa to
2c2fcb9
Compare
kakcy
left a comment
kakcy
left a comment
There was a problem hiding this comment.
I made a few comments, please check them.
| defer cancel() | ||
|
|
||
| if domainEvent.Type != domainevent.Event_DEMO_ORGANIZATION_CREATED { | ||
| msg.Ack() |
There was a problem hiding this comment.
In this case, is there no need for logs or subscriberHandledCounter.Inc()?
There was a problem hiding this comment.
Yeah, I think we should ignore events that aren't relevant
There was a problem hiding this comment.
I also think that unrelated events should be ignored.
In that case, isn't it necessary to collect logs or call subscriberHandledCounter.Inc()?
When I check the code for other errors or irregular cases, I see that logs are collected and subscriberHandledCounter.Inc() is called.
https://github.com/bucketeer-io/bucketeer/blob/notify-demo-org-create/pkg/subscriber/processor/demo_organization_creation_notifier.go#L107-L116
There was a problem hiding this comment.
Oh that error case means that something already went wrong with the message and we don't know if that message is relevant or not
There was a problem hiding this comment.
The subscriberHandledCounter.Inc() should be called whenever the processing fails or not.
It means that the processor tried to handle the messages.
We also have another metric that reports what happened.
E.g.
It could OK, MissingID, etc
subscriberHandledCounter.WithLabelValues(subscriberAuditLog, codes.MissingID.String()).Inc()
There was a problem hiding this comment.
Got it, I've added the subscriberHandledCounter.Inc() for this
There was a problem hiding this comment.
Pull Request Overview
This PR implements a Slack notifier for demo organization creation events to resolve issue #2040. It adds support for notifying via Slack webhook when a demo organization is created, including protobuf message definitions, event processing, and configuration updates.
- Adds new notification type for demo organization creation events
- Implements complete Slack notification flow from event processing to message delivery
- Updates protobuf definitions across JavaScript and Go generated files
Reviewed Changes
Copilot reviewed 15 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ui/web-v2/src/proto/notification/subscription_pb.js | Adds DOMAIN_EVENT_DEMO_ORGANIZATION source type |
| ui/web-v2/src/proto/notification/sender/notification_pb.js | Adds DemoOrganizationCreationNotification message type and methods |
| proto/notification/subscription.proto | Defines DOMAIN_EVENT_DEMO_ORGANIZATION enum value |
| proto/notification/subscription.pb.go | Generated Go code for subscription proto changes |
| proto/notification/sender/notification.proto | Defines DemoOrganizationCreationNotification message structure |
| proto/notification/sender/notification.pb.go | Generated Go code for notification proto changes |
| pkg/subscriber/processor/demo_organization_creation_notifier.go | New processor for handling demo organization creation events |
| pkg/subscriber/processor/processors.go | Registers new demo organization notifier processor |
| pkg/subscriber/processor/metrics.go | Adds metrics constant for demo organization events |
| pkg/subscriber/cmd/server/server.go | Integrates demo organization notifier into server |
| pkg/notification/sender/notifier/slack.go | Implements Slack message formatting for demo organization notifications |
| manifests/bucketeer/values.dev.yaml | Development configuration for demo organization notifier |
| manifests/bucketeer/charts/subscriber/values.yaml | Helm chart configuration template |
| api-description/web-api.swagger.yaml | Updates API documentation with new source type |
| api-description/apidocs.swagger.yaml | Updates API documentation with new source type |
Comments suppressed due to low confidence (2)
manifests/bucketeer/values.dev.yaml:516
- The configuration key 'slackWebhookURL' is inconsistent with the struct field name 'WebHookURL' used in the Go code. This mismatch could cause configuration parsing issues.
slackWebhookURL: ""
manifests/bucketeer/charts/subscriber/values.yaml:258
- The configuration key 'webhookURL' is inconsistent with both the struct field name 'WebHookURL' and the dev configuration 'slackWebhookURL'. This naming inconsistency could cause configuration parsing issues.
webhookURL: ""
| logger.Error("demoOrganizationCreationNotifier: failed to unmarshal config", zap.Error(err)) | ||
| return nil | ||
| } | ||
| slackNotifier := notifier.NewSlackNotifier(notifierConfig.WebEndpoint) |
There was a problem hiding this comment.
The SlackNotifier constructor is called with WebEndpoint but based on the context, it should be called with the webhook URL from the Slack configuration. The current code passes the wrong parameter - it should use notifierConfig.Notifier.Slack.WebHookURL instead of notifierConfig.WebEndpoint.
| slackNotifier := notifier.NewSlackNotifier(notifierConfig.WebEndpoint) | |
| slackNotifier := notifier.NewSlackNotifier(notifierConfig.Notifier.Slack.WebHookURL) |
|
LGTM！ |
github-project-automation
Bot
moved this from In Progress
to Weekly done
in Bucketeer Tasks
To resolve #2040