Update Azure fileshare hook to use azure-storage-file-share instead of azure-storage-file #33904
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
boring-cyborg
bot
added
area:providers
provider:google
|
Hi @potiuk / @eladkal I'm trying to update
I would like to hear your thoughts on this |
|
We can do a major release. I think this case justify it. |
…f azure-storage-file
Lee-W
reviewed
Aug 31, 2023
Comment on lines
41
to
53
| * Remove protocol param from Azure fileshare connection extras | ||
| * Remove deprecated extra__azure_fileshare__ prefix from Azure fileshare connection extras | ||
| * Remove share_name, directory_name param from AzureFileShareHook.check_for_directory in favor of AzureFileShareHook share_name and directory_path param | ||
| * Remove share_name, directory_name param from AzureFileShareHook.check_for_file method in favor of AzureFileShareHook share_name and directory_path param | ||
| * Remove share_name, directory_name param from AzureFileShareHook.list_directories_and_files | ||
| * Remove share_name, directory_name param from AzureFileShareHook.list_files in favor of AzureFileShareHook share_name and directory_path param | ||
| * AzureFileShareHook method create_share accept kwargs from ShareServiceClient.create_share instead of FileService.create_share | ||
| * AzureFileShareHook method delete_share accept kwargs from ShareServiceClient.delete_share instead of FileService.delete_share | ||
| * Remove share_name, directory_name param from AzureFileShareHook.create_directory in favor of AzureFileShareHook share_name and directory_path param | ||
| * Remove share_name, directory_name, file_name param from AzureFileShareHook.get_file in favor of AzureFileShareHook share_name and file_path | ||
| * Remove share_name, directory_name, file_name param from AzureFileShareHook.get_file_to_stream in favor of AzureFileShareHook share_name and file_path | ||
| * Remove share_name, directory_name, file_name param from AzureFileShareHook.load_file in favor of AzureFileShareHook share_name and file_path | ||
| * Remove AzureFileShareHook.load_string, AzureFileShareHook.load_stream in favor of AzureFileShareHook.load_data |
There was a problem hiding this comment.
Suggested change
| * Remove protocol param from Azure fileshare connection extras | |
| * Remove deprecated extra__azure_fileshare__ prefix from Azure fileshare connection extras | |
| * Remove share_name, directory_name param from AzureFileShareHook.check_for_directory in favor of AzureFileShareHook share_name and directory_path param | |
| * Remove share_name, directory_name param from AzureFileShareHook.check_for_file method in favor of AzureFileShareHook share_name and directory_path param | |
| * Remove share_name, directory_name param from AzureFileShareHook.list_directories_and_files | |
| * Remove share_name, directory_name param from AzureFileShareHook.list_files in favor of AzureFileShareHook share_name and directory_path param | |
| * AzureFileShareHook method create_share accept kwargs from ShareServiceClient.create_share instead of FileService.create_share | |
| * AzureFileShareHook method delete_share accept kwargs from ShareServiceClient.delete_share instead of FileService.delete_share | |
| * Remove share_name, directory_name param from AzureFileShareHook.create_directory in favor of AzureFileShareHook share_name and directory_path param | |
| * Remove share_name, directory_name, file_name param from AzureFileShareHook.get_file in favor of AzureFileShareHook share_name and file_path | |
| * Remove share_name, directory_name, file_name param from AzureFileShareHook.get_file_to_stream in favor of AzureFileShareHook share_name and file_path | |
| * Remove share_name, directory_name, file_name param from AzureFileShareHook.load_file in favor of AzureFileShareHook share_name and file_path | |
| * Remove AzureFileShareHook.load_string, AzureFileShareHook.load_stream in favor of AzureFileShareHook.load_data | |
| * Remove protocol param from Azure fileshare connection extras | |
| * Remove deprecated extra__azure_fileshare__ prefix from Azure fileshare connection extras | |
| * Remove share_name, directory_name param from the following in favor of AzureFileShareHook share_name and directory_path param | |
| * AzureFileShareHook.check_for_directory | |
| * AzureFileShareHook.check_for_file | |
| * AzureFileShareHook.list_files | |
| * AzureFileShareHook.create_directory | |
| * AzureFileShareHook.get_file | |
| * AzureFileShareHook.get_file_to_stream | |
| * AzureFileShareHook.load_file | |
| * Remove share_name, directory_name param from AzureFileShareHook.list_directories_and_files | |
| * AzureFileShareHook method create_share accept kwargs from ShareServiceClient.create_share instead of FileService.create_share | |
| * AzureFileShareHook method delete_share accept kwargs from ShareServiceClient.delete_share instead of FileService.delete_share | |
| * Remove AzureFileShareHook.load_string, AzureFileShareHook.load_stream in favor of AzureFileShareHook.load_data |
Should we consolidate similar changes into one nested list?
There was a problem hiding this comment.
I have updated it a bit, PTAL
Comment on lines
55
to
57
| self._connection_string = None | ||
| self._account_access_key = None | ||
| self._sas_token = None |
| if self._conn: | ||
| return self._conn | ||
| conn = self.get_connection(self.conn_id) | ||
| def get_conn(self) -> None: |
There was a problem hiding this comment.
May I know why do we want to return None for this method? I thought we'd get a file service client
There was a problem hiding this comment.
azure-storage-file-share exposes multiple clients, so I have added some property (share_service_client, share_directory_client, share_file_client) to access them instead of returning just one client from this method.
There was a problem hiding this comment.
If that's the case, should we add a message to avoid confusion? calling get_conn and return nothing might be confusing
|
I added an issue for this recently. Can you add to the description that this closes #33850 Thanks |
phanikumv
reviewed
Sep 1, 2023
eladkal
requested changes
Sep 1, 2023
eladkal
approved these changes
Sep 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes: #33850
azure-storage-file has not been updated since August 2019 and it is now outdated.
AzureFileShareHook is getting just in one operator AzureFileShareToGCSOperator, that too only
list_filesandget_file_to_streammethods so little risk if we release some breaking changes.The new azure-storage-file-share provides a more elegant way to call Azure API. Also, it allows to use features like a TokenCredential.
Since the update on the Azure SDK side is huge making it fully backward-compatible is a bit difficult and may not be possible
In the Airflow repo, we consider hook as a public interface/API so we might not like to do a direct breaking change release in that case I'll like to propose adding deprecation warring in the existing method and the param here is a WIP PR #33905 and then in the subsequent release, we can remove the deprecated one and use release this PR and also we will expose only whose method from Azure file share hook which is getting used in Airflow codebase.
^ 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.rstor{issue_number}.significant.rst, in newsfragments.