ashb commented on code in PR #73878:
URL: https://github.com/apache/airflow/pull/73878#discussion_r4132265782
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -289,7 +289,11 @@ def _clone_repo_if_required(self) -> None:
@retry(
retry=retry_if_exception_type((InvalidGitRepositoryError,
GitCommandError)),
- stop=stop_after_attempt(2),
+ # GitHub rejects a just-issued App installation token with "Repository
not found" for a
+ # few seconds. Back off between attempts so one lands after the token
has propagated,
+ # instead of failing the task on an immediate second attempt.
+ stop=stop_after_attempt(5),
+ wait=wait_exponential(multiplier=2, max=15),
Review Comment:
I don't like new config options, but I do wonder if this needs to be
configurable? If you are using an SSH key for instance then 5 attempts probably
doesn't make any difference.
So this would retry after 1s, 2s, 4s, 8s, and finally 15s before failing?
oooh better than a config value would be to somehow take the retry values
from extra params in the Airflow connection?
##########
providers/git/tests/unit/git/bundles/test_git.py:
##########
@@ -1707,12 +1714,43 @@ def
test_clone_bare_repo_invalid_repository_error_retry_fails(
with pytest.raises(InvalidGitRepositoryError, match="Invalid git
repository"):
bundle._clone_bare_repo_if_required()
- # Verify cleanup was called twice (once for each failed attempt)
- assert mock_rmtree.call_count == 2
+ # Verify cleanup was called once for each failed attempt
+ assert mock_rmtree.call_count == 5
mock_rmtree.assert_called_with(bundle.bare_repo_path)
- # Verify Repo was called twice (failed attempt + failed retry)
- assert mock_repo_class.call_count == 2
+ # Verify Repo was called once per attempt (failed attempt + four
failed retries)
+ assert mock_repo_class.call_count == 5
+
+ @mock.patch("airflow.providers.git.bundles.git.GitHook")
+ def test_clone_bare_repo_waits_out_transient_repository_not_found(self,
mock_githook):
+ """GitHub rejects a just-issued App installation token with
"Repository not found" for a few
+ seconds. The bare clone must back off and retry until it lands after
the token has propagated,
+ not fail the task on an immediate second attempt."""
+ mock_githook.return_value.repo_url = AIRFLOW_HTTPS_URL
+ mock_githook.return_value.env = {}
+ bundle = GitDagBundle(name="test", git_conn_id=CONN_HTTPS,
tracking_ref=GIT_DEFAULT_BRANCH)
+
+ attempts = []
+ sleeps = []
+
+ def _clone_from(url, to_path, bare, env):
+ attempts.append(url)
+ if len(attempts) < 3:
+ raise GitCommandError(["git", "clone"], 128, stderr="remote:
Repository not found.")
+ Repo.init(to_path, bare=True)
+
+ with (
+ mock.patch("airflow.providers.git.bundles.git.Repo.clone_from",
side_effect=_clone_from),
+ mock.patch.object(bundle, "_fetch_bare_repo"),
+ mock.patch.object(GitDagBundle._clone_bare_repo_if_required.retry,
"sleep", sleeps.append),
+ ):
+ bundle._clone_bare_repo_if_required()
+
+ assert len(attempts) == 3
+ assert bundle.bare_repo.git_dir == str(bundle.bare_repo_path)
+ assert len(sleeps) == 2, "each retry must wait for the token to become
usable"
+ assert all(s > 0 for s in sleeps)
+ assert sleeps == sorted(sleeps), "backoff must not shrink between
attempts"
Review Comment:
Honestly, I think this test is overkill and re-testing the `@retry`
decorator. I'd say to remove 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
