Skip to content

Conversation

@Adaverse
Copy link
Contributor

closes: #31811

Builds account URL for below-mentioned auth methods if account URL is not provided using account name (login)

  • Public read
  • Shared access key

It helps avoid avoidable error. Doesn't apply to connection string and Azure AD.
Updated docs to reflect the same and implemented test to cover the changes made.
Exposed extra field (which was hidden) to incorporate the changes in #31783 which was missed.

A sample is provided below
Before -

20230622_234834

After -

20230622_235820


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@Adaverse
Copy link
Contributor Author

cc @phanikumv

@pankajastro pankajastro requested a review from phanikumv June 26, 2023 06:55
@Adaverse Adaverse marked this pull request as draft June 27, 2023 06:11
@Adaverse Adaverse force-pushed the patch-4 branch 2 times, most recently from fd2c9d8 to d622ead Compare June 27, 2023 18:19
@Adaverse Adaverse marked this pull request as ready for review June 27, 2023 21:16
@potiuk potiuk merged commit 46ee1c2 into apache:main Jun 27, 2023
return BlobServiceClient(
account_url=f"https://{conn.login}.blob.core.windows.net/{sas_token}", **extra
)
return BlobServiceClient(account_url=f"{account_url}/{sas_token}", **extra)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will give incorrect url if account_url ends with a slash e.g:
account_url = https://{conn.login}.blob.core.windows.net/
then here we will end up with:
account_url = https://{conn.login}.blob.core.windows.net//sas_token

Copy link
Contributor Author

@Adaverse Adaverse Jul 12, 2023

Choose a reason for hiding this comment

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

Yes, will correct it with patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Airflow Connection Type Azure Blob Storage - Shared Access Key field

3 participants