-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Feature Notification per Connection #10999
Conversation
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.
Looks like we should have a discussion next week regarding if requiring webhooks is the right move or not
required: | ||
- workspaceId | ||
- name | ||
- webhookUrl |
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.
This webhookUrl
requirement seems to be only a requirement if the notificationType
is slack, or anything else that uses a webhook. For example, it shouldn't be required if we add a notification that utilizes customer.io for emails.
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.
@terencecho what do you think remove webhookUrl
and add a config
which stores the config for Slack or other notification system?
@@ -9,11 +9,8 @@ | |||
import com.google.common.collect.ImmutableMap.Builder; | |||
import io.airbyte.analytics.TrackingClient; | |||
import io.airbyte.commons.map.MoreMaps; | |||
import io.airbyte.config.Notification; | |||
import io.airbyte.config.*; |
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.
nit:
import io.airbyte.config.*; | |
import io.airbyte.config.NotificationLegacy;; |
@@ -14,14 +14,9 @@ | |||
import com.google.common.collect.ImmutableMap; | |||
import com.google.common.collect.ImmutableMap.Builder; | |||
import io.airbyte.analytics.TrackingClient; | |||
import io.airbyte.config.JobConfig; | |||
import io.airbyte.config.*; |
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.
nit:
import io.airbyte.config.*; | |
import io.airbyte.config.NotificationLegacy;; |
type: object | ||
required: | ||
- workspaceId | ||
- name | ||
- webhookUrl |
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.
same comment as above, regarding if we should be requiring webhooks for configs
.set(NOTIFICATION_CONFIG.ID, Notification.getNotificationId()) | ||
.set(NOTIFICATION_CONFIG.WORKSPACE_ID, Notification.getWorkspaceId()) | ||
.set(NOTIFICATION_CONFIG.NAME, Notification.getName()) | ||
.set(NOTIFICATION_CONFIG.WEBHOOK_URL, Notification.getWebhookUrl()) |
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.
if we don't require webhooks, we'd need to add in something more specific similar to the current SlackNotificationConfig
. Not the nicest solution since it embeds another config into this config, but would add flexibility in adding required information for new Notification_types
.withTombstone(false); | ||
} | ||
|
||
private NotificationLegacy generateNotification() { |
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.
what do you think about naming this generateNotificationLegacy
since it's referred to legacy everywhere else. And making generateNotificationNew -> generateNotification
Hi @marcosmarxm, |
What
This PR implements the notification per connection.
Closes #8037
How
Following the TechSpec.
Adding new API endpoints to create/update/remove notification configurations and notification/connection links.
Also changing the current behavior of the workspace notification level to continue working with the new feature.
Current Status
Notification
toNotificationLegacy
(sorry this helps me changing easily the code in the future to adopt new workflow to older one too)JobNotifier
andairbyte/notification
to useNotificationLegacy
.V0_35_50_001__AddNotificationTable.java
the name file I'll update in the future.airbyte-api/src/main/openapi/config.yaml
and all files insideairbyte-config
WorkspaceHandler.java
What is WIP:
WorkspaceHandler
to a newNotificationHandler
to improve readability and domain.JobNotifier
to execute new workflow using the new notification feature.defaultNotification
.馃毃 User Impact 馃毃
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 馃毃馃毃 emoji so changelog authors can further highlight this if needed.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.