Skip to content

Commit d5932dc

Browse files
committed
fix: address Copilot review feedback on elasticsearch9 returner
- Fix event_return() to index each event inside the loop instead of only the last one; empty events list no longer crashes - Normalize ret['data'] into ret['return'] when return is None so the payload sent to Elasticsearch contains the actual data - Fix typos in __virtual__ error messages and debug log string - Add skipif marker for ES9-specific __virtual__ test - Add tests for multiple events and empty events list
1 parent 827a616 commit d5932dc

File tree

2 files changed

+50
-9
lines changed

2 files changed

+50
-9
lines changed

src/saltext/elasticsearch/returners/elasticsearch9_mod.py

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -129,10 +129,10 @@ def __virtual__():
129129
if not HAS_ELASTICSEARCH:
130130
return (
131131
False,
132-
"Cannot load module elasticsearch: elasticsearch librarielastic not found",
132+
"Cannot load module elasticsearch: elasticsearch library not found",
133133
)
134134
if ES_MAJOR_VERSION < 9:
135-
return (False, "Cannot load the module, elasticserach version is not 9+")
135+
return (False, "Cannot load the module, elasticsearch version is not 9+")
136136

137137
return __virtualname__
138138

@@ -247,6 +247,11 @@ def returner(ret):
247247
)
248248
return
249249

250+
# If there is data but no explicit return value, normalize it so that
251+
# downstream logic that expects ret["return"] still sees the payload.
252+
if ret.get("return") is None and ret.get("data") is not None:
253+
ret["return"] = ret["data"]
254+
250255
# Build the index name
251256
if options["states_single_index"] and job_fun in STATE_FUNCTIONS:
252257
index = f"salt-{STATE_FUNCTIONS[job_fun]}"
@@ -334,7 +339,7 @@ def dst(self, _dt):
334339
}
335340

336341
if options["debug_returner_payload"]:
337-
log.debug("elasicsearch payload: %s", data)
342+
log.debug("elasticsearch payload: %s", data)
338343

339344
# Post the payload
340345
ret = __salt__["elasticsearch.document_create"](
@@ -360,11 +365,11 @@ def event_return(events):
360365
for event in events:
361366
data = {"tag": event.get("tag", ""), "data": event.get("data", "")}
362367

363-
__salt__["elasticsearch.document_create"](
364-
index=index,
365-
id_=uuid.uuid4(),
366-
document=salt.utils.json.dumps(data),
367-
)
368+
__salt__["elasticsearch.document_create"](
369+
index=index,
370+
id_=uuid.uuid4(),
371+
document=salt.utils.json.dumps(data),
372+
)
368373

369374

370375
def prep_jid(nocache=False, passed_jid=None): # pylint: disable=unused-argument

tests/unit/returners/test_elasticsearch.py

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ def test__virtual_without_elasticsearch():
9595
assert "elasticsearch" in result[1].lower()
9696

9797

98+
@pytest.mark.skipif(ES_MAJOR_VERSION < 9, reason="ES9-specific __virtual__ behavior")
9899
def test__virtual_with_old_version():
99100
"""
100101
Test __virtual__ returns False when ES_MAJOR_VERSION < 9.
@@ -272,7 +273,8 @@ def test_returner_no_data_no_return(mock_salt, default_options):
272273

273274
def test_returner_with_data_field(mock_salt, default_options):
274275
"""
275-
data is not None, should proceed even if return is None.
276+
data is not None, should proceed even if return is None, and data should
277+
be normalized into ret['return'] so the payload contains it.
276278
"""
277279
ret = {
278280
"fun": "test.ping",
@@ -289,6 +291,9 @@ def test_returner_with_data_field(mock_salt, default_options):
289291
):
290292
elasticsearch_return.returner(ret)
291293
mock_salt["elasticsearch.document_create"].assert_called_once()
294+
kwargs = mock_salt["elasticsearch.document_create"].call_args[1]
295+
doc = salt.utils.json.loads(kwargs["document"])
296+
assert doc["data"] == {"some": "data"}
292297

293298

294299
def test_returner_index_date(mock_salt, default_options, sample_ret):
@@ -536,6 +541,37 @@ def test_event_return(mock_salt, default_options):
536541
assert kwargs["index"] == "salt-master-event-cache"
537542

538543

544+
def test_event_return_multiple_events(mock_salt, default_options):
545+
"""
546+
Multiple events should each produce a document_create call.
547+
"""
548+
events = [
549+
{"tag": "salt/job/1/ret/minion1", "data": {"return": True}},
550+
{"tag": "salt/job/2/ret/minion2", "data": {"return": False}},
551+
{"tag": "salt/job/3/ret/minion3", "data": {"return": "ok"}},
552+
]
553+
with (
554+
patch.dict(elasticsearch_return.__salt__, mock_salt),
555+
patch.dict(elasticsearch_return.__opts__, {}),
556+
patch("salt.returners.get_returner_options", return_value=default_options),
557+
):
558+
elasticsearch_return.event_return(events)
559+
assert mock_salt["elasticsearch.document_create"].call_count == 3
560+
561+
562+
def test_event_return_empty(mock_salt, default_options):
563+
"""
564+
Empty events list should not call document_create and should not crash.
565+
"""
566+
with (
567+
patch.dict(elasticsearch_return.__salt__, mock_salt),
568+
patch.dict(elasticsearch_return.__opts__, {}),
569+
patch("salt.returners.get_returner_options", return_value=default_options),
570+
):
571+
elasticsearch_return.event_return([])
572+
mock_salt["elasticsearch.document_create"].assert_not_called()
573+
574+
539575
def test_event_return_with_index_date(mock_salt, default_options):
540576
"""
541577
index_date=True appends date to event index.

0 commit comments

Comments
 (0)