-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Fix HttpAsyncHook headers #31010
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
Fix HttpAsyncHook headers #31010
Conversation
Fix extra fields used as headers
|
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)
|
|
@sunank200 Can you please take a look at this? I think this was just a typo. |
pankajastro
left a comment
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.
LGTM
|
We should add/update test for it. One of the reasons it did not work here was that either test was wrong or missing. |
sunank200
left a comment
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.
|
I have no clue how to do that. Figuring it out would take too much time for me. I just started this with a one character fix... |
Happy to guide you if you want. If not, then maybe somone looking here will help and add it. but generally we do not merge a change where tests do not fail before and do not fail after, because we have no way to verify it and keep it verified in the future versions. |
|
I marked it with "good first issue" which usually indicates to people that things are up for grabs and if you won't add tests, hopefully it will drag attentions of those who can add it (though we rarely do that for PRs) |
|
You can find http-related tests https://github.com/apache/airflow/tree/main/tests/providers/http and we are using pytest so probably you will have to add a test in https://github.com/apache/airflow/blob/main/tests/providers/http/hooks/test_http.py you can use pytest or breeze to run these tests on your local machine as well |
|
Some fixes are needed. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
|
@sunank200 Can you please help adding these test to your contribution? I had no time to figure out how to change the test env to 3.8+ python. |
|
Fixed in #32409 |
Fix extra fields used as headers