Copilot commented on code in PR #65006:
URL: https://github.com/apache/airflow/pull/65006#discussion_r3067846064
##########
helm-tests/tests/helm_tests/other/test_redis.py:
##########
@@ -82,14 +84,17 @@ def assert_password_and_broker_url_secrets(
assert REDIS_OBJECTS["SECRET_BROKER_URL"] not in
k8s_obj_by_key.keys()
def assert_broker_url_env(
- self, k8s_obj_by_key,
expected_broker_url_secret_name=REDIS_OBJECTS["SECRET_BROKER_URL"][1]
+ self,
+ k8s_obj_by_key,
+ expected_broker_url_secret_name=REDIS_OBJECTS["SECRET_BROKER_URL"][1],
+ airflow_fullname: str = RELEASE_NAME_REDIS,
):
broker_url_secret_in_scheduler =
self.get_broker_url_secret_in_deployment(
- k8s_obj_by_key, "StatefulSet", "worker"
+ k8s_obj_by_key, "StatefulSet", "worker", airflow_fullname
)
assert broker_url_secret_in_scheduler ==
expected_broker_url_secret_name
broker_url_secret_in_worker = self.get_broker_url_secret_in_deployment(
- k8s_obj_by_key, "Deployment", "scheduler"
+ k8s_obj_by_key, "Deployment", "scheduler", airflow_fullname
)
assert broker_url_secret_in_worker == expected_broker_url_secret_name
Review Comment:
In `assert_broker_url_env`, the variable names are swapped relative to the
actual resources being inspected ("scheduler" is read from the `worker`
StatefulSet, and "worker" is read from the `scheduler` Deployment). This is
confusing and makes future changes error-prone; please rename the variables (or
swap the assignments) so they match the workload being queried.
##########
helm-tests/tests/helm_tests/other/test_redis.py:
##########
@@ -276,6 +281,36 @@ def test_external_redis_broker_url_secret_name(self,
executor):
self.assert_broker_url_env(k8s_obj_by_key,
expected_broker_url_secret_name)
+ @pytest.mark.parametrize("executor", CELERY_EXECUTORS_PARAMS)
+ def test_redis_secrets_support_fullname_override(self, executor):
+ fullname_override = "redis-fullname-override"
+ fullname_override_objects = {
+ "NETWORK_POLICY": ("NetworkPolicy",
f"{fullname_override}-redis-policy"),
+ "SERVICE": ("Service", f"{fullname_override}-redis"),
+ "STATEFUL_SET": ("StatefulSet", f"{fullname_override}-redis"),
+ "SECRET_PASSWORD": ("Secret",
f"{fullname_override}-redis-password"),
+ "SECRET_BROKER_URL": ("Secret", f"{fullname_override}-broker-url"),
+ }
+
+ k8s_objects = render_chart(
+ RELEASE_NAME_REDIS,
+ {
+ "fullnameOverride": fullname_override,
+ "useStandardNaming": True,
+ "networkPolicies": {"enabled": True},
+ "executor": executor,
+ "redis": {"enabled": True},
+ },
+ )
+ k8s_obj_by_key = prepare_k8s_lookup_dict(k8s_objects)
+ created_redis_objects = set(fullname_override_objects.values()) &
set(k8s_obj_by_key.keys())
+ assert created_redis_objects == set(fullname_override_objects.values())
+ self.assert_broker_url_env(
+ k8s_obj_by_key,
+
expected_broker_url_secret_name=fullname_override_objects["SECRET_BROKER_URL"][1],
+ airflow_fullname="redis-fullname-override",
Review Comment:
`test_redis_secrets_support_fullname_override` defines `fullname_override`
but then passes a hard-coded string for `airflow_fullname`. Use the
`fullname_override` variable to avoid duplication and reduce the chance of the
test drifting if the value changes.
```suggestion
airflow_fullname=fullname_override,
```
##########
chart/newsfragments/65006.significant.rst:
##########
@@ -0,0 +1,3 @@
+``fernet-key``, ``redis-password`` and ``broker-url`` secrets migrated to use
standard naming as well as some other K8s resources.
+
+This likely breaks existing installations with ``useStandardNaming=True`` that
have a ``fullnameOverride != Release.Name``, ``nameOverride != Release.Name``
or use the airflow helm chart as a dependency.
Review Comment:
This newsfragment appears to overstate the scope of the change: it says
fernet-key and redis-password secrets (and "some other K8s resources") were
migrated to standard naming, but the PR changes shown are specifically about
the broker URL secret *reference* (secretKeyRef name) matching
`airflow.fullname`. Please rewrite this fragment to describe the actual
behavior change (fixing broker URL secret name resolution when
`useStandardNaming`/`fullnameOverride` cause `airflow.fullname !=
.Release.Name`) and only call out breaking behavior if there is a confirmed
upgrade impact.
```suggestion
Fixed broker URL secret name resolution to use ``airflow.fullname``
consistently.
This fixes deployments where ``useStandardNaming=True`` and
``airflow.fullname != .Release.Name``, for example when using
``fullnameOverride``, ``nameOverride``, or the Airflow Helm chart as a
dependency.
```
--
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]
