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

Update the wasb connection #1994

Merged
merged 6 commits into from
Aug 8, 2023
Merged

Update the wasb connection #1994

merged 6 commits into from
Aug 8, 2023

Conversation

utkarsharma2
Copy link
Collaborator

@utkarsharma2 utkarsharma2 commented Aug 8, 2023

In version apache-airflow-providers-microsoft-azure==6.2.3 we are no longer getting the self.hook.get_conn().account_name to bypass the issue we can create connection in below fashion:

- conn_id: wasb_conn_with_access_key
  conn_type: wasb
  login: astrosdk
  host: astrosdk.blob.core.windows.net
  description: null
  extra:
    shared_access_key: $AZURE_WASB_ACCESS_KEY

@tatiana tatiana marked this pull request as draft August 8, 2023 11:17
@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (c593075) 89.67% compared to head (44186f9) 89.67%.
Report is 1 commits behind head on main.

❗ Current head 44186f9 differs from pull request most recent head 7630152. Consider uploading reports for the commit 7630152 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1994   +/-   ##
=======================================
  Coverage   89.67%   89.67%           
=======================================
  Files          72       72           
  Lines        4262     4262           
  Branches      530      530           
=======================================
  Hits         3822     3822           
  Misses        343      343           
  Partials       97       97           
Flag Coverage Δ
PythonSDK 89.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@utkarsharma2 utkarsharma2 changed the title Test main branch Add upperbound for apache-airflow-providers-microsoft-azure <= 6.2.2 Aug 8, 2023
@utkarsharma2 utkarsharma2 marked this pull request as ready for review August 8, 2023 11:35
@ephraimbuddy
Copy link
Contributor

In version apache-airflow-providers-microsoft-azure==6.2.3 we are no longer getting the self.hook.get_conn().account_name

Why are we no longer getting it? If it's a regression, we need to fix it in airflow
Limiting this will cause issues in runtime

@@ -73,7 +73,7 @@ amazon = [
"smart-open[s3]>=5.2.1",
]
azure = [
"apache-airflow-providers-microsoft-azure",
"apache-airflow-providers-microsoft-azure<=6.2.2", # TODO: Unpin this when we get self.hook.get_conn().account_name as not none for wasb hook.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have some ticket for this in upstream lib, I feel that way it is easy to track.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just created an upstream bug, will create a bug on our end as well - apache/airflow#33203

@utkarsharma2
Copy link
Collaborator Author

@ephraimbuddy Yes, I think so, it's a bug in the latest release 6.2.3. I'm planning to look into it post-releasing SDK.

@ephraimbuddy
Copy link
Contributor

@ephraimbuddy Yes, I think so, it's a bug in the latest release 6.2.3. I'm planning to look into it post-releasing SDK.

If we release this, it won't work with runtime so why release now instead of fixing the issue and making it part of the current providers release?

@utkarsharma2
Copy link
Collaborator Author

utkarsharma2 commented Aug 8, 2023

@ephraimbuddy There is a customer waiting for pandas 2.0 support which is part of this release.
Here - https://astronomer.slack.com/archives/C02B8SPT93K/p1689013818453639

@phanikumv
Copy link
Collaborator

@ephraimbuddy There is a customer waiting for pandas 2.0 support which is part of this release. Here - https://astronomer.slack.com/archives/C02B8SPT93K/p1689013818453639

But they would access cloud IDE by (indirectly) using Astro runtime right?

@utkarsharma2
Copy link
Collaborator Author

@phanikumv I'm not sure if they use runtime or not. But as @ephraimbuddy suggested a fix in connection, should unlock us from runtime issue as well. I'm trying the fix as we speak.

@tatiana
Copy link
Collaborator

tatiana commented Aug 8, 2023

@utkarsharma2, we could also try to change how we build the URL in this part of the code:
https://github.com/astronomer/astro-sdk/blob/main/python-sdk/src/astro/files/locations/azure/wasb.py#L123-L142

It seems the get_conn may expose a field account_url, we could try it out:
https://github.com/apache/airflow/blob/main/airflow/providers/microsoft/azure/hooks/wasb.py#L193C25-L193C36

@utkarsharma2 utkarsharma2 requested a review from a team August 8, 2023 12:47
@utkarsharma2
Copy link
Collaborator Author

@tatiana I looked into it and even account_url is not populating the account_name

@utkarsharma2 utkarsharma2 changed the title Add upperbound for apache-airflow-providers-microsoft-azure <= 6.2.2 Update the wasb connection Aug 8, 2023
@utkarsharma2 utkarsharma2 merged commit e86dce8 into main Aug 8, 2023
@utkarsharma2 utkarsharma2 deleted the DummyBranch branch August 8, 2023 13:03
utkarsharma2 added a commit that referenced this pull request Aug 8, 2023
In version apache-airflow-providers-microsoft-azure==6.2.3 we are no
longer getting the `self.hook.get_conn().account_name` to bypass the
issue we can create connection in below fashion:
```
- conn_id: wasb_conn_with_access_key
  conn_type: wasb
  login: astrosdk
  host: astrosdk.blob.core.windows.net
  description: null
  extra:
    shared_access_key: $AZURE_WASB_ACCESS_KEY
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants