Miretpl commented on code in PR #68043:
URL: https://github.com/apache/airflow/pull/68043#discussion_r3374433617
##########
chart/newsfragments/68043.significant.rst:
##########
@@ -0,0 +1,12 @@
+Deprecated elements from ``ingress`` have been removed.
+
+* ``ingress.enabled`` has been replaced. There are now separate flags to
control the API server and
+ flower individually, ``ingress.web.enabled`` and ``ingress.flower.enabled``.
+* ``ingress.apiServer.host`` has been renamed to ``ingress.apiServer.hosts``
and is now an array.
+* ``ingress.apiServer.tls`` has been renamed to
``ingress.apiServer.hosts[*].tls`` and can be set per host.
+* ``ingress.web.host`` has been renamed to ``ingress.web.hosts``
+* ``ingress.web.tls`` has been renamed to ``ingress.web.hosts[*].tls``
+* ``ingress.flower.host`` has been renamed to ``ingress.flower.hosts``
+* ``ingress.flower.tls`` has been renamed to ``ingress.flower.hosts[*].tls``
+* ``ingress.statsd.host`` has been renamed to ``ingress.statsd.hosts`` and is
now an array.
+* ``ingress.pgbouncer.host`` has been renamed to ``ingress.pgbouncer.hosts``
and is now an array.
Review Comment:
```suggestion
* ``ingress.apiServer.host`` has been renamed to ``ingress.apiServer.hosts``
and is now an array.
* ``ingress.apiServer.tls`` has been renamed to
``ingress.apiServer.hosts[*].tls`` and can be set per host.
* ``ingress.web.host`` has been renamed to ``ingress.web.hosts`` and is now
an array.
* ``ingress.web.tls`` has been renamed to ``ingress.web.hosts[*].tls`` and
can be set per host.
* ``ingress.flower.host`` has been renamed to ``ingress.flower.hosts`` and
is now an array.
* ``ingress.flower.tls`` has been renamed to ``ingress.flower.hosts[*].tls``
and can be set per host.
* ``ingress.statsd.host`` has been renamed to ``ingress.statsd.hosts`` and
is now an array.
* ``ingress.pgbouncer.host`` has been renamed to ``ingress.pgbouncer.hosts``
and is now an array.
```
##########
chart/tests/helm_tests/airflow_aux/test_basic_helm_chart.py:
##########
@@ -227,7 +227,7 @@ def test_labels_are_valid(self, executor):
},
"pgbouncer": {"enabled": True},
"redis": {"enabled": True},
- "ingress": {"enabled": True},
+ "ingress": {"flower": {"enabled": True}},
Review Comment:
```suggestion
"ingress": {"flower": {"enabled": True}, "apiServer":
{"enabled": True},
```
But I think that this test should be rewritten, as basically it will pass
with warnings if something changes, which can lead to the introduction of some
breaking changes without notice by the reviewer (probably for the follow-up PR).
##########
chart/tests/helm_tests/apiserver/test_ingress_apiserver.py:
##########
@@ -133,41 +62,20 @@ def test_should_ingress_host_entry_not_exist(self):
assert not jmespath.search("spec.rules[*].host", docs[0])
@pytest.mark.parametrize(
- ("global_value", "api_server_value", "expected"),
+ ("api_server_value", "expected"),
[
- (None, None, False),
- (None, False, False),
- (None, True, True),
- (False, None, False),
- (True, None, True),
- (False, True, True), # We will deploy it if _either_ are true
- (True, False, True),
+ (None, False),
+ (False, False),
+ (True, True),
],
)
- def test_ingress_created(self, global_value, api_server_value, expected):
- values = {"ingress": {}}
- if global_value is not None:
- values["ingress"]["enabled"] = global_value
+ def test_ingress_created(self, api_server_value, expected):
+ values = {}
if api_server_value is not None:
- values["ingress"]["apiServer"] = {"enabled": api_server_value}
- if values["ingress"] == {}:
- del values["ingress"]
+ values["ingress"] = {"apiServer": {"enabled": api_server_value}}
docs = render_chart(values=values,
show_only=["templates/api-server/api-server-ingress.yaml"])
assert expected == (len(docs) == 1)
- def test_should_add_component_specific_labels(self):
- docs = render_chart(
- values={
- "ingress": {"enabled": True},
Review Comment:
I think we should modify this test case like:
```python
"ingress": {"apiServer": {"enabled": True}},
```
instead of:
```python
"ingress": {"enabled": True},
```
and not deleting it.
##########
chart/values.schema.json:
##########
Review Comment:
This list now has only one element, so I guess we could also change the
`oneOf` too
##########
chart/tests/helm_tests/apiserver/test_ingress_apiserver.py:
##########
@@ -133,41 +62,20 @@ def test_should_ingress_host_entry_not_exist(self):
assert not jmespath.search("spec.rules[*].host", docs[0])
@pytest.mark.parametrize(
- ("global_value", "api_server_value", "expected"),
+ ("api_server_value", "expected"),
[
- (None, None, False),
- (None, False, False),
- (None, True, True),
- (False, None, False),
- (True, None, True),
- (False, True, True), # We will deploy it if _either_ are true
- (True, False, True),
+ (None, False),
+ (False, False),
+ (True, True),
Review Comment:
```suggestion
(None, False),
(None, True),
(False, False),
(True, True),
```
to have full coverage.
--
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]
