-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Add Jira Notifier implementation #35397
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.
In general looks good
Just need to add tests and information into the provider documentation. Another PR's for the reference:
ee24614
to
b684d21
Compare
@Taragolis Addressed the above mentioned suggestions. Still working on the tests and the docs. |
Documentation should be pretty simple: copy from one of existed provider, replace by new example (you have in description of PR), change reference to the Notifier classes, build it locally by run And if you need any help you might ask the questions in the Slack channel |
00efcb4
to
539aa93
Compare
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.
Some NIT which might make it better
In general looks good, also check documentation locally
docs/apache-airflow-providers-atlassian-jira/notifications/jira-notifier-howto-guide.rst
Outdated
Show resolved
Hide resolved
docs/apache-airflow-providers-atlassian-jira/notifications/jira-notifier-howto-guide.rst
Outdated
Show resolved
Hide resolved
Built and checked the new docs locally. There weren't any issues. Will check again with the suggestions applied. |
That is provider version. Providers release cycle are different then Airflow itself |
BashOperator( | ||
task_id="mytask", | ||
on_failure_callback=[ | ||
send_jira_notification( |
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.
Shouodn't this be JiraNotifier(...)
?
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.
All the notifiers seem to have followed this pattern. e.g.,
send_chime_notification = ChimeNotifier |
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.
Yeah, this pattern exists from the very first notifier.
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.
Pattern aside.. this is an example that we publish in docs so we can change it if we wish.
Isn't on_failure_callback=ChimeNotifier(...)
Works the same and is more intuitive?
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.
Pattern aside.. this is an example that we publish in docs so we can change it if we wish.
Yes, can use either approach. But then it'll look inconsistent with the existing examples in other notifier docs though. e.g., https://airflow.apache.org/docs/apache-airflow-providers-amazon/8.6.0/notifications/chime_notifier_howto_guide.html
Isn't
on_failure_callback=ChimeNotifier(...)
Works the same and is more intuitive?
Functionally, exactly the same. Personally for me, having an action verb in the name reads better in this context though. Like, on failure, send this notification.
A kind reminder on this. Are we good to merge? |
Fixes #35261
Sample usage:
^ 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.