Skip to content
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

rewrite opsgenie alert hook with official python sdk, related issue #18641 #20263

Merged

Conversation

pierrejeambrun
Copy link
Member

@pierrejeambrun pierrejeambrun commented Dec 13, 2021

Hello guys,

As suggested in this issue, I tried rewriting the opsgenie alert hook with the official python sdk. I also added an example dag for the opsgenie_alert_operator

This is my first contribution to Airflow, any guidance or feedback would be appreciated :). To not break the backward compatibility due to the interface change, do you want me to keep the original file and mark everything as deprecated and create another hook for this ?

Best regards,

resolves #18641

@boring-cyborg
Copy link

boring-cyborg bot commented Dec 13, 2021

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 (flake8, 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.
    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

@pierrejeambrun pierrejeambrun force-pushed the opsgenie-alert-hook-with-official-sdk branch 11 times, most recently from e9bfcad to 01bcf88 Compare December 14, 2021 10:01
@pierrejeambrun
Copy link
Member Author

Just amended to fix checks.

@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 18, 2021
@potiuk
Copy link
Member

potiuk commented Dec 18, 2021

Static checks failing (I heartily recommend pre-commit installation)

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Dec 20, 2021

@potiuk, thanks for that, I just installed them and fixed the provider-yamls pre-commit.

setup.py Outdated Show resolved Hide resolved
UPDATING.md Outdated Show resolved Hide resolved
@pierrejeambrun pierrejeambrun force-pushed the opsgenie-alert-hook-with-official-sdk branch 2 times, most recently from 0c3c1ce to 2a5668d Compare December 20, 2021 18:16
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

LGTM

Can you please rebase ?
The test is failing on distutils which was fixed in #20420

@pierrejeambrun pierrejeambrun force-pushed the opsgenie-alert-hook-with-official-sdk branch from 2a5668d to 7e153a3 Compare December 20, 2021 20:53
@pierrejeambrun
Copy link
Member Author

Yes of course, I juste rebased onto the latest main version.

@eladkal
Copy link
Contributor

eladkal commented Dec 21, 2021

There is a still a failure
https://github.com/apache/airflow/runs/4591740391?check_suite_focus=true#step:6:865

You probably need to add a fix similar to https://github.com/apache/airflow/pull/20420/files
See the instructions in the test failure message

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Dec 21, 2021

Hi,

I have fixed that, and the job successfully ran locally, we should be good now.

We were missing warnings from fsspec.

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

LGTM

@potiuk any further comments?

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

@pierrejeambrun please check updated comment
I'll merge this PR after fixing it

dev/provider_packages/prepare_provider_packages.py Outdated Show resolved Hide resolved
@pierrejeambrun pierrejeambrun force-pushed the opsgenie-alert-hook-with-official-sdk branch from 51e0e59 to cadd66a Compare December 21, 2021 18:08
@pierrejeambrun
Copy link
Member Author

@eladkal We should be good 😄

@eladkal eladkal merged commit 46a6088 into apache:main Dec 21, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Dec 21, 2021

Awesome work, congrats on your first merged pull request!

@pierrejeambrun pierrejeambrun deleted the opsgenie-alert-hook-with-official-sdk branch December 21, 2021 20:31
@eladkal
Copy link
Contributor

eladkal commented Dec 21, 2021

Thanks @pierrejeambrun for your contribution!
If you are interested in taking another task check #8937 which is kinda similar

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Dec 22, 2021

My pleasure, it was really interesting helping on that.

I will try to first close my other PR #20386 that will certainly require more work. (I’ll also be OOO for a week)

After that if it is still available I can take it

@eladkal
Copy link
Contributor

eladkal commented Dec 22, 2021

@pierrejeambrun you can comment on the issue and i'll assign you

@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge kind:documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor OpsgenieAlertHook to official python sdk
5 participants