Skip to content

Conversation

@wegory
Copy link
Contributor

@wegory wegory commented Jan 16, 2023

BLO-143

Default alert platform remains as Discord (backward compatible).

Slack is now added as an option.

The alert_platform Airflow variable will allow developers to choose the platform to receive alerts, i.e., discord or slack.

Developers choosing slack have to further define the following variables:

  • slack_alerts_webhook_url
  • slack_alerts_default_owner
  • slack_alerts_dag_owners [optional]

Related PR: polygon-etl-config

Copy link
Collaborator

@gulshngill gulshngill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Test failed, so we might want to fix that before merging too

f"task_id: {task_id}, log_url: {log_url}, context: {context}",
)

platform = Variable.get(f"alert_platform", "discord")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
platform = Variable.get(f"alert_platform", "discord")
platform = Variable.get("alert_platform", "discord")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Fixed as per suggestion.

I think test fix is going to be a separate task. From the recently closed PRs, we are ignoring the failed test. @gulshngill

@wegory wegory requested a review from gulshngill January 30, 2023 04:54
@wegory wegory removed their assignment Jan 30, 2023
@wegory wegory merged commit 91aaf7d into main Jan 30, 2023
@wegory wegory deleted the feat/slack_alerts branch January 30, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants