Repository navigation
AwsBaseHook isn't using connection.host, using connection.extra.host instead #17833
Description
Apache Airflow version:
Version: v2.1.2
Git Version: .release:2.1.2+d25854dd413aa68ea70fb1ade7fe01425f456192
OS:
PRETTY_NAME="Debian GNU/Linux 10 (buster)"
NAME="Debian GNU/Linux"
VERSION_ID="10"
VERSION="10 (buster)"
VERSION_CODENAME=buster
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"
Apache Airflow Provider versions:
Deployment:
Docker compose using reference docker compose from here: https://airflow.apache.org/docs/apache-airflow/stable/start/docker.html
What happened:
Connection specifications have a host field. This is declared as a member variable of the connection class here:
airflow/airflow/models/connection.py
Line 101 in 96f7e3f
It's correctly used in the BaseHook class, eg here:
Line 69 in 96f7e3f
However in AwsBaseHook, it's expected to be in extra, here:
airflow/airflow/providers/amazon/aws/hooks/base_aws.py
Lines 404 to 406 in 96f7e3f
This should be changed to
endpoint_url = connection_object.host
What you expected to happen:
AwsBaseHook should use the connection.host value, not connection.extra.host
How to reproduce it:
See above code
Anything else we need to know:
Are you willing to submit a PR?
Probably. It'd also be good for someone inexperienced. This would represent a backwards-incompatible change, any problem with that?
Activity
boring-cyborg commentedon Aug 25, 2021
Thanks for opening your first issue here! Be sure to follow the issue template!
potiuk commentedon Aug 25, 2021
Nice one. @alwaysmpe - I can assign you this one.
You can make it in the way that it checks if "host" is in extra an uses it from there if it's there (+raise a deprecation warning) and falls-back to
connection.hostotherwise. This will make it backwards compatible (and we will be able to remove it in the future then).blcksrx commentedon Aug 27, 2021
In my opinion its better to put a deprecation warning as the first step to not broke user connection.
alwaysmpe commentedon Sep 2, 2021
FYI: I'm currently moving house. Should have a look at this over the weekend. My thought was leave the current functionality working as is with a deprecated warning. Then if the host value in extra isn't present, use the connection defined version. Probably needs to combine host/port number, so I'll add a function to the connection to do that.
ashb commentedon Sep 2, 2021
That sounds about right, yes
prajvan commentedon Oct 9, 2021
hi @ashb i can take this up, if it isnt being worked upon
potiuk commentedon Oct 9, 2021
Depends if @alwaysmpe wants to continue, but generally feel free.
alwaysmpe commentedon Oct 9, 2021
@prajvan feel free! Life's been complicated, this slipped the to-do list a bit.
eshabhide commentedon Feb 28, 2022
Hi, looks like this issue is not getting any traction. I am happy to take a look at it.
eladkal commentedon Feb 28, 2022
@eshabhide assigned to you