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/11/23 20:02:31 UTC
[GitHub] [airflow] uranusjr opened a new pull request #19789: Fix missing region_name arguments in AWS provider
uranusjr opened a new pull request #19789:
URL: https://github.com/apache/airflow/pull/19789
I mishandled #19725.
cc @dimon222 @eladkal
--
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 pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977114443
Needs also a fix to the static tests of PostgresHook https://github.com/apache/airflow/runs/4303759942?check_suite_focus=true#step:10:163
--
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] uranusjr edited a comment on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977161257
No, the tests were only failing because they checked for `resource_type` and `client_type` arguments that you removed. The additions to the public API do not affect tests (because they are optional).
--
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 edited a comment on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
eladkal edited a comment on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977180345
> ```
> An error occurred (404) when calling the HeadObject operation: Not Found
> ```
>
> So calling `_get_credentials(region_name=None)` does not work after all?
Mmm woot :/
I think we should revert https://github.com/apache/airflow/pull/19725 first as it breaks main branch (static checks) and figure it out after...
--
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 a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755472415
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -539,7 +539,7 @@ def expand_role(self, role: str) -> str:
if "/" in role:
return role
else:
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
Review comment:
I don't think so...
Also lets not add more joy to this party :)
adding region_name is enhancement rather than addressing the bug/warnings problem so if someone thinks it's needed we can have it in a separated PR
--
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] dimon222 edited a comment on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977184751
https://github.com/apache/airflow/pull/19725/files#diff-3bd25af5c6450acdcccdd9e5da7372ea6c243b716ab1ce0436f39b4c07b8f5de
Replace in both s3 methods
`session.client(`
to
`session.resource(`
--
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] dimon222 commented on a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755517009
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -173,7 +173,7 @@ def get_bucket(self, bucket_name: Optional[str] = None) -> str:
:return: the bucket object to the bucket name.
:rtype: boto3.S3.Bucket
"""
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
s3_resource = session.client('s3', endpoint_url=endpoint_url, config=self.config, verify=self.verify)
Review comment:
session.resource(
--
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] uranusjr commented on a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755491582
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -164,16 +164,18 @@ def check_for_bucket(self, bucket_name: Optional[str] = None) -> bool:
return False
@provide_bucket_name
- def get_bucket(self, bucket_name: Optional[str] = None) -> str:
+ def get_bucket(self, bucket_name: Optional[str] = None, *, region_name: Optional[str] = None) -> str:
"""
Returns a boto3.S3.Bucket object
:param bucket_name: the name of the bucket
:type bucket_name: str
:return: the bucket object to the bucket name.
:rtype: boto3.S3.Bucket
+ :param region_name: The name of the aws region in which to get the bucket.
+ :type region_name: str
"""
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=region_name)
Review comment:
I felt it’s apretty cheap feature to add. But you’re right, let’s not complicate things. We can always add those back is someone wants 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimon222 commented on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977159372
I guess then tests also have to be reverted as well since those are expecting region_name to be passed?
--
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] uranusjr commented on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-978198715
We’re do this again from scratch.
--
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] uranusjr commented on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-978203119
New PR in #19815
--
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] uranusjr commented on a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755479008
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -539,7 +539,7 @@ def expand_role(self, role: str) -> str:
if "/" in role:
return role
else:
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
Review comment:
Doesn’t really matter since it’s internal; I like being explicit, so let’s not. (Actually I would’ve made it a keyword-only argument normally, but this is already a hot fix and I don’t want to complicate things unnecessarily.)
--
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] uranusjr commented on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977190447
Giving this one last shot. If it does not work, revert is available in #19791.
--
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 edited a comment on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
eladkal edited a comment on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977180345
> ```
> An error occurred (404) when calling the HeadObject operation: Not Found
> ```
>
> So calling `_get_credentials(region_name=None)` does not work after all?
Mmm woot :/
Not on my laptop so cant take a look :(
I think we should revert https://github.com/apache/airflow/pull/19725 first as it breaks main branch (static checks) and figure it out after...
--
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] dimon222 edited a comment on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977159372
I guess then tests also have to be reverted as well since those are expecting region_name to be passed now?
We a bit overthought this change. If there's so much opportunity for refactoring its worth a separate PR altogether for that kind of work in general.
--
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] dimon222 edited a comment on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977159372
I guess then tests also have to be reverted since those are expecting region_name to be passed now?
We a bit overthought this change. If there's so much opportunity for refactoring its worth a separate PR altogether for that kind of work in general.
--
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] dimon222 commented on a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755517104
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -341,7 +341,7 @@ def get_key(self, key: str, bucket_name: Optional[str] = None) -> S3Transfer:
:return: the key object from the bucket
:rtype: boto3.s3.Object
"""
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
s3_resource = session.client('s3', endpoint_url=endpoint_url, config=self.config, verify=self.verify)
Review comment:
session.resource(
--
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] uranusjr commented on a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755464410
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -539,7 +539,7 @@ def expand_role(self, role: str) -> str:
if "/" in role:
return role
else:
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
Review comment:
Do we want to add `region_name` to this?
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -539,7 +539,7 @@ def expand_role(self, role: str) -> str:
if "/" in role:
return role
else:
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
Review comment:
Do we want to add `region_name` to `expand_role`?
--
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] dimon222 commented on a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755478200
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -164,16 +164,18 @@ def check_for_bucket(self, bucket_name: Optional[str] = None) -> bool:
return False
@provide_bucket_name
- def get_bucket(self, bucket_name: Optional[str] = None) -> str:
+ def get_bucket(self, bucket_name: Optional[str] = None, *, region_name: Optional[str] = None) -> str:
"""
Returns a boto3.S3.Bucket object
:param bucket_name: the name of the bucket
:type bucket_name: str
:return: the bucket object to the bucket name.
:rtype: boto3.S3.Bucket
+ :param region_name: The name of the aws region in which to get the bucket.
+ :type region_name: str
"""
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=region_name)
Review comment:
It didn't have region_name provided before (even before my PR). Are we changing the behavior altogether now? This sounds like a fix but for completely different unrelated problem :)
--
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] dimon222 commented on a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755476557
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -539,7 +539,7 @@ def expand_role(self, role: str) -> str:
if "/" in role:
return role
else:
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
Review comment:
Since `_get_credentials` itself described region_name via hint as `Optional[Str]`... Should we instead add default = None for it in arguments?
--
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] dimon222 edited a comment on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977159372
I guess then tests also have to be reverted as well since those are expecting region_name to be passed now?
We a bit overthought this change. If there's so much opportunity for refactoring its worth a separate PR altogether for that kind of work in general
--
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 a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755472415
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -539,7 +539,7 @@ def expand_role(self, role: str) -> str:
if "/" in role:
return role
else:
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
Review comment:
I don't think so...
Also lets not add more joy to this party :)
adding region_name is enhancement rather than addressing the bug/warnings problem so if someone thinks it's needed we can have it in a separated PR (I think)
--
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] github-actions[bot] commented on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977126988
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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] uranusjr commented on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977125100
Thanks, fixed.
--
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] dimon222 commented on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977184751
https://github.com/apache/airflow/pull/19725/files#diff-3bd25af5c6450acdcccdd9e5da7372ea6c243b716ab1ce0436f39b4c07b8f5de
Replace
`session.client(`
to
`session.resource(`
--
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] uranusjr commented on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977176055
```
An error occurred (404) when calling the HeadObject operation: Not Found
```
So calling `_get_credentials(region_name=None)` does not work after all?
--
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] uranusjr commented on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977174196
Hmm, what are these `AttributeError: 'S3' object has no attribute 'Object'` errors about?
--
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] dimon222 commented on a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755517104
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -341,7 +341,7 @@ def get_key(self, key: str, bucket_name: Optional[str] = None) -> S3Transfer:
:return: the key object from the bucket
:rtype: boto3.s3.Object
"""
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
s3_resource = session.client('s3', endpoint_url=endpoint_url, config=self.config, verify=self.verify)
Review comment:
Should be `session.resource()` instead of `session.client()` (fix for earlier regression)
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -173,7 +173,7 @@ def get_bucket(self, bucket_name: Optional[str] = None) -> str:
:return: the bucket object to the bucket name.
:rtype: boto3.S3.Bucket
"""
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
s3_resource = session.client('s3', endpoint_url=endpoint_url, config=self.config, verify=self.verify)
Review comment:
Should be `session.resource()` instead of `session.client()` (fix for earlier regression)
--
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] dimon222 commented on a change in pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#discussion_r755517104
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -341,7 +341,7 @@ def get_key(self, key: str, bucket_name: Optional[str] = None) -> S3Transfer:
:return: the key object from the bucket
:rtype: boto3.s3.Object
"""
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
s3_resource = session.client('s3', endpoint_url=endpoint_url, config=self.config, verify=self.verify)
Review comment:
Should be `session.resource()` instead
##########
File path: airflow/providers/amazon/aws/hooks/s3.py
##########
@@ -173,7 +173,7 @@ def get_bucket(self, bucket_name: Optional[str] = None) -> str:
:return: the bucket object to the bucket name.
:rtype: boto3.S3.Bucket
"""
- session, endpoint_url = self._get_credentials()
+ session, endpoint_url = self._get_credentials(region_name=None)
s3_resource = session.client('s3', endpoint_url=endpoint_url, config=self.config, verify=self.verify)
Review comment:
Should be `session.resource()` instead
--
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 pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977180345
> ```
> An error occurred (404) when calling the HeadObject operation: Not Found
> ```
>
> So calling `_get_credentials(region_name=None)` does not work after all?
Mmm woot :/
I think we should revert https://github.com/apache/airflow/pull/19725 first as it breaks main branch and figure it out after...
--
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] uranusjr closed pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr closed pull request #19789:
URL: https://github.com/apache/airflow/pull/19789
--
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] uranusjr commented on pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19789:
URL: https://github.com/apache/airflow/pull/19789#issuecomment-977161257
No, the tests were only failing because they checked for `resource_type` and `client_type` arguments that you removed. The additions to the public API does not affect tests (because they are optional).
--
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] uranusjr closed pull request #19789: Fix missing region_name arguments in AWS provider
Posted by GitBox <gi...@apache.org>.
uranusjr closed pull request #19789:
URL: https://github.com/apache/airflow/pull/19789
--
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