-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
New Feature Notification per Connection #10999
Changes from all commits
dca7b63
c857837
94c142a
bc56777
33da4b9
a776223
3a49e4e
8caf1fe
c43464d
994379b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,21 +2,32 @@ | |
"$schema": http://json-schema.org/draft-07/schema# | ||
"$id": https://github.com/airbytehq/airbyte/blob/master/airbyte-config/models/src/main/resources/types/Notification.yaml | ||
title: Notification | ||
description: Notification Settings | ||
description: notification configuration | ||
type: object | ||
required: | ||
- workspaceId | ||
- name | ||
- webhookUrl | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
- notificationType | ||
additionalProperties: true | ||
additionalProperties: false | ||
properties: | ||
# Instead of this type field, we would prefer a json schema "oneOf" but unfortunately, | ||
# the jsonschema2pojo does not seem to support it yet: https://github.com/joelittlejohn/jsonschema2pojo/issues/392 | ||
workspaceId: | ||
type: string | ||
format: uuid | ||
notificationId: | ||
type: string | ||
format: uuid | ||
name: | ||
type: string | ||
webhookUrl: | ||
type: string | ||
notificationType: | ||
"$ref": NotificationType.yaml | ||
sendOnSuccess: | ||
defaultNotification: | ||
type: boolean | ||
default: false | ||
sendOnFailure: | ||
description: if set true the webhook will be triggered to all connections | ||
tombstone: | ||
description: | ||
if not set or false, the configuration is active. if true, then this | ||
configuration is permanently off. | ||
type: boolean | ||
default: true | ||
slackConfiguration: | ||
"$ref": SlackNotificationConfiguration.yaml |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
--- | ||
"$schema": http://json-schema.org/draft-07/schema# | ||
"$id": https://github.com/airbytehq/airbyte/blob/master/airbyte-config/models/src/main/resources/types/NotificationConnection.yaml | ||
title: NotificationConnection | ||
description: notification connection link | ||
type: object | ||
required: | ||
- workspaceId | ||
- notificationId | ||
- sendOnSuccess | ||
- sendOnFailure | ||
- tombstone | ||
additionalProperties: false | ||
properties: | ||
notificationConnectionId: | ||
type: string | ||
format: uuid | ||
workspaceId: | ||
type: string | ||
format: uuid | ||
notificationId: | ||
type: string | ||
format: uuid | ||
connectionId: | ||
type: string | ||
format: uuid | ||
sendOnSuccess: | ||
type: boolean | ||
sendOnFailure: | ||
type: boolean | ||
tombstone: | ||
description: | ||
if not set or false, the configuration is active. if true, then this | ||
configuration is permanently off. | ||
type: boolean |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
--- | ||
"$schema": http://json-schema.org/draft-07/schema# | ||
"$id": https://github.com/airbytehq/airbyte/blob/master/airbyte-config/models/src/main/resources/types/NotificationLegacy.yaml | ||
title: NotificationLegacy | ||
description: Notification Settings | ||
type: object | ||
required: | ||
- notificationType | ||
additionalProperties: true | ||
properties: | ||
# Instead of this type field, we would prefer a json schema "oneOf" but unfortunately, | ||
# the jsonschema2pojo does not seem to support it yet: https://github.com/joelittlejohn/jsonschema2pojo/issues/392 | ||
notificationType: | ||
"$ref": NotificationType.yaml | ||
sendOnSuccess: | ||
type: boolean | ||
default: false | ||
sendOnFailure: | ||
type: boolean | ||
default: true | ||
slackConfiguration: | ||
"$ref": SlackNotificationConfiguration.yaml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,8 @@ | |
import static io.airbyte.db.instance.configs.jooq.Tables.ACTOR_OAUTH_PARAMETER; | ||
import static io.airbyte.db.instance.configs.jooq.Tables.CONNECTION; | ||
import static io.airbyte.db.instance.configs.jooq.Tables.CONNECTION_OPERATION; | ||
import static io.airbyte.db.instance.configs.jooq.Tables.NOTIFICATION_CONFIG; | ||
import static io.airbyte.db.instance.configs.jooq.Tables.NOTIFICATION_CONNECTION; | ||
import static io.airbyte.db.instance.configs.jooq.Tables.OPERATION; | ||
import static io.airbyte.db.instance.configs.jooq.Tables.STATE; | ||
import static io.airbyte.db.instance.configs.jooq.Tables.WORKSPACE; | ||
|
@@ -33,6 +35,8 @@ | |
import io.airbyte.config.ConfigWithMetadata; | ||
import io.airbyte.config.DestinationConnection; | ||
import io.airbyte.config.DestinationOAuthParameter; | ||
import io.airbyte.config.Notification; | ||
import io.airbyte.config.NotificationConnection; | ||
import io.airbyte.config.OperatorDbt; | ||
import io.airbyte.config.OperatorNormalization; | ||
import io.airbyte.config.SourceConnection; | ||
|
@@ -762,6 +766,48 @@ private void writeStandardWorkspace(final List<StandardWorkspace> configs) throw | |
}); | ||
} | ||
|
||
private void writeNotification(final List<Notification> configs) throws IOException { | ||
database.transaction(ctx -> { | ||
writeNotification(configs, ctx); | ||
return null; | ||
}); | ||
} | ||
|
||
private void writeNotificationConnection(final List<NotificationConnection> configs) throws IOException { | ||
database.transaction(ctx -> { | ||
writeNotificationConnection(configs, ctx); | ||
return null; | ||
}); | ||
} | ||
|
||
private void writeNotification(final List<Notification> configs, final DSLContext ctx) { | ||
configs.forEach((Notification) -> { | ||
ctx.insertInto(NOTIFICATION_CONFIG) | ||
.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 commentThe 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 |
||
.set(NOTIFICATION_CONFIG.DEFAULT_NOTIFICATION, Notification.getDefaultNotification()) | ||
.set(NOTIFICATION_CONFIG.NOTIFICATION_TYPE, Notification.getNotificationType().value()) | ||
.set(NOTIFICATION_CONFIG.TOMBSTONE, Notification.getTombstone()) | ||
.execute(); | ||
}); | ||
} | ||
|
||
private void writeNotificationConnection(final List<NotificationConnection> configs, final DSLContext ctx) { | ||
configs.forEach((NotificationConnection) -> { | ||
ctx.insertInto(NOTIFICATION_CONNECTION) | ||
.set(NOTIFICATION_CONNECTION.NOTIFICATION_CONNECTION_ID, NotificationConnection.getNotificationConnectionId()) | ||
.set(NOTIFICATION_CONNECTION.WORKSPACE_ID, NotificationConnection.getWorkspaceId()) | ||
.set(NOTIFICATION_CONNECTION.NOTIFICATION_ID, NotificationConnection.getNotificationId()) | ||
.set(NOTIFICATION_CONNECTION.CONNECTION_ID, NotificationConnection.getConnectionId()) | ||
.set(NOTIFICATION_CONNECTION.SEND_ON_SUCCESS, NotificationConnection.getSendOnSuccess()) | ||
.set(NOTIFICATION_CONNECTION.SEND_ON_FAILURE, NotificationConnection.getSendOnFailure()) | ||
.set(NOTIFICATION_CONNECTION.TOMBSTONE, NotificationConnection.getTombstone()) | ||
.execute(); | ||
}); | ||
} | ||
|
||
private void writeStandardWorkspace(final List<StandardWorkspace> configs, final DSLContext ctx) { | ||
final OffsetDateTime timestamp = OffsetDateTime.now(); | ||
configs.forEach((standardWorkspace) -> { | ||
|
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 thenotificationType
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 aconfig
which stores the config for Slack or other notification system?