Copilot commented on code in PR #61866:
URL: https://github.com/apache/airflow/pull/61866#discussion_r3066481738
##########
providers/redis/tests/unit/redis/hooks/test_redis.py:
##########
@@ -63,19 +63,27 @@ def test_get_conn_with_extra_config(self,
mock_get_connection, mock_redis):
hook = RedisHook()
hook.get_conn()
- mock_redis.assert_called_once_with(
- host=connection.host,
- username=connection.login,
- password=connection.password,
- port=connection.port,
- db=connection.extra_dejson["db"],
- ssl=connection.extra_dejson["ssl"],
- ssl_cert_reqs=connection.extra_dejson["ssl_cert_reqs"],
- ssl_ca_certs=connection.extra_dejson["ssl_ca_certs"],
- ssl_keyfile=connection.extra_dejson["ssl_keyfile"],
- ssl_certfile=connection.extra_dejson["ssl_certfile"],
- ssl_check_hostname=connection.extra_dejson["ssl_check_hostname"],
- )
+
+ call_kwargs = mock_redis.call_args[1]
+ assert call_kwargs["host"] == connection.host
+ assert call_kwargs["username"] == connection.login
+ assert call_kwargs["password"] == connection.password
+ assert call_kwargs["port"] == connection.port
+ assert call_kwargs["db"] == connection.extra_dejson["db"]
+ assert call_kwargs["ssl"] == connection.extra_dejson["ssl"]
+ assert call_kwargs["ssl_cert_reqs"] ==
connection.extra_dejson["ssl_cert_reqs"]
+ assert call_kwargs["ssl_ca_certs"] ==
connection.extra_dejson["ssl_ca_certs"]
+ assert call_kwargs["ssl_keyfile"] ==
connection.extra_dejson["ssl_keyfile"]
+ assert call_kwargs["ssl_certfile"] ==
connection.extra_dejson["ssl_certfile"]
+ assert call_kwargs["ssl_check_hostname"] ==
connection.extra_dejson["ssl_check_hostname"]
+
+ # Verify driver info is present if the installed redis-py version
supports it
+ if "driver_info" in call_kwargs:
+ driver_info = call_kwargs["driver_info"]
+ assert hasattr(driver_info, "formatted_name"), "DriverInfo should
have formatted_name attribute"
+ assert "apache-airflow" in driver_info.formatted_name
+ elif "lib_name" in call_kwargs:
+ assert "apache-airflow" in call_kwargs["lib_name"]
Review Comment:
The driver-identification assertion is currently optional: if neither
`driver_info` nor `lib_name` is present in `call_kwargs`, the test still
passes. Add a final `else: pytest.fail(...)` (or assert that at least one key
exists) so the test actually verifies the new identification behavior.
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -87,13 +93,32 @@ def get_conn(self):
self.port,
self.db,
)
+
+ # Add driver info for client identification if supported
+ # This allows Redis server to identify Airflow as the upstream
driver.
+ # See: https://redis.io/docs/latest/commands/client-setinfo/
+ driver_info_options = {}
+ try:
+ # Try to use DriverInfo class if available
+ from redis import DriverInfo
+
+ driver_info =
DriverInfo().add_upstream_driver("apache-airflow", provider_version)
Review Comment:
Importing `DriverInfo` inside `get_conn()` violates Airflow’s guideline to
keep imports at module level. Consider doing a module-level `try/except
ImportError` to set `DriverInfo` (or a sentinel) once, then reference it here,
so the function avoids runtime imports while still supporting older redis-py
versions.
##########
providers/redis/tests/unit/redis/hooks/test_redis.py:
##########
@@ -63,19 +63,27 @@ def test_get_conn_with_extra_config(self,
mock_get_connection, mock_redis):
hook = RedisHook()
hook.get_conn()
- mock_redis.assert_called_once_with(
- host=connection.host,
- username=connection.login,
- password=connection.password,
- port=connection.port,
- db=connection.extra_dejson["db"],
- ssl=connection.extra_dejson["ssl"],
- ssl_cert_reqs=connection.extra_dejson["ssl_cert_reqs"],
- ssl_ca_certs=connection.extra_dejson["ssl_ca_certs"],
- ssl_keyfile=connection.extra_dejson["ssl_keyfile"],
- ssl_certfile=connection.extra_dejson["ssl_certfile"],
- ssl_check_hostname=connection.extra_dejson["ssl_check_hostname"],
- )
+
+ call_kwargs = mock_redis.call_args[1]
+ assert call_kwargs["host"] == connection.host
Review Comment:
This test no longer asserts that `Redis(...)` was called exactly once. Add
an explicit `mock_redis.assert_called_once()` (or `assert mock_redis.call_count
== 1`) before inspecting `call_args` to prevent the test from passing if
multiple calls occur.
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -87,13 +93,32 @@ def get_conn(self):
self.port,
self.db,
)
+
+ # Add driver info for client identification if supported
+ # This allows Redis server to identify Airflow as the upstream
driver.
+ # See: https://redis.io/docs/latest/commands/client-setinfo/
+ driver_info_options = {}
+ try:
+ # Try to use DriverInfo class if available
+ from redis import DriverInfo
+
+ driver_info =
DriverInfo().add_upstream_driver("apache-airflow", provider_version)
+ driver_info_options = {"driver_info": driver_info}
+ except ImportError:
+ # Fallback to lib_name parameter if supported
+ if _SUPPORTS_LIB_NAME:
+ driver_info_options = {
+ "lib_name":
f"redis-py(apache-airflow_v{provider_version})",
Review Comment:
The client-identification metadata currently reports `apache-airflow` with
`provider_version` (e.g., 4.x), which does not match actual Apache Airflow
versions and may mislead Redis admins. Consider either using the real Airflow
version for `apache-airflow`, or changing the reported name to the provider
package (e.g. `apache-airflow-providers-redis`) if you intend to report the
provider version.
```suggestion
# This allows Redis server to identify the Redis provider as the
upstream driver.
# See: https://redis.io/docs/latest/commands/client-setinfo/
driver_info_options = {}
try:
# Try to use DriverInfo class if available
from redis import DriverInfo
driver_info = DriverInfo().add_upstream_driver(
"apache-airflow-providers-redis", provider_version
)
driver_info_options = {"driver_info": driver_info}
except ImportError:
# Fallback to lib_name parameter if supported
if _SUPPORTS_LIB_NAME:
driver_info_options = {
"lib_name":
f"redis-py(apache-airflow-providers-redis_v{provider_version})",
```
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -87,13 +93,32 @@ def get_conn(self):
self.port,
self.db,
)
+
+ # Add driver info for client identification if supported
+ # This allows Redis server to identify Airflow as the upstream
driver.
+ # See: https://redis.io/docs/latest/commands/client-setinfo/
+ driver_info_options = {}
+ try:
+ # Try to use DriverInfo class if available
+ from redis import DriverInfo
+
+ driver_info =
DriverInfo().add_upstream_driver("apache-airflow", provider_version)
+ driver_info_options = {"driver_info": driver_info}
+ except ImportError:
+ # Fallback to lib_name parameter if supported
+ if _SUPPORTS_LIB_NAME:
+ driver_info_options = {
+ "lib_name":
f"redis-py(apache-airflow_v{provider_version})",
Review Comment:
Similar to the `DriverInfo` path, the `lib_name` fallback embeds
`provider_version` while labeling it as `apache-airflow_v...`, which can be
interpreted as an Airflow version. Use a name/version combination that can’t be
confused with the Airflow core version (e.g. provider package name + provider
version, or `apache-airflow` + Airflow version).
```suggestion
"lib_name":
f"redis-py(apache-airflow-providers-redis_v{provider_version})",
```
--
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]
