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/12/22 15:28:09 UTC
[GitHub] [airflow] potiuk opened a new pull request #20463: Fix backwards compatibility issue in AWS provider's _get_credentials
potiuk opened a new pull request #20463:
URL: https://github.com/apache/airflow/pull/20463
The #19815 change introduced backwards incompatibility for
the _get_credentials method - which is a centerpiece of AWS
provider and is likely to be overwritten by the user who want
for example inject auditing or other credentials-related custom
beheviours when interfacing with AWS even if the method is
protected.
The change added default for region, which caused signature
incompatibility with such derived classes. Unfortunately, we
already released 2.5.0 provider with this change. We had to
yank it and in order to avoid adding backwards-incompatible
3.0.0 release we are going to release 2.5.1 with this change
included.
Fixes: #20457
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
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/main/UPDATING.md).
--
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 pull request #20463: Fix backwards compatibility issue in AWS provider's _get_credentials
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20463:
URL: https://github.com/apache/airflow/pull/20463#issuecomment-999787269
But yeah. In this case when I added the default to expand_role - it could be back set to None default.
--
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 #20463: Fix backwards compatibility issue in AWS provider's _get_credentials
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20463:
URL: https://github.com/apache/airflow/pull/20463#issuecomment-999704096
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] potiuk commented on pull request #20463: Fix backwards compatibility issue in AWS provider's _get_credentials
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20463:
URL: https://github.com/apache/airflow/pull/20463#issuecomment-999723665
Aaaaah tooo fast :) (just saw your comment @kaxil @dstandish @uranusjr - let me know if there is anything to fix :)
--
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 pull request #20463: Fix backwards compatibility issue in AWS provider's _get_credentials
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20463:
URL: https://github.com/apache/airflow/pull/20463#issuecomment-999661018
All `test_base_aws.py` passed locally after the changes.
--
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 merged pull request #20463: Fix backwards compatibility issue in AWS provider's _get_credentials
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20463:
URL: https://github.com/apache/airflow/pull/20463
--
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] dstandish commented on pull request #20463: Fix backwards compatibility issue in AWS provider's _get_credentials
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #20463:
URL: https://github.com/apache/airflow/pull/20463#issuecomment-999736831
nothing to fix i'd say but i am curious why we no longer let the hook handle auth more completely....
like it used to be
```python
s3_resource = self.get_resource_type('s3')
return s3_resource.Bucket(bucket_name)
```
but as of [this change](https://github.com/potiuk/airflow/commit/83b51e53062dc596a630edd4bd01407a556f1aa6) we have much more complicated expression and get our hands more dirty. just curious for the reasoning on that. is there a reason not to let base hook handle giving us an authenticated resource, instead of repeating that logic within method calls?
also why _not_ let region_name be optional (i.e. have a default of None) in `_get_credentials`? that doesn't necessarily harm backcompat
--
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 pull request #20463: Fix backwards compatibility issue in AWS provider's _get_credentials
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20463:
URL: https://github.com/apache/airflow/pull/20463#issuecomment-999660056
Once we review and merge this one I will prepare 2.5.1 release of the provider.
--
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 pull request #20463: Fix backwards compatibility issue in AWS provider's _get_credentials
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20463:
URL: https://github.com/apache/airflow/pull/20463#issuecomment-999786339
You'll find answers here @dstandish : https://github.com/apache/airflow/issues/20457#issuecomment-999185822
The backcompat is kinda unexpected. But somewhat realistic case (and I guess @kaxil and @uranusjr already got a report about it) when someone overrides the _get_credentials() "an old way" and expand_role() would fail in this case.
This is not obvious but might happen.
--
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