You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/08/25 17:17:24 UTC
[GitHub] [airflow] alwaysmpe opened a new issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
alwaysmpe opened a new issue #17833:
URL: https://github.com/apache/airflow/issues/17833
<!--
Welcome to Apache Airflow!
Please complete the next sections or the issue will be closed.
-->
**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:
https://github.com/apache/airflow/blob/96f7e3fec76a78f49032fbc9a4ee9a5551f38042/airflow/models/connection.py#L101
It's correctly used in the BaseHook class, eg here:
https://github.com/apache/airflow/blob/96f7e3fec76a78f49032fbc9a4ee9a5551f38042/airflow/hooks/base.py#L69
However in AwsBaseHook, it's expected to be in extra, here:
https://github.com/apache/airflow/blob/96f7e3fec76a78f49032fbc9a4ee9a5551f38042/airflow/providers/amazon/aws/hooks/base_aws.py#L404-L406
**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 is absolutely not required, but we are happy to guide you in contribution process
especially if you already have a good understanding of how to implement the fix.
Airflow is a community-managed project and we love to bring new contributors in.
Find us in #airflow-how-to-pr on Slack!
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eshabhide commented on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
eshabhide commented on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-1053836164
Hi, looks like this issue is not getting any traction. I am happy to take a look at it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-905773352
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.host` otherwise. This will make it backwards compatible (and we will be able to remove it in the future then).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] alwaysmpe commented on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
alwaysmpe commented on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-939263552
@prajvan feel free! Life's been complicated, this slipped the to-do list a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] prajvan commented on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
prajvan commented on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-939246901
hi @ashb i can take this up, if it isnt being worked upon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
eladkal commented on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-1053905753
@eshabhide assigned to you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-939258911
Depends if @alwaysmpe wants to continue, but generally feel free.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-905723673
Thanks for opening your first issue here! Be sure to follow the issue template!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] alwaysmpe commented on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
alwaysmpe commented on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-912050468
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, 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-912065820
> 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.
That sounds about right, yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blcksrx commented on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
blcksrx commented on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-907429690
In my opinion its better to put a deprecation warning as the first step to not broke user connection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] alwaysmpe edited a comment on issue #17833: AwsBaseHook isn't using connection.host, using connection.extra.host instead
Posted by GitBox <gi...@apache.org>.
alwaysmpe edited a comment on issue #17833:
URL: https://github.com/apache/airflow/issues/17833#issuecomment-912050468
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org