fix(label-known-issues): clean up tempfiles

openqa-label-known-issues

@@ -1,10 +1,11 @@
 #!/usr/bin/env python3
 # Copyright SUSE LLC
-# ruff: file-ignore[blind-except, complex-structure, non-imperative-mood, line-too-long, boolean-type-hint-positional-argument, boolean-positional-value-in-call, too-many-return-statements, too-many-branches, too-many-arguments, too-many-locals, too-many-statements, too-many-positional-arguments, magic-value-comparison, try-except-pass, suppressible-exception, uncapitalized-environment-variables, print, try-consider-else, try-except-in-loop, manual-list-comprehension]
+# ruff: file-ignore[blind-except, complex-structure, non-imperative-mood, line-too-long, boolean-type-hint-positional-argument, boolean-positional-value-in-call, too-many-return-statements, too-many-branches, too-many-arguments, too-many-locals, too-many-positional-arguments, magic-value-comparison, try-except-pass, suppressible-exception, uncapitalized-environment-variables, print, try-consider-else, try-except-in-loop, manual-list-comprehension]
 """Takes an openQA job URL, looks for matching "known issues" and labels/restarts."""
 
 from __future__ import annotations
 
+import contextlib
 import json
 import logging
 import os
@@ -345,13 +346,17 @@ def handle_unreachable(
     openqa_client: OpenQAClient,
 ) -> int:
     html_out = os.environ.get("JOB_HTML_FILE")
-    temp_html_created = False
-    if not html_out:
-        fd, html_out = tempfile.mkstemp(prefix="openqa-label-known-issues--job-details-")
-        os.close(fd)
-        temp_html_created = True
+    keep_job_html_file = os.environ.get("KEEP_JOB_HTML_FILE") == "1"
+
+    with contextlib.ExitStack() as stack:
+        if not html_out:
+            tmp_file = stack.enter_context(
+                tempfile.NamedTemporaryFile(
+                    prefix="openqa-label-known-issues--job-details-", delete=not keep_job_html_file
+                )
+            )
+            html_out = tmp_file.name
 
-    try:
         try:
             head_res = client.head(testurl, follow_redirects=True)
             head_ok = head_res.status_code < 400
@@ -407,13 +412,6 @@ def handle_unreachable(
             )
             return 1
         return 0
-    finally:
-        keep_job_html_file = os.environ.get("KEEP_JOB_HTML_FILE") == "1"
-        if temp_html_created and not keep_job_html_file:
-            try:
-                pathlib.Path(html_out).unlink()
-            except Exception:
-                pass
 
 
 def handle_unreviewed(
@@ -526,13 +524,15 @@ def investigate_issue(
         return
 
     report_file = os.environ.get("REPORT_FILE")
-    temp_report_created = False
-    if not report_file:
-        fd, report_file = tempfile.mkstemp(prefix="openqa-label-known-issues--output-")
-        os.close(fd)
-        temp_report_created = True
+    keep_report_file = os.environ.get("KEEP_REPORT_FILE") == "1"
+
+    with contextlib.ExitStack() as stack:
+        if not report_file:
+            tmp_file = stack.enter_context(
+                tempfile.NamedTemporaryFile(prefix="openqa-label-known-issues--output-", delete=not keep_report_file)
+            )
+            report_file = tmp_file.name
 
-    try:
         print(f"Requesting jobs/{job_id} via openqa-cli")
         cmd_job = ["openqa-cli", *openqa_client.base_args, f"jobs/{job_id}?ancestors=1"]
         try:
@@ -604,14 +604,6 @@ def investigate_issue(
             openqa_client=openqa_client,
         )
 
-    finally:
-        keep_report_file = os.environ.get("KEEP_REPORT_FILE") == "1"
-        if temp_report_created and not keep_report_file:
-            try:
-                pathlib.Path(report_file).unlink()
-            except Exception:
-                pass
-
 
 @app.command()
 def main(

tests/test_openqa_label_known_issues.py

@@ -475,7 +475,7 @@ def test_label_on_issues_without_tickets(mocker: MockerFixture) -> None:
     assert mock_lbl.call_count == 2
 
 
-def test_handle_unreachable(mocker: MockerFixture) -> None:
+def test_handle_unreachable(mocker: MockerFixture, tmp_path: pathlib.Path) -> None:
     mock_client = MagicMock(spec=httpx.Client)
 
     # 1. testurl head failure, host_url not in testurl
@@ -560,20 +560,23 @@ def test_handle_unreachable(mocker: MockerFixture) -> None:
     )
     assert res == 0
 
-    # 8. testurl downloaded, younger than 14 days, KEEP_JOB_HTML_FILE is true (unlink skipped / test cleanup branches)
-    mocker.patch.dict("os.environ", {"KEEP_JOB_HTML_FILE": "1"})
+    # 8. testurl downloaded, younger than 14 days, JOB_HTML_FILE set
+    mocker.patch.dict("os.environ", {"JOB_HTML_FILE": str(tmp_path / "custom_html")})
     res = openqa_label_known_issues.handle_unreachable(
         "http://host.com/test", "123", mock_client, "http://host.com", []
     )
     assert res == 0
+    assert (tmp_path / "custom_html").exists()
 
-    # 9. Exception during unlink (cleanup exception branch covers line 342-343)
-    mocker.patch.dict("os.environ", {"KEEP_JOB_HTML_FILE": "0"}, clear=True)
-    mocker.patch("pathlib.Path.unlink", side_effect=Exception("unlink err"))
-    res = openqa_label_known_issues.handle_unreachable(
-        "http://host.com/test", "123", mock_client, "http://host.com", []
-    )
-    assert res == 0
+    # 9. KEEP_JOB_HTML_FILE is true without JOB_HTML_FILE sets delete=False
+    with patch("tempfile.NamedTemporaryFile") as mock_named_temp:
+        mock_named_temp.return_value.__enter__.return_value.name = str(tmp_path / "temp_html")
+        mocker.patch.dict("os.environ", {"KEEP_JOB_HTML_FILE": "1"}, clear=True)
+        res = openqa_label_known_issues.handle_unreachable(
+            "http://host.com/test", "123", mock_client, "http://host.com", []
+        )
+        assert res == 0
+        mock_named_temp.assert_called_once_with(prefix="openqa-label-known-issues--job-details-", delete=False)
 
 
 def test_handle_unreviewed(mocker: MockerFixture, tmp_path: pathlib.Path) -> None:
@@ -756,25 +759,31 @@ def test_investigate_issue(mocker: MockerFixture, tmp_path: pathlib.Path) -> Non
     )
     # unreachable matched and returns early
 
-    # 8. REPORT_FILE, KEEP_REPORT_FILE set
+    # 8. REPORT_FILE set
     mock_unreachable.return_value = 0
-    mocker.patch.dict("os.environ", {"REPORT_FILE": str(tmp_path / "custom_report"), "KEEP_REPORT_FILE": "1"})
+    mocker.patch.dict("os.environ", {"REPORT_FILE": str(tmp_path / "custom_report"), "KEEP_REPORT_FILE": "0"})
     mock_resp_log.status_code = 200
     mock_resp_log.text = "some text"
     openqa_label_known_issues.investigate_issue(
         "http://host/tests/123", mock_client, openqa_label_known_issues.OpenQAClient("http://host"), [], "http://host"
     )
     assert (tmp_path / "custom_report").exists()
 
-    # 9. Exception during report file unlink (covers lines 544-545 finally cleanup branch)
-    mocker.patch.dict("os.environ", {"REPORT_FILE": "", "KEEP_REPORT_FILE": "0"}, clear=True)
-    mocker.patch("pathlib.Path.unlink", side_effect=Exception("unlink err"))
-    openqa_label_known_issues.investigate_issue(
-        "http://host/tests/123", mock_client, openqa_label_known_issues.OpenQAClient("http://host"), [], "http://host"
-    )
+    # 9. KEEP_REPORT_FILE is true without REPORT_FILE sets delete=False
+    with patch("tempfile.NamedTemporaryFile") as mock_named_temp:
+        mock_named_temp.return_value.__enter__.return_value.name = str(tmp_path / "temp_report")
+        mocker.patch.dict("os.environ", {"KEEP_REPORT_FILE": "1"}, clear=True)
+        openqa_label_known_issues.investigate_issue(
+            "http://host/tests/123",
+            mock_client,
+            openqa_label_known_issues.OpenQAClient("http://host"),
+            [],
+            "http://host",
+        )
+        mock_named_temp.assert_called_once_with(prefix="openqa-label-known-issues--output-", delete=False)
 
     # 10. label_on_issues_without_tickets returns True (covers line 519 return)
-    mocker.patch("pathlib.Path.unlink", side_effect=None)
+    mocker.patch.dict("os.environ", {"REPORT_FILE": "", "KEEP_REPORT_FILE": "0"}, clear=True)
     mock_unreachable.return_value = 0
     mock_sub.return_value = Mock(stdout='{"job": {"state": "done", "result": "failed", "reason": "myreason"}}')
     mock_resp_log.status_code = 200