Copilot commented on code in PR #65866:
URL: https://github.com/apache/airflow/pull/65866#discussion_r3143386197
##########
chart/templates/secrets/redis-secrets.yaml:
##########
@@ -18,7 +18,7 @@
*/}}
# We will create these secrets (if necessary) _even if_ we aren't
-# currently using CeleryExecutor or CeleryKubernetesExecutor. As we are
+# currently using CeleryExecutor. As we are
# relying on the "pre-install" hack to prevent changing randomly generated
passwords,
Review Comment:
The top comment has become grammatically incorrect ("even if we aren't
currently using CeleryExecutor. As we are …"). Please rephrase so it reads as a
single coherent explanation (e.g., "… even if we aren't currently using
CeleryExecutor, because we rely on …").
##########
helm-tests/tests/helm_tests/airflow_core/test_api_server.py:
##########
@@ -25,9 +25,7 @@
# Everything in here needcs to set airflowVersion to get the API server to
render
def render_chart(values=None, **kwargs):
- values = values or {}
- values.setdefault("airflowVersion", "3.0.0")
- return _render_chart(values=values, **kwargs)
+ return _render_chart(values=values or {}, **kwargs)
Review Comment:
This comment is now inaccurate after removing the default airflowVersion
injection (the chart defaults already render the API server). Also there’s a
typo ("needcs"). Please either remove the comment or update it to reflect why
this wrapper exists now.
##########
chart/newsfragments/65866.significant.rst:
##########
@@ -0,0 +1,3 @@
+Dropped support for Airflow <= 3.1.0
Review Comment:
The newsfragment says support was dropped for Airflow "<= 3.1.0", but the
chart now enforces a minimum of 3.1.0 (i.e., it drops "< 3.1.0"). Please adjust
the wording to avoid implying 3.1.0 is unsupported.
```suggestion
Dropped support for Airflow < 3.1.0
```
##########
helm-tests/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1891,34 +1887,21 @@ def test_should_not_add_command_by_default(self):
assert jmespath.search("spec.containers[0].command", docs[0]) is None
@pytest.mark.parametrize(
- ("airflow_version", "workers_values", "kerberos_init_container",
"expected_config_name"),
+ ("workers_values", "kerberos_init_container", "expected_config_name"),
[
- (None, {"kerberosSidecar": {"enabled": True}}, False,
"api-server-config"),
- (None, {"kubernetes": {"kerberosSidecar": {"enabled": True}}},
False, "api-server-config"),
- (None, {"kubernetes": {"kerberosInitContainer": {"enabled":
True}}}, True, "api-server-config"),
- (None, {"kerberosInitContainer": {"enabled": True}}, True,
"api-server-config"),
- ("2.11.0", {"kubernetes": {"kerberosSidecar": {"enabled": True}}},
False, "webserver-config"),
- ("2.11.0", {"kerberosSidecar": {"enabled": True}}, False,
"webserver-config"),
- (
- "2.11.0",
- {"kubernetes": {"kerberosInitContainer": {"enabled": True}}},
- True,
- "webserver-config",
- ),
- ("2.11.0", {"kerberosInitContainer": {"enabled": True}}, True,
"webserver-config"),
+ ({"kerberosSidecar": {"enabled": True}}, False,
"api-server-config"),
+ ({"kubernetes": {"kerberosSidecar": {"enabled": True}}}, False,
"api-server-config"),
+ ({"kubernetes": {"kerberosInitContainer": {"enabled": True}}},
True, "api-server-config"),
+ ({"kerberosInitContainer": {"enabled": True}}, True,
"api-server-config"),
],
)
def test_webserver_config_for_kerberos(
- self, airflow_version, workers_values, kerberos_init_container,
expected_config_name
+ self, workers_values, kerberos_init_container, expected_config_name
):
values = {
- "airflowVersion": airflow_version,
"workers": workers_values,
"apiServer": {"apiServerConfigConfigMapName": "config"},
- "webserver": {"webserverConfigConfigMapName": "config"},
}
Review Comment:
The test name still refers to "webserver_config" even though the chart now
mounts API server config (and the expected volume is "api-server-config").
Renaming the test to reflect API server config will make intent clearer and
avoid confusion when webserver support is removed.
--
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]
