Support retriveing extra fields which do not have extra prefix set - Azure ADF DAG failure #899
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
With the 5.0.0 release of the Microsoft Azure PR, the PR apache/airflow#27758 introduced a breaking change where while creating new connections, the ``extra__`` prefix is no longer set for extra fields in the conneciton. This issue was not identified with testing the 5.0.0 RC because, it only happens for new connections that are created. The existing connections still contain the extra fiels with the ``extra__`` prefix. Hence, the existing code which looks for the connection field with the prefix ``extra__azure_data_factory__subscriptionId`` works on the older deployment with the new provider release as the connection was created before the release, but it fails on new deployments when a fresh connection is created. To fix this, we're removing the prefix while retrieving the connection field and at the same time we're supporting previously created connections by using the same ``get_field`` method from Airflow OSS introduced in the same PR above to allow backward compatibility.
pankajkoti
requested review from
phanikumv,
rajaths010494 and
pankajastro
as code owners
pankajkoti
changed the title
Codecov ReportBase: 98.61% // Head: 98.61% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #899 +/- ##
=======================================
Coverage 98.61% 98.61%
=======================================
Files 89 89
Lines 4974 4975 +1
=======================================
+ Hits 4905 4906 +1
Misses 69 69
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
rajaths010494
approved these changes
Feb 27, 2023
|
@pankajkoti good that we got the root cause of the issue and fixed it. |
|
I am merging this PR to test deployment from CircleCI main branch to new Astronomer Validation Org deployment. I will address comments if any that come later in a separate PR. |
ashb
reviewed
Feb 27, 2023
| import inspect | ||
| from functools import wraps | ||
| from typing import Any, Optional, TypeVar, Union, cast | ||
|
|
||
| from airflow.exceptions import AirflowException | ||
| from airflow.providers.microsoft.azure.hooks.data_factory import AzureDataFactoryHook | ||
| from airflow.providers.microsoft.azure.hooks.data_factory import AzureDataFactoryHook, get_field |
There was a problem hiding this comment.
Importing this will fail on v4 of the provider I think - is that a problem? Is this meant to only work with v5+?
There was a problem hiding this comment.
It's meant to support both versions. Thanks, @ashb , I will create another PR to fix this.
There was a problem hiding this comment.
Created a PR #900 to fix this.
pankajkoti
added a commit
that referenced
this pull request
Feb 27, 2023
PR #899 introduced an import for method ``get_field`` and started its usage. However, as reviewed and suggested by @ashb this will fail on providers with version < 5.0.0 as it was only the release 5.0.0 which included the ``get_field`` method. To solve this, we remove the dependency on the imported method and introduce the same method in local module.
This was referenced Feb 27, 2023
pankajkoti
added a commit
that referenced
this pull request
Feb 27, 2023
* Fix get_field import availability - PR #899 followup PR #899 introduced an import for method ``get_field`` and started its usage. However, as reviewed and suggested by @ashb this will fail on providers with version < 5.0.0 as it was only the release 5.0.0 which included the ``get_field`` method. To solve this, we remove the dependency on the imported method and introduce the same method in local module.
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.
With the 5.0.0 release of the Microsoft Azure PR, the PR apache/airflow#27047 introduced a breaking change where while creating new connections, the
extra__prefix is no longer set for extra fields in the conneciton. This issue was not identified with testing the 5.0.0 RC because, it only happens for new connections that are created. The existing connections still contain the extra fiels with theextra__prefix. Hence, the existing code which looks for the connection field with the prefixextra__azure_data_factory__subscriptionIdworks on the older deployment with the new provider release as the connection was created before the release, but it fails on new deployments when a fresh connection is created.To fix this, we're removing the prefix while retrieving the connection field and at the same time we're supporting previously created connections by using the same
get_fieldmethod from Airflow OSS introduced in the same PR above to allow backward compatibility.closes: #897