You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Andrushika (via GitHub)" <gi...@apache.org> on 2026/07/30 09:44:38 UTC
[PR] Resolve AzureFileShareToGCSOperator directory_name alias after rendering [airflow]
Andrushika opened a new pull request, #70740:
URL: https://github.com/apache/airflow/pull/70740
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
Resolve AzureFileShareToGCSOperator directory_name alias after rendering
---
## Why
`directory_name` and `directory_path` are both template fields, rendered after `__init__` runs. The old constructor resolved the deprecated `directory_name` -> `directory_path` alias there, on the un-rendered values, which #70296 flags.
## What
Move the alias copy into `render_template_fields`, not `execute`. The alias needs to know if `directory_path` was passed, and with `render_template_as_native_obj=True` a supplied `directory_path` can render to `None`. So the check runs before rendering, where the raw values are still visible, and the copy runs after. The deprecation warning stays in `__init__`.
related: #70296
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Code (Opus 4.8)
Generated-by: Claude Code (Opus 4.8) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve AzureFileShareToGCSOperator directory_name alias after rendering [airflow]
Posted by "Andrushika (via GitHub)" <gi...@apache.org>.
Andrushika commented on code in PR #70740:
URL: https://github.com/apache/airflow/pull/70740#discussion_r3702990998
##########
providers/google/tests/unit/google/cloud/transfers/test_azure_fileshare_to_gcs.py:
##########
@@ -56,6 +61,96 @@ def test_init(self):
assert operator.dest_gcs == GCS_PATH_PREFIX
assert operator.google_impersonation_chain == IMPERSONATION_CHAIN
+ @mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.AzureFileShareHook")
+ @mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.GCSHook")
+ def test_directory_name_alias_uses_rendered_value(self, gcs_mock_hook, azure_fileshare_mock_hook):
+ """A templated directory_name is aliased to directory_path using its rendered value, not the Jinja expression."""
+ dag = DAG("test_azure_fileshare_alias", schedule=None, start_date=DEFAULT_DATE)
+ with pytest.warns(AirflowProviderDeprecationWarning, match="Use 'directory_path' instead"):
+ operator = AzureFileShareToGCSOperator(
+ task_id=TASK_ID,
+ share_name=AZURE_FILESHARE_SHARE,
+ directory_name="{{ params.legacy_dir }}",
+ params={"legacy_dir": "rendered/dir"},
+ azure_fileshare_conn_id=AZURE_FILESHARE_CONN_ID,
+ gcp_conn_id=GCS_CONN_ID,
+ dest_gcs=GCS_PATH_PREFIX,
+ return_gcs_uris=True,
+ dag=dag,
+ )
+ assert operator.directory_path is None
+
+ operator.render_template_fields({"params": {"legacy_dir": "rendered/dir"}})
+ assert operator.directory_path == "rendered/dir"
+
+ azure_fileshare_mock_hook.return_value.list_files.return_value = MOCK_FILES
+ operator.execute(None)
+ azure_fileshare_mock_hook.assert_any_call(
+ share_name=AZURE_FILESHARE_SHARE,
+ azure_fileshare_conn_id=AZURE_FILESHARE_CONN_ID,
+ directory_path="rendered/dir",
+ )
+
+ @mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.AzureFileShareHook")
+ @mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.GCSHook")
+ def test_native_directory_path_rendering_to_none_is_not_aliased(
+ self, gcs_mock_hook, azure_fileshare_mock_hook
+ ):
+ """
+ Behaviour-preservation guard for the __init__ -> render_template_fields move.
+
+ Deciding the alias from the pre-render values must be kept: with render_template_as_native_obj
+ an explicitly supplied directory_path can render to None, and re-inferring the alias afterwards
+ would wrongly fall back to the deprecated directory_name. This passes on the unrefactored code
+ (the provision decision was made in __init__) and fails if the decision is moved past rendering.
+ """
+ dag = DAG(
+ "test_azure_fileshare_native",
+ schedule=None,
+ start_date=DEFAULT_DATE,
+ render_template_as_native_obj=True,
+ )
+ operator = AzureFileShareToGCSOperator(
+ task_id=TASK_ID,
+ share_name=AZURE_FILESHARE_SHARE,
+ directory_path="{{ params.p }}",
+ directory_name="legacy",
+ params={"p": None},
+ azure_fileshare_conn_id=AZURE_FILESHARE_CONN_ID,
+ gcp_conn_id=GCS_CONN_ID,
+ dest_gcs=GCS_PATH_PREFIX,
+ return_gcs_uris=True,
+ dag=dag,
+ )
+
+ operator.render_template_fields({"params": {"p": None}})
+ assert operator.directory_path is None
+
+ # The deprecated alias must not silently substitute directory_name; a genuinely unset
+ # directory surfaces as the operator's own error instead of listing the wrong directory.
+ azure_fileshare_mock_hook.return_value.list_files.return_value = MOCK_FILES
+ with pytest.raises(RuntimeError, match="directory_name must be set"):
+ operator.execute(None)
+
+ @pytest.mark.parametrize(
+ "extra_kwargs",
+ [
+ pytest.param({"directory_path": AZURE_FILESHARE_DIRECTORY_PATH}, id="directory_path-supplied"),
+ pytest.param({}, id="neither-supplied"),
+ ],
+ )
+ def test_no_directory_name_deprecation_without_directory_name(self, extra_kwargs):
Review Comment:
Dropped, thanks for pointing it out!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve AzureFileShareToGCSOperator directory_name alias after rendering [airflow]
Posted by "Andrushika (via GitHub)" <gi...@apache.org>.
Andrushika commented on code in PR #70740:
URL: https://github.com/apache/airflow/pull/70740#discussion_r3702987110
##########
providers/google/src/airflow/providers/google/cloud/transfers/azure_fileshare_to_gcs.py:
##########
@@ -127,6 +128,14 @@ def __init__(
else:
self.return_gcs_uris = return_gcs_uris
+ def render_template_fields(self, context: Context, jinja_env: jinja2.Environment | None = None) -> None:
Review Comment:
Fixed with the flag approach you suggested. Added `test_mapped_task_aliases_directory_name` that goes through `partial()` -> `unmap()` -> `execute()`. I checked it fails on the previous version of this PR and passes on main. Thanks for catching this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve AzureFileShareToGCSOperator directory_name alias after rendering [airflow]
Posted by "Andrushika (via GitHub)" <gi...@apache.org>.
Andrushika commented on PR #70740:
URL: https://github.com/apache/airflow/pull/70740#issuecomment-5165112697
Thanks for reviewing, Shahar! I've addressed the requested changes. Could you take another look when you have time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve AzureFileShareToGCSOperator directory_name alias after rendering [airflow]
Posted by "Andrushika (via GitHub)" <gi...@apache.org>.
Andrushika commented on PR #70740:
URL: https://github.com/apache/airflow/pull/70740#issuecomment-5129230144
cc @shahar1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move AzureFileShareToGCSOperator directory_name alias out of __init__ [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #70740:
URL: https://github.com/apache/airflow/pull/70740#issuecomment-5646626000
### Backport failed to create: v3-3-test. View the failure log <a href='https://github.com/apache/airflow/actions/runs/34700431850'> Run details </a>
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>❌</td>
<td>v3-3-test</td>
<td><a href="https://github.com/apache/airflow/commit/df9a3e73c8b3ebb5cbca2134f4d35f6c9901ae0d"><img src='https://img.shields.io/badge/Commit-df9a3e7-red' alt='Commit Link'></a></td>
</tr>
</table>
You can attempt to backport this manually by running:
```bash
cherry_picker df9a3e7 v3-3-test
```
This should apply the commit to the v3-3-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.
After you have resolved the conflicts, you can continue the backport process by running:
```bash
cherry_picker --continue
```
If you don't have cherry-picker installed, see the [installation guide](https://github.com/apache/airflow/blob/main/dev/README.md#how-to-backport-pr-with-cherry-picker-cli).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move AzureFileShareToGCSOperator directory_name alias out of __init__ [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 merged PR #70740:
URL: https://github.com/apache/airflow/pull/70740
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve AzureFileShareToGCSOperator directory_name alias after rendering [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #70740:
URL: https://github.com/apache/airflow/pull/70740#discussion_r3695204837
##########
providers/google/src/airflow/providers/google/cloud/transfers/azure_fileshare_to_gcs.py:
##########
@@ -127,6 +128,14 @@ def __init__(
else:
self.return_gcs_uris = return_gcs_uris
+ def render_template_fields(self, context: Context, jinja_env: jinja2.Environment | None = None) -> None:
Review Comment:
This override won't run for mapped tasks. `MappedOperator.render_template_fields` calls `unmapped_task._do_render_template_fields(...)` directly rather than the operator's own `render_template_fields` — see the comment at [`mappedoperator.py:844-847`](https://github.com/apache/airflow/blob/main/task-sdk/src/airflow/sdk/definitions/mappedoperator.py#L844). I confirmed it with a scratch DAG: a subclass override fires for a plain task and not for a `.partial().expand()` one.
So `AzureFileShareToGCSOperator.partial(directory_name="dir").expand(...)` regresses: `__init__` no longer aliases, the override never fires, and `execute` hits `RuntimeError: The directory_name must be set!.`. That path works on `main` today — `unmap()` goes through `__init__`, which aliased the raw template string into `directory_path`, and `_do_render_template_fields` then rendered it correctly. (Which also means the un-rendered-alias bug this PR fixes only ever affected non-mapped tasks.)
Decide in `__init__` and apply in `execute` — that keeps your pre-render decision, which is the right call, and `execute` runs on both the mapped and unmapped paths:
```python
# in __init__, replacing the render_template_fields override
self._use_directory_name = directory_path is None and directory_name is not None
# at the top of execute()
if self._use_directory_name:
self.directory_path = self.directory_name
```
`_use_directory_name` isn't a template field, so `validate_operators_init` stays satisfied and the exemption removal still holds. If you'd rather keep it at render time, overriding `_do_render_template_fields` covers both paths too, but it's a private API — I'd lean toward the flag.
Either way, please add a mapped-task test (`.partial(directory_name=...)` → assert `directory_path` after execute), since that's the case that regressed.
##########
providers/google/tests/unit/google/cloud/transfers/test_azure_fileshare_to_gcs.py:
##########
@@ -56,6 +61,96 @@ def test_init(self):
assert operator.dest_gcs == GCS_PATH_PREFIX
assert operator.google_impersonation_chain == IMPERSONATION_CHAIN
+ @mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.AzureFileShareHook")
+ @mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.GCSHook")
+ def test_directory_name_alias_uses_rendered_value(self, gcs_mock_hook, azure_fileshare_mock_hook):
+ """A templated directory_name is aliased to directory_path using its rendered value, not the Jinja expression."""
+ dag = DAG("test_azure_fileshare_alias", schedule=None, start_date=DEFAULT_DATE)
+ with pytest.warns(AirflowProviderDeprecationWarning, match="Use 'directory_path' instead"):
+ operator = AzureFileShareToGCSOperator(
+ task_id=TASK_ID,
+ share_name=AZURE_FILESHARE_SHARE,
+ directory_name="{{ params.legacy_dir }}",
+ params={"legacy_dir": "rendered/dir"},
+ azure_fileshare_conn_id=AZURE_FILESHARE_CONN_ID,
+ gcp_conn_id=GCS_CONN_ID,
+ dest_gcs=GCS_PATH_PREFIX,
+ return_gcs_uris=True,
+ dag=dag,
+ )
+ assert operator.directory_path is None
+
+ operator.render_template_fields({"params": {"legacy_dir": "rendered/dir"}})
+ assert operator.directory_path == "rendered/dir"
+
+ azure_fileshare_mock_hook.return_value.list_files.return_value = MOCK_FILES
+ operator.execute(None)
+ azure_fileshare_mock_hook.assert_any_call(
+ share_name=AZURE_FILESHARE_SHARE,
+ azure_fileshare_conn_id=AZURE_FILESHARE_CONN_ID,
+ directory_path="rendered/dir",
+ )
+
+ @mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.AzureFileShareHook")
+ @mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.GCSHook")
+ def test_native_directory_path_rendering_to_none_is_not_aliased(
+ self, gcs_mock_hook, azure_fileshare_mock_hook
+ ):
+ """
+ Behaviour-preservation guard for the __init__ -> render_template_fields move.
+
+ Deciding the alias from the pre-render values must be kept: with render_template_as_native_obj
+ an explicitly supplied directory_path can render to None, and re-inferring the alias afterwards
+ would wrongly fall back to the deprecated directory_name. This passes on the unrefactored code
+ (the provision decision was made in __init__) and fails if the decision is moved past rendering.
+ """
+ dag = DAG(
+ "test_azure_fileshare_native",
+ schedule=None,
+ start_date=DEFAULT_DATE,
+ render_template_as_native_obj=True,
+ )
+ operator = AzureFileShareToGCSOperator(
+ task_id=TASK_ID,
+ share_name=AZURE_FILESHARE_SHARE,
+ directory_path="{{ params.p }}",
+ directory_name="legacy",
+ params={"p": None},
+ azure_fileshare_conn_id=AZURE_FILESHARE_CONN_ID,
+ gcp_conn_id=GCS_CONN_ID,
+ dest_gcs=GCS_PATH_PREFIX,
+ return_gcs_uris=True,
+ dag=dag,
+ )
+
+ operator.render_template_fields({"params": {"p": None}})
+ assert operator.directory_path is None
+
+ # The deprecated alias must not silently substitute directory_name; a genuinely unset
+ # directory surfaces as the operator's own error instead of listing the wrong directory.
+ azure_fileshare_mock_hook.return_value.list_files.return_value = MOCK_FILES
+ with pytest.raises(RuntimeError, match="directory_name must be set"):
+ operator.execute(None)
+
+ @pytest.mark.parametrize(
+ "extra_kwargs",
+ [
+ pytest.param({"directory_path": AZURE_FILESHARE_DIRECTORY_PATH}, id="directory_path-supplied"),
+ pytest.param({}, id="neither-supplied"),
+ ],
+ )
+ def test_no_directory_name_deprecation_without_directory_name(self, extra_kwargs):
Review Comment:
This test passes unchanged on `main` — the `__init__` condition is semantically identical before and after (`self.directory_path`/`self.directory_name` were assigned unmodified from the parameters just above), so it exercises pre-existing behaviour rather than anything this PR changes.
[AGENTS.md Testing Standards](https://github.com/apache/airflow/blob/main/AGENTS.md#testing-standards): *"Target exactly 100% coverage of what the PR changes — no more, no less... every test must fail without the PR's change. Do not add tests for pre-existing logic that was already present before the PR."*
Worth dropping, unless you're deliberately backfilling a gap — in which case a one-line note saying so would help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org