feature(providers): added `OpsgenieNotifier` #35530

Eric-Mendes · 2023-11-08T15:02:01Z

Added an OpsgenieNotifier.

It is almost the exact same implementation as OpsgenieCreateAlertOperator, but given that now notifiers exist, I assumed it was better off as a notifier.

Sample usage:

    from datetime import datetime
    from airflow import DAG
    from airflow.operators.bash import BashOperator
    from airflow.providers.opsgenie.notifications.opsgenie import send_opsgenie_notification

    with DAG(
        "opsgenie_notifier",
        start_date=datetime(2023, 1, 1),
        on_failure_callback=[
            send_opsgenie_notification(
                payload={"message": "Something went wrong!"}
            )
        ],
    ):
        BashOperator(
            task_id="mytask",
            bash_command="fail",
            on_failure_callback=[
                send_opsgenie_notification(
                    payload={"message": "Something went wrong!"}
                )
            ],
        )

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

boring-cyborg · 2023-11-08T15:02:06Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

Taragolis · 2023-11-08T17:32:41Z

airflow/providers/opsgenie/notifications/opsgenie.py

+        alias: str | None = None,
+        description: str | None = None,
+        responders: list[dict] | None = None,
+        visible_to: list[dict] | None = None,
+        actions: list[str] | None = None,
+        tags: list[str] | None = None,
+        details: dict | None = None,
+        entity: str | None = None,
+        source: str | None = None,
+        priority: str | None = None,
+        user: str | None = None,
+        note: str | None = None,


I'm not sure maybe from user perspective better to have single payload: dict | None = None attribute rather than multiple attributes which later on combined into the single one payload?

Just a question, I'm never use Opsgenie Provider.

Just a question, I'm never use Opsgenie Provider.

For now, me neither, but I'll be a user soon, so that's why I have decided to open this PR.
From a software engineering perspective I feel that this approach works best because it's easier to see what's optional and what isn't, and also the types of each parameter in case you forget. It also makes its usage nearly identical to the OpsgenieCreateAlertOperator, so there is no learning curve if someone wants to swap it for this notifier. And besides that there is always dict unpacking if someone already has a payload and wants to use it in this notifier.
I'm open to suggestions, though!

Building on what @Taragolis said, just one downside, you always have to keep up with the Opsgenie service. In the event that one of the parameters gets deprecated, someone has to make the change here and you would always have to keep up with the changes.

How do you propose I deal with the required params following the payload: dict | None = None approach, @utkarsharma2?

If we wont have some hints (optional) we could create TypedDict, and use it as payload type something like payload: CreateAlertPayload

from typing import TypedDict from typing_extensions import NotRequired # For compat with Python < 3.11 class CreateAlertPayload(TypedDict): message: str alias: NotRequired[str] description: NotRequired[None] responders: NotRequired[list[dict]] visible_to: NotRequired[list[dict]] actions: NotRequired[list[str]] tags: NotRequired[list[str]] details: NotRequired[dict] entity: NotRequired[str] source: NotRequired[str] priority: NotRequired[str] user: NotRequired[str] note: NotRequired[str]

@Taragolis also, how do we make sure that the template_fields will be able to be templated?

Where do you think it could go? I was thinking a common.py file inside the provider so that it could be reused by the OpsgenieCreateAlertOperator, but I'm open to suggestions.

Maybe something opsgenie_typing.py and place CreateAlertPayload there, just suggestion just because: There are only two hard things in Computer Science: cache invalidation and naming things.

How do we deal with the opsgenie_conn_id?

Connection id usually (always?) is the separate parameter. so keep it as it is.

Make payload as single dictionary could be better by the reason which @utkarsharma2 mentioned.

how do we make sure that the template_fields will be able to be templated?

If you put payload attribute (or whatever it named) as part of template_fields then it would be templated. e.g.

send_opsgenie_notification( payload={ "message": "Something went wrong in {{ dag.dag_id }}!" "user": "{{var.value.user }}" )

Won't making payload a template_field also make the iterable fields templated? Would that be ok?

Done adding the payload arg.

As you can see from the sad path, it's been hard to ensure the schema for payload with TypedDict. Is it the only way to go, @Taragolis?

Taragolis · 2023-11-08T18:21:52Z

Need to fix Static Checks, you could run locally breeze static-checks {id-of-failed-check} --all-files e.g. breeze static-checks ruff --all-files or you could run it thought pre-commit e.g. pre-commit run ruff-format --all-files

You could have a look what failed in Tests / Static checks (pull_request) logs

Also need to add into the Guides toctree this line

Notifications <notifications/index>

Under the Operators

airflow/docs/apache-airflow-providers-opsgenie/index.rst

Lines 32 to 37 in 22d5834

    
           .. toctree:: 
        
               :hidden: 
        
               :maxdepth: 1 
        
               :caption: Guides 
        
               Operators <operators/index>

docs/apache-airflow-providers-opsgenie/notifications/opsgenie_notifier.rst

airflow/providers/opsgenie/notifications/opsgenie.py

utkarsharma2 · 2023-11-09T14:28:48Z

airflow/providers/opsgenie/notifications/opsgenie.py

+        alias: str | None = None,
+        description: str | None = None,
+        responders: list[dict] | None = None,
+        visible_to: list[dict] | None = None,
+        actions: list[str] | None = None,
+        tags: list[str] | None = None,
+        details: dict | None = None,
+        entity: str | None = None,
+        source: str | None = None,
+        priority: str | None = None,
+        user: str | None = None,
+        note: str | None = None,


Building on what @Taragolis said, just one downside, you always have to keep up with the Opsgenie service. In the event that one of the parameters gets deprecated, someone has to make the change here and you would always have to keep up with the changes.

Co-authored-by: Utkarsh Sharma <utkarsharma2@gmail.com>

Taragolis

Need to fix static checks and documentation.

Better use pre-commit which save a lot of time, because when it run some of static checks might fixed during checks.

tests/system/providers/opsgenie/example_opsgenie_notifier.py

docs/apache-airflow-providers-opsgenie/notifications/opsgenie_notifier.rst

docs/apache-airflow-providers-opsgenie/notifications/index.rst

Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

…notifier.rst Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

docs/apache-airflow-providers-opsgenie/notifications/opsgenie_notifier.rst

Taragolis · 2023-11-17T10:43:38Z

tests/providers/opsgenie/typing/test_opsgenie.py

+        with pytest.raises(Exception):  # should raise exception because message is a required key
+            notifier = OpsgenieNotifier(payload=config_no_message)
+            notifier({"dag": dag})
+            mock_opsgenie_alert_hook.return_value.create_alert.assert_called_once_with(
+                expected_payload_dict_no_message
+            )


Well it would not raise any error because you mock OpsgenieAlertHook.get_conn

What we actually want to test here?

If payload really requires message at all times, as it should. Look at my inline comment there
should raise exception because message is a required key

But this behaviour depend on upstream library which you mocks, so this error never happen in tests

As result tests failed

My point here that we should not tests upstream library exceptions, because it could changed in the future.

…notifier.rst Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

boring-cyborg · 2023-11-20T20:49:30Z

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

--------- Co-authored-by: Utkarsh Sharma <utkarsharma2@gmail.com> Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

Eric-Mendes added 2 commits November 7, 2023 18:36

feature(providers): added OpsgenieNotifier

5738d65

added documentation and tests

6acbd6b

boring-cyborg bot added area:providers kind:documentation provider:opsgenie labels Nov 8, 2023

Taragolis reviewed Nov 8, 2023

View reviewed changes

utkarsharma2 reviewed Nov 9, 2023

View reviewed changes

Eric-Mendes and others added 2 commits November 9, 2023 12:03

Update airflow/providers/opsgenie/notifications/opsgenie.py

c7a7014

Co-authored-by: Utkarsh Sharma <utkarsharma2@gmail.com>

fixed static checks errors

047d529

Taragolis reviewed Nov 9, 2023

View reviewed changes

Eric-Mendes and others added 8 commits November 9, 2023 17:20

Update tests/system/providers/opsgenie/example_opsgenie_notifier.py

615cad3

Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

Update docs/apache-airflow-providers-opsgenie/notifications/opsgenie_…

d8efba1

…notifier.rst Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

Update docs/apache-airflow-providers-opsgenie/notifications/index.rst

76e71f6

Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

Update docs/apache-airflow-providers-opsgenie/notifications/index.rst

47189a5

Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

Update tests/system/providers/opsgenie/example_opsgenie_notifier.py

957fd28

Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

fixed static checks

df76d1c

payload arg

1a7dc55

fixed tests

6d386e6

Eric-Mendes requested a review from Taragolis November 13, 2023 20:04

added typing tests

62ba758

Taragolis reviewed Nov 17, 2023

View reviewed changes

Eric-Mendes and others added 2 commits November 17, 2023 14:02

Update docs/apache-airflow-providers-opsgenie/notifications/opsgenie_…

78fc221

…notifier.rst Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

removed tool-versions

b00a95e

Eric-Mendes requested a review from Taragolis November 19, 2023 16:53

Taragolis approved these changes Nov 20, 2023

View reviewed changes

potiuk approved these changes Nov 20, 2023

View reviewed changes

potiuk merged commit 2d01cfd into apache:main Nov 20, 2023

ephraimbuddy pushed a commit that referenced this pull request Nov 23, 2023

feature(providers): added OpsgenieNotifier (#35530)

bd644fa

--------- Co-authored-by: Utkarsh Sharma <utkarsharma2@gmail.com> Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

eladkal mentioned this pull request Nov 24, 2023

Status of testing Providers that were prepared on November 24, 2023 #35845

Closed

69 tasks

ephraimbuddy pushed a commit that referenced this pull request Nov 26, 2023

feature(providers): added OpsgenieNotifier (#35530)

5369fdc

--------- Co-authored-by: Utkarsh Sharma <utkarsharma2@gmail.com> Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

feature(providers): added OpsgenieNotifier #35530

feature(providers): added OpsgenieNotifier #35530

Uh oh!

Conversation

Eric-Mendes commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborg bot commented Nov 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taragolis commented Nov 8, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taragolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taragolis Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boring-cyborg bot commented Nov 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feature(providers): added `OpsgenieNotifier` #35530

feature(providers): added `OpsgenieNotifier` #35530

Eric-Mendes commented Nov 8, 2023 •

edited

Loading

Taragolis Nov 17, 2023 •

edited

Loading