Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

Commit

Permalink
Make dag_id required argument to ensure all Slack messages are skipped
Browse files Browse the repository at this point in the history
  • Loading branch information
stacimc committed Sep 2, 2022
1 parent 9342420 commit 5fa08e9
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 9 deletions.
8 changes: 4 additions & 4 deletions openverse_catalog/dags/common/slack.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ def send(self, notification_text: str = "Airflow notification") -> Response:

def send_message(
text: str,
dag_id: str | None = None,
dag_id: str,
username: str = "Airflow",
icon_emoji: str = ":airflow:",
markdown: bool = True,
Expand All @@ -226,7 +226,7 @@ def send_message(
) -> None:
"""Send a simple slack message, convenience message for short/simple messages."""
log.info(text)
if not should_send_message(http_conn_id, dag_id):
if not should_send_message(dag_id, http_conn_id):
return

environment = Variable.get("environment", default_var="dev")
Expand All @@ -241,7 +241,7 @@ def send_message(
s.send(text)


def should_send_message(http_conn_id=SLACK_NOTIFICATIONS_CONN_ID, dag_id=None):
def should_send_message(dag_id, http_conn_id=SLACK_NOTIFICATIONS_CONN_ID):
"""
Returns true if a Slack connection is defined and we are in production (or
the message override is set).
Expand Down Expand Up @@ -271,7 +271,7 @@ def should_send_message(http_conn_id=SLACK_NOTIFICATIONS_CONN_ID, dag_id=None):

def send_alert(
text: str,
dag_id: str | None = None,
dag_id: str,
username: str = "Airflow Alert",
icon_emoji: str = ":airflow:",
markdown: bool = True,
Expand Down
10 changes: 5 additions & 5 deletions tests/dags/common/test_slack.py
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ def test_should_send_message(

def test_should_send_message_is_false_without_hook(http_hook_mock):
http_hook_mock.get_conn.side_effect = AirflowNotFoundException("nope")
assert not should_send_message()
assert not should_send_message(dag_id="test_workflow")


@pytest.mark.parametrize("environment", ["dev", "prod"])
Expand All @@ -342,7 +342,7 @@ def test_send_message(environment, http_hook_mock):
"common.slack.Variable"
) as MockVariable:
MockVariable.get.side_effect = [environment]
send_message("Sample text", username="DifferentUser")
send_message("Sample text", dag_id="test_workflow", username="DifferentUser")
http_hook_mock.run.assert_called_with(
endpoint=None,
data=f'{{"username": "DifferentUser | {environment}", "unfurl_links": true, "unfurl_media": true,'
Expand All @@ -355,16 +355,16 @@ def test_send_message(environment, http_hook_mock):

def test_send_message_does_not_send_if_checks_fail(http_hook_mock):
with mock.patch("common.slack.should_send_message", return_value=False):
send_message("Sample text", username="DifferentUser")
send_message("Sample text", dag_id="test_workflow", username="DifferentUser")
http_hook_mock.run.assert_not_called()


def test_send_alert():
with mock.patch("common.slack.send_message") as send_message_mock:
send_alert("Sample text", username="DifferentUser")
send_alert("Sample text", dag_id="test_workflow", username="DifferentUser")
send_message_mock.assert_called_with(
"Sample text",
None,
"test_workflow",
"DifferentUser",
":airflow:",
True,
Expand Down

0 comments on commit 5fa08e9

Please sign in to comment.