kaxil commented on code in PR #64879:
URL: https://github.com/apache/airflow/pull/64879#discussion_r3097494268
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -300,9 +300,13 @@ def _fetch_bare_repo(self):
reraise=True,
)
def _fetch_submodules(self) -> None:
- self._log.info("Initializing and updating submodules",
repo_path=self.repo_path)
- self.repo.git.submodule("sync", "--recursive")
- self.repo.git.submodule("update", "--init", "--recursive", "--jobs",
"1")
+ ssh_env_cm = nullcontext()
+ if self.hook and (cmd := self.hook.env.get("GIT_SSH_COMMAND")):
+ ssh_env_cm = self.repo.git.custom_environment(GIT_SSH_COMMAND=cmd)
Review Comment:
Only `GIT_SSH_COMMAND` is forwarded via `custom_environment`, but
`configure_hook_env` also populates `hook.env` with
`SSH_ASKPASS`/`DISPLAY`/`SSH_ASKPASS_REQUIRE` for passphrase-protected keys.
These work today because `_fetch_submodules` is always called inside an active
`configure_hook_env()` in `_initialize`/`refresh`, which sets those on
`os.environ`. If a future refactor drops that outer context,
passphrase-protected keys will silently break while the non-passphrase path
keeps working. Would you consider forwarding the full dict
(`custom_environment(**self.hook.env)`) or at minimum a comment documenting
that this depends on the outer `configure_hook_env` context?
##########
providers/git/tests/unit/git/bundles/test_git.py:
##########
@@ -920,6 +920,66 @@ def
test_clone_bare_repo_invalid_repository_error_retry_fails(
# Verify Repo was called twice (failed attempt + failed retry)
assert mock_repo_class.call_count == 2
+ @mock.patch("airflow.providers.git.bundles.git.GitHook")
+ def test_fetch_submodules_uses_custom_environment_when_git_ssh_command_set(
+ self, mock_githook_class
+ ):
+ """GIT_SSH_COMMAND from hook.env is passed to custom_environment;
submodule runs inside that context."""
+ expected_ssh_cmd = (
+ "ssh -i /path/key -o IdentitiesOnly=yes -o
UserKnownHostsFile=/path/known_hosts"
+ )
+ mock_hook = mock_githook_class.return_value
+ mock_hook.repo_url = "[email protected]:apache/airflow.git"
+ mock_hook.env = {"GIT_SSH_COMMAND": expected_ssh_cmd}
+
+ mock_repo = mock.MagicMock()
+ ssh_ctx = mock.MagicMock()
+ mock_repo.git.custom_environment.return_value = ssh_ctx
+
+ bundle = GitDagBundle(
+ name="test",
+ git_conn_id="git_default",
+ tracking_ref="main",
+ version="123456",
+ submodules=True,
+ )
+ bundle.repo = mock_repo
+
+ bundle._fetch_submodules()
+
+
mock_repo.git.custom_environment.assert_called_once_with(GIT_SSH_COMMAND=expected_ssh_cmd)
+ ssh_ctx.__enter__.assert_called_once()
+ ssh_ctx.__exit__.assert_called_once()
+ mock_repo.git.submodule.assert_has_calls(
+ [mock.call("sync", "--recursive"), mock.call("update", "--init",
"--recursive", "--jobs", "1")]
+ )
+ assert mock_repo.git.mock_calls[0] ==
mock.call.custom_environment(GIT_SSH_COMMAND=expected_ssh_cmd)
+ assert mock_repo.git.mock_calls[1] == mock.call.submodule("sync",
"--recursive")
Review Comment:
This assertion will fail. With a `with` block, `mock_repo.git.mock_calls` is
`[call.custom_environment(...), call.custom_environment().__enter__(),
call.submodule("sync", "--recursive"), call.submodule("update", ...),
call.custom_environment().__exit__(None, None, None)]`, so index `[1]` is
`__enter__()`, not the submodule call. I verified this empirically. Either drop
these two index assertions (the `assert_has_calls` above already enforces
submodule ordering) or use index `[2]`.
##########
providers/git/tests/unit/git/bundles/test_git.py:
##########
@@ -920,6 +920,66 @@ def
test_clone_bare_repo_invalid_repository_error_retry_fails(
# Verify Repo was called twice (failed attempt + failed retry)
assert mock_repo_class.call_count == 2
+ @mock.patch("airflow.providers.git.bundles.git.GitHook")
+ def test_fetch_submodules_uses_custom_environment_when_git_ssh_command_set(
+ self, mock_githook_class
+ ):
+ """GIT_SSH_COMMAND from hook.env is passed to custom_environment;
submodule runs inside that context."""
+ expected_ssh_cmd = (
+ "ssh -i /path/key -o IdentitiesOnly=yes -o
UserKnownHostsFile=/path/known_hosts"
+ )
+ mock_hook = mock_githook_class.return_value
+ mock_hook.repo_url = "[email protected]:apache/airflow.git"
+ mock_hook.env = {"GIT_SSH_COMMAND": expected_ssh_cmd}
+
+ mock_repo = mock.MagicMock()
+ ssh_ctx = mock.MagicMock()
+ mock_repo.git.custom_environment.return_value = ssh_ctx
+
+ bundle = GitDagBundle(
+ name="test",
+ git_conn_id="git_default",
+ tracking_ref="main",
+ version="123456",
+ submodules=True,
+ )
+ bundle.repo = mock_repo
+
+ bundle._fetch_submodules()
+
+
mock_repo.git.custom_environment.assert_called_once_with(GIT_SSH_COMMAND=expected_ssh_cmd)
+ ssh_ctx.__enter__.assert_called_once()
+ ssh_ctx.__exit__.assert_called_once()
+ mock_repo.git.submodule.assert_has_calls(
+ [mock.call("sync", "--recursive"), mock.call("update", "--init",
"--recursive", "--jobs", "1")]
+ )
+ assert mock_repo.git.mock_calls[0] ==
mock.call.custom_environment(GIT_SSH_COMMAND=expected_ssh_cmd)
Review Comment:
This pair of `mock_calls[0]`/`mock_calls[1]` checks is redundant with
`assert_has_calls` on the line above, which already enforces the order of
submodule calls. Dropping both also fixes the broken `[1]` index. A cleaner
check for "custom_environment is entered before submodule runs" is
`ssh_ctx.__enter__.assert_called_once()` plus `assert_has_calls`, which you
already have.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
