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

Refactor: Directly pass HttpOperator data and json arguments to requests and aiohttp params #38512

Closed

Conversation

gazev
Copy link
Contributor

@gazev gazev commented Mar 26, 2024

related: #37291

Both requests and aiohttp deal with json and data arguments in the same
manner (https://requests.readthedocs.io/en/latest/api/#requests.post and https://docs.aiohttp.org/en/stable/client_reference.html#aiohttp.ClientSession.request), so it would be more transparent to simply pass these arguments
directly. I believe it is also a good choice to be transparent in this
direct usage because requests is a very popular HTTP library so most
people already know how these arguments are expected.

The params argument could also be introduced in the HttpOperator with the same goal.

This PR was meant to address #37291 after seeing the discussion started here #37293 (comment)
The PR includes a breaking change.

Copy link

boring-cyborg bot commented Mar 26, 2024

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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.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: [email protected]
    Slack: https://s.apache.org/airflow-slack

@gazev gazev changed the title Refactor: Directly pass HttpOperator data and json arguments to requests and aiohttp params (#37291) Refactor: Directly pass HttpOperator data and json arguments to requests and aiohttp params Mar 26, 2024
…equests and aiohttp params

Transparently pass `data` and `json` HttpOperator arguments to `requests.Request`
and `aiohttp.ClientSession.post` (and other ClientSession HTTP methods).

This is a breaking  change.
@gazev gazev force-pushed the refactor-httpoperator-data-json-params branch from b5bd7b2 to 55712da Compare March 26, 2024 19:15
@gazev
Copy link
Contributor Author

gazev commented Apr 9, 2024

@potiuk I opened the PR because of this discussion #37293 (comment) but hadn't realized you said a breaking change would not be ideal. What should I do with the PR? Thank you!

@potiuk
Copy link
Member

potiuk commented Apr 11, 2024

Does the other PR fixes the same thing? If so - close it. If not - change yours to add new things. Why do you need to ask me :) ? Seems pretty straightforward?

@gazev gazev closed this Apr 11, 2024
@gazev
Copy link
Contributor Author

gazev commented Apr 11, 2024

Does the other PR fixes the same thing? If so - close it. If not - change yours to add new things. Why do you need to ask me :) ? Seems pretty straightforward?

I am really sorry for the inconvenience, I hadn't realized the breaking change was implemented in the mentioned PR.

@potiuk
Copy link
Member

potiuk commented Apr 11, 2024

No need to apologise :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants