-
Notifications
You must be signed in to change notification settings - Fork 14.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup code for elasticsearch<8 #35707
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @Taragolis to remove it altogether from the documentation, but also updating the doc here pending min version decision sounds good.
use_ssl
parameter@@ -671,14 +671,11 @@ def test_retrieve_config_keys(): | |||
""" | |||
with conf_vars( | |||
{ | |||
("elasticsearch_configs", "use_ssl"): "True", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to keep it in the tests for check that we do not use non-existed parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which parameter are you worried about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
("elasticsearch_configs", "use_ssl"): "True",
This mostly for check that we remove it from resulting arguments here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but we no longer need it because we are on elasticsearch>7
the only reference to this parameter was in the docs which was wrong
@@ -671,14 +671,11 @@ def test_retrieve_config_keys(): | |||
""" | |||
with conf_vars( | |||
{ | |||
("elasticsearch_configs", "use_ssl"): "True", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
("elasticsearch_configs", "use_ssl"): "True",
This mostly for check that we remove it from resulting arguments here
Starting
elasticsearch>=8
there is no moreuse_ssl
see https://github.com/apache/airflow/pull/33135/files#r1285347163
mentioning also open question about supporting
elasticsearch<8
#33281 (comment)
cc @sunank200 @Owen-CH-Leung
EDIT:
This PR practically reverts #33281 as provider setup is to support elasticsearch>=8
airflow/airflow/providers/elasticsearch/provider.yaml
Line 62 in 177da90
Thus users can't install older version of the library anyway.