Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX AWS deferrable operators by using AioCredentials when using assume_role #32733

Merged
merged 5 commits into from
Jul 22, 2023

Conversation

rishi-kulkarni
Copy link
Contributor

@rishi-kulkarni rishi-kulkarni commented Jul 21, 2023

Closes: #32732 airflow.providers.amazong.aws.hooks.base_aws.BaseSessionFactory feeds synchronous credentials to aiobotocore when using assume_role

Fixes airflow.providers.amazong.aws.hooks.base_aws.BaseSessionFactory feeds synchronous credentials to aiobotocore when using `assume_role` apache#32732
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 21, 2023

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@shubham22
Copy link

shubham22 commented Jul 21, 2023

@vandonr-amz @syedahsn and @vincbeck - can you please prioritize reviewing this?

@rishi-kulkarni
Copy link
Contributor Author

Could someone re-trigger the one failing check? It looks like there was a network hiccup when it was uploading the test results.

@vincbeck
Copy link
Contributor

Could someone re-trigger the one failing check? It looks like there was a network hiccup when it was uploading the test results.

Just did

@vincbeck
Copy link
Contributor

Would it be possible to add unit tests to cover when deferrable is True

Copy link
Contributor

@vandonr-amz vandonr-amz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for catching and fixing this !

airflow/providers/amazon/aws/hooks/base_aws.py Outdated Show resolved Hide resolved
Copy link
Contributor

@syedahsn syedahsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for fixing it!
(I agree with @vincbeck about adding a unit test if possible)

@rishi-kulkarni
Copy link
Contributor Author

rishi-kulkarni commented Jul 21, 2023

@syedahsn @vincbeck Sure, I updated the existing tests for this method, which weren't checking if credentials._refresh is a coroutine. This additional assertion causes these tests to fail on main but pass on this branch.

Alternatively, I could check if get_frozen_credentials is a coroutine, since that was what specifically caused the error in the linked issue. Thoughts?

EDIT: Actually, since get_frozen_credentials is part of the public API of these credential objects, it's probably safer to make assertions against it.

@vincbeck
Copy link
Contributor

Interesting! LGTM. I'll merge it once the CI is green

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hussein-awala hussein-awala changed the title FIX: deferrable operators now use AioCredentials FIX AWS deferrable operators by using AioCredentials when using assume_role Jul 22, 2023
@hussein-awala hussein-awala merged commit 57f2032 into apache:main Jul 22, 2023
42 checks passed
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 22, 2023

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
6 participants