Copilot commented on code in PR #63813:
URL: https://github.com/apache/airflow/pull/63813#discussion_r3025338939
##########
providers/google/tests/unit/google/cloud/_internal_client/test_secret_manager_client.py:
##########
@@ -103,4 +115,19 @@ def test_get_existing_key_with_version(self,
mock_secrets_client):
)
mock_client.secret_version_path.assert_called_once_with("project_id",
"existing", "test-version")
assert secret == "result"
-
mock_client.access_secret_version.assert_called_once_with(request={"name":
"full-path"})
+ mock_client.access_secret_version.assert_called_once_with(
+ request={"name": "full-path"},
timeout=ACCESS_SECRET_VERSION_TIMEOUT
+ )
+
+ @mock.patch(INTERNAL_CLIENT_MODULE + ".SecretManagerServiceClient")
+ def test_get_secret_deadline_exceeded(self, mock_secrets_client):
+ mock_client = mock.MagicMock()
+ mock_secrets_client.return_value = mock_client
+ mock_client.secret_version_path.return_value = "full-path"
Review Comment:
This new test uses an unspecced `mock.MagicMock()`, which can mask
attribute/method typos (tests will still pass even if production code calls a
non-existent method). Prefer `autospec=True` on the patch or
`mock.MagicMock(spec=SecretManagerServiceClient)`/a narrower spec for the
client mock.
##########
providers/google/src/airflow/providers/google/cloud/_internal_client/secret_manager_client.py:
##########
@@ -97,3 +104,14 @@ def get_secret(self, secret_id: str, project_id: str,
secret_version: str = "lat
secret_id,
)
return None
+ except DeadlineExceeded:
+ self.log.warning(
+ "Google Cloud API Call Error (DeadlineExceeded): "
+ "Secret Manager request for Secret ID %s timed out after %ss.",
+ secret_id,
+ ACCESS_SECRET_VERSION_TIMEOUT,
+ )
+ raise TimeoutError(
+ f"Secret Manager request for Secret ID {secret_id} timed out "
+ f"after {ACCESS_SECRET_VERSION_TIMEOUT}s"
+ )
Review Comment:
In the `DeadlineExceeded` handler, the original gRPC exception is being
discarded. Capture it (`except DeadlineExceeded as exc`) and re-raise
`TimeoutError(...)` using exception chaining (`raise ... from exc`) so
callers/operators can still inspect the underlying cause in tracebacks.
##########
providers/google/src/airflow/providers/google/cloud/_internal_client/secret_manager_client.py:
##########
@@ -97,3 +104,14 @@ def get_secret(self, secret_id: str, project_id: str,
secret_version: str = "lat
secret_id,
)
return None
+ except DeadlineExceeded:
+ self.log.warning(
+ "Google Cloud API Call Error (DeadlineExceeded): "
+ "Secret Manager request for Secret ID %s timed out after %ss.",
+ secret_id,
+ ACCESS_SECRET_VERSION_TIMEOUT,
+ )
+ raise TimeoutError(
+ f"Secret Manager request for Secret ID {secret_id} timed out "
Review Comment:
The timeout warning (and the raised `TimeoutError` message) only includes
`secret_id`, which can be ambiguous across projects/versions. Consider logging
(and including in the exception message) `project_id` and `secret_version`, or
the fully-qualified secret version resource name, to make operational debugging
actionable.
```suggestion
"Secret Manager request for Secret ID %s (resource %s) timed
out after %ss.",
secret_id,
name,
ACCESS_SECRET_VERSION_TIMEOUT,
)
raise TimeoutError(
f"Secret Manager request for Secret ID {secret_id} (resource
{name}) timed 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
