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

Fixes AzureFileShare connection extras #16388

Merged
merged 3 commits into from
Jun 11, 2021

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jun 11, 2021

The Azure File Share connection has not been creted in #15159 and it
caused an unexpected side effect as the default Azure Connection
passed service_options dictionary to FileService
with key that was unexpected.

This change fixes two things:

  1. adds AzureFileShare connection that has separate conn_type
    and handles the extra_options specific for FileService Hook
    available in the Airflow UI.

  2. handles the "deprecated" way of passing keys without UI prefix
    but raises a deprecation warning when such key is passed or
    when the Wasb connection is used with an empty extras rather
    than Azure File Share.

Fixes #16254


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@potiuk
Copy link
Member Author

potiuk commented Jun 11, 2021

cc: @sunkickr @cvsekhar

@potiuk potiuk force-pushed the fix-azure-fileshare-connection-extras branch 2 times, most recently from 2b27c6d to 63a71d1 Compare June 11, 2021 13:31
@potiuk potiuk requested a review from eladkal June 11, 2021 13:32
@potiuk potiuk requested review from kaxil, jmcarp and ashb June 11, 2021 14:40
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jun 11, 2021
@github-actions
Copy link

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.

The Azure File Share connection has not been creted in apache#15159 and it
caused an unexpected side effect as the default Azure Connection
passed service_options dictionary to FileService
with key that was unexpected.

This change fixes two things:

1) adds AzureFileShare connection that has separate conn_type
   and handles the extra_options specific for FileService Hook
   available in the Airflow UI.

2) handles the "deprecated" way of passing keys without UI prefix
   but raises a deprecation warning when such key is passed or
   when the Wasb connection is used with an empty extras rather
   than Azure File Share.

Fixes apache#16254
@potiuk potiuk force-pushed the fix-azure-fileshare-connection-extras branch from 63a71d1 to 3cf66c9 Compare June 11, 2021 16:41
@potiuk
Copy link
Member Author

potiuk commented Jun 11, 2021

I also changed the tests to fix the annoying behaviour of having to fix tests after every new connection (I am now just checking if the returned arrays have reasonable size > x) rather than exact content/list. That should be good enough and avoid the extra hoop of falling CI when you think you've added new connection/provider etc.

@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 11, 2021
@sunkickr
Copy link
Contributor

@potiuk thanks for fixing this error!

@potiuk
Copy link
Member Author

potiuk commented Jun 11, 2021

No problem. I am merging it in soon and that's last thing before releasing the providers (it's been delayed due to my unavailability but I am catching up).

@sunkickr
Copy link
Contributor

@potiuk an Azure Container Volume connection was also not created in #15159. Wondering if this would cause any similar issues with AzureContainerVolumeHook as it also uses the 'wasb_default' connection id?

@potiuk
Copy link
Member Author

potiuk commented Jun 11, 2021

Good point. Yeah I think this needs fixing as well:

if 'connection_string' in service_options:

It will not crash, but it will not use the extra either

@potiuk
Copy link
Member Author

potiuk commented Jun 11, 2021

I also noticed that howtos needs updating

@potiuk
Copy link
Member Author

potiuk commented Jun 11, 2021

Added new connection type and updated the howtos in latest fixup @sunkickr - would you mind taking a look ?

@potiuk potiuk requested a review from turbaszek as a code owner June 11, 2021 19:21
@potiuk potiuk merged commit 0c80a7d into apache:main Jun 11, 2021
@potiuk potiuk deleted the fix-azure-fileshare-connection-extras branch June 11, 2021 21:38
potiuk added a commit to potiuk/airflow that referenced this pull request Jun 22, 2021
* Fixes AzureFileShare connection extras

The Azure File Share connection has not been creted in apache#15159 and it
caused an unexpected side effect as the default Azure Connection
passed service_options dictionary to FileService
with key that was unexpected.

This change fixes two things:

1) adds AzureFileShare connection that has separate conn_type
   and handles the extra_options specific for FileService Hook
   available in the Airflow UI.

2) handles the "deprecated" way of passing keys without UI prefix
   but raises a deprecation warning when such key is passed or
   when the Wasb connection is used with an empty extras rather
   than Azure File Share.

Fixes apache#16254

* fixup! Fixes AzureFileShare connection extras

* fixup! fixup! Fixes AzureFileShare connection extras

(cherry picked from commit 0c80a7d)
@potiuk potiuk restored the fix-azure-fileshare-connection-extras branch April 26, 2022 20:53
@potiuk potiuk deleted the fix-azure-fileshare-connection-extras branch July 29, 2022 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers full tests needed We need to run full set of tests for this PR to merge okay to merge It's ok to merge this PR as it does not require more tests provider:microsoft-azure Azure-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wasb connection to Azure Fileshareservice Hook not working
4 participants