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 2020/04/24 00:38:17 UTC
[GitHub] [airflow] houqp opened a new pull request #8534: fix: aws shook should work without conn id
houqp opened a new pull request #8534:
URL: https://github.com/apache/airflow/pull/8534
This patch makes behavior of hook consistent with documentation.
AWS hooks should support falling back to using default credential chain
lookup behavior when connection id is not specified.
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Target Github ISSUE in description if exists
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#issuecomment-620349037
Thanks @houqp
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415184153
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
I think we should not use `Optional ` here. Should only use in default `None`, am I wrong?
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -254,7 +253,7 @@ def _assume_role(
assume_role_kwargs["ExternalId"] = extra_config.get(
"external_id"
)
- role_session_name = "Airflow_" + self.aws_conn_id
+ role_session_name = "Airflow_" + str(self.aws_conn_id)
Review comment:
How about
```suggestion
role_session_name = f"Airflow_{self.aws_conn_id}"
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415321895
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
This PR is just about to bring back this behavior. `aws hook should work without conn id`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415317753
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
> This does not cause error for AWS. AWS hooks should support falling back to using default credential chain lookup behavior when connection id is not specified.
!!!!!!! Wow, I don't know aws have this behavior!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415217630
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
@turbaszek @potiuk https://docs.python.org/3/library/typing.html#typing.Optional WDYT
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie edited a comment on pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
zhongjiajie edited a comment on pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#issuecomment-620039650
It's odd, GA failed and I could not restart it. could you pls force push to restart? @houqp
<img width="1336" alt="Screen Shot 2020-04-27 at 22 57 58" src="https://user-images.githubusercontent.com/15820530/80386919-9a3a9500-88da-11ea-915e-0846e79408c7.png">
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415309631
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
@zhongjiajie is right:
> Optional[X] is equivalent to Union[X, None].
we should use `aws_conn_id: str = "aws_default"` because passing `None` will cause an error (probably)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#issuecomment-620039650
GA failed and I could not restart it. could you pls force push to restart? @houqp
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415217502
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
I not sure about that, I init think it should be use only if default value equal to `None`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415427532
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
> the wording in the documentation linked by @zhongjiajie is a little bit misleading, which probably caused confusion between `Optional` type and optional argument.
I can quite agree. In fact, I think introducing types in dynamically typed langauge creates more confusion that in statically typed one. In most places in airflow, I think we are using `Optional` when the default value is `None`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415375821
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
the wording in the documentation linked by @zhongjiajie is a little bit misleading, which probably caused confusion between `Optional` type and optional argument.
`def foo(a: Optional[str] = 'test'):` tells mypy that argument `a` needs to be either `None` or a `str`. Since the default value `'test` is a string, this is a valid use of `Optional` type annotation. `a: Optional[str] = None` is also a valid annotation.
If we allow `a` to take `None` as value, but annotate it as `def foo(a: str='test')`, then mypy will not be able to catch errors like `b = a + '_suffix'` because the annotation `str` tells mypy that `a` will always be a string. Mypy will also prevent users from calling `foo` with `foo(None)` due to missing `Optional` in the type annotation, which is not what we want.
Behavior wise, both `aws_conn_id: Optional[str] = None` and `aws_conn_id: Optional[str] = 'aws_default'` will work for aws hook. I kept it to `aws_conn_id: Optional[str] = 'aws_default'` because that's what it used to be. I don't know if it's going to break anything if default is changed to `None` since there might be users out there actually using `aws_default` connection as the way to inject global aws credential instead of using default credential chain lookup.
In AWS land, the best practice is to use default credential chain lookup, so `aws_conn_id: Optional[str] = None` would be better. It's up to us whether we are willing risk breaking existing users in order to adopt a better practice. I don't have a strong opinion on this, but if anyone thinks we should take the chance for airflow 2.0, I am open to change it to `None`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415314647
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
> This does not cause error for AWS. AWS hooks should support falling back to using default credential chain lookup behavior when connection id is not specified.
That's why I used "probably" because I don't know AWS hook behavior. Still we should use this:
```
aws_conn_id: str = "aws_default"
```
or this:
```
aws_conn_id: Optional[str] = None
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415322742
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
For GCP, we used a different behavior GCP always need a connection because it defines connection information. A special connection mode is ADC expressed by connection configuration. ADC is the default configuration in Airflow.
```
`Application Default Credentials`_ provides an easy way to obtain
credentials to call Google APIs for server-to-server or local applications.
This function acquires credentials from the environment in the following
order:
1. If the environment variable ``GOOGLE_APPLICATION_CREDENTIALS`` is set
to the path of a valid service account JSON private key file, then it is
loaded and returned. The project ID returned is the project ID defined
in the service account file if available (some older files do not
contain project ID information).
2. If the `Google Cloud SDK`_ is installed and has application default
credentials set they are loaded and returned.
To enable application default credentials with the Cloud SDK run::
gcloud auth application-default login
If the Cloud SDK has an active project, the project ID is returned. The
active project can be set using::
gcloud config set project
3. If the application is running in the `App Engine standard environment`_
then the credentials and project ID from the `App Identity Service`_
are used.
4. If the application is running in `Compute Engine`_ or the
`App Engine flexible environment`_ then the credentials and project ID
are obtained from the `Metadata Service`_.
5. If no credentials are found,
:class:`~google.auth.exceptions.DefaultCredentialsError` will be raised.
.. _Application Default Credentials: https://developers.google.com\
/identity/protocols/application-default-credentials
.. _Google Cloud SDK: https://cloud.google.com/sdk
.. _App Engine standard environment: https://cloud.google.com/appengine
.. _App Identity Service: https://cloud.google.com/appengine/docs/python\
/appidentity/
.. _Compute Engine: https://cloud.google.com/compute
.. _App Engine flexible environment: https://cloud.google.com\
/appengine/flexible
.. _Metadata Service: https://cloud.google.com/compute/docs\
/storing-retrieving-metadata
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#issuecomment-619450077
@zhongjiajie @potiuk added test case for None.
also thanks @zhongjiajie for fixing the typo for me :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #8534: fix: aws shook should work without conn id
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415031692
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: str = "aws_default",
Review comment:
Optional is required only when default value is `None`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415375821
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
the wording in the documentation linked by @zhongjiajie is a little bit misleading, which probably caused confusion between `Optional` type and optional argument.
`def foo(a: Optional[str] = 'test'):` tells mypy that argument `a` needs to be either `None` or a `str`. Since the default value `'test'` is a string, this is a valid use of `Optional` type annotation. `a: Optional[str] = None` is also a valid annotation.
If we allow `a` to take `None` as value, but annotate it as `def foo(a: str='test')`, then mypy will not be able to catch errors like `b = a + '_suffix'` because the annotation `str` tells mypy that `a` will always be a string. Mypy will also prevent users from calling `foo` with `foo(None)` due to missing `Optional` in the type annotation, which is not what we want.
Behavior wise, both `aws_conn_id: Optional[str] = None` and `aws_conn_id: Optional[str] = 'aws_default'` will work for aws hook. I kept it to `aws_conn_id: Optional[str] = 'aws_default'` because that's what it used to be. I don't know if it's going to break anything if default is changed to `None` since there might be users out there actually using `aws_default` connection as the way to inject global aws credential instead of using default credential chain lookup.
In AWS land, the best practice is to use default credential chain lookup, so `aws_conn_id: Optional[str] = None` would be better. It's up to us whether we are willing risk breaking existing users in order to adopt a better practice. I don't have a strong opinion on this, but if anyone thinks we should take the chance for airflow 2.0, I am open to change it to `None`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415325115
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
Authorization using the metaserver is particularly problematic, because the server may have wider rights than we expect the instance user to have. Example: The server may have read / write access to Stackdriver logs, but the user should not have these privileges. The similar thing is with system buckets that the server can use, but the user should not be able to modify them. A similar thing looks like system buckets that the server can use, but the user should not be able to modify them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#issuecomment-619390399
I think we only need one more test as Jerak said.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415323023
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
There is only one exception to this rule in Airflow 1.10.x, which has been fixed in Airflow 2.0 - GKEPodOperator.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415204127
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
Optional is a valid type annotation here because we allow user to explicitly pass None as the value.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415883172
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
> > the wording in the documentation linked by @zhongjiajie is a little bit misleading, which probably caused confusion between `Optional` type and optional argument.
>
> I can quite agree. In fact, I think introducing types in dynamically typed langauge creates more confusion that in statically typed one. In most places in airflow, I think we are using `Optional` when the default value is `None`
Yep, It's a little confusion here, thanks for the clarification. And maybe we better keep default value not equal to None.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415446445
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
I also do not have a strong opinion, but I wanted to present the GCP approach for comparison.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415321895
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
This PR is just about to bring back this behavior. `fix: aws hook should work without conn id`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415314105
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
This does not cause error for AWS. AWS hooks should support falling back to using default credential chain lookup behavior when connection id is not specified.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #8534: fix: aws hook should work without conn id
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415322742
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
For GCP, we used a different behavior. GCP always need a connection because it defines connection information. A special connection mode is ADC expressed by connection configuration. ADC is the default configuration in Airflow.
```
`Application Default Credentials`_ provides an easy way to obtain
credentials to call Google APIs for server-to-server or local applications.
This function acquires credentials from the environment in the following
order:
1. If the environment variable ``GOOGLE_APPLICATION_CREDENTIALS`` is set
to the path of a valid service account JSON private key file, then it is
loaded and returned. The project ID returned is the project ID defined
in the service account file if available (some older files do not
contain project ID information).
2. If the `Google Cloud SDK`_ is installed and has application default
credentials set they are loaded and returned.
To enable application default credentials with the Cloud SDK run::
gcloud auth application-default login
If the Cloud SDK has an active project, the project ID is returned. The
active project can be set using::
gcloud config set project
3. If the application is running in the `App Engine standard environment`_
then the credentials and project ID from the `App Identity Service`_
are used.
4. If the application is running in `Compute Engine`_ or the
`App Engine flexible environment`_ then the credentials and project ID
are obtained from the `Metadata Service`_.
5. If no credentials are found,
:class:`~google.auth.exceptions.DefaultCredentialsError` will be raised.
.. _Application Default Credentials: https://developers.google.com\
/identity/protocols/application-default-credentials
.. _Google Cloud SDK: https://cloud.google.com/sdk
.. _App Engine standard environment: https://cloud.google.com/appengine
.. _App Identity Service: https://cloud.google.com/appengine/docs/python\
/appidentity/
.. _Compute Engine: https://cloud.google.com/compute
.. _App Engine flexible environment: https://cloud.google.com\
/appengine/flexible
.. _Metadata Service: https://cloud.google.com/compute/docs\
/storing-retrieving-metadata
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #8534: fix: aws shook should work without conn id
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415023032
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: str = "aws_default",
Review comment:
Should not that be Optional[str] ?
##########
File path: airflow/utils/log/s3_task_handler.py
##########
@@ -48,7 +48,7 @@ def hook(self):
from airflow.providers.amazon.aws.hooks.s3 import S3Hook
return S3Hook(remote_conn_id)
except Exception: # pylint: disable=broad-except
- self.log.error(
+ self.log.exception(
Review comment:
nice!
##########
File path: tests/providers/amazon/aws/hooks/test_base_aws.py
##########
@@ -208,3 +208,8 @@ def test_expand_role(self):
arn = hook.expand_role('test-role')
expect_arn = conn.get_role(RoleName='test-role').get('Role').get('Arn')
self.assertEqual(arn, expect_arn)
+
Review comment:
We need test for None I guess.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org