Skip to content

Fix default mails #221

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.

Sign up for GitHub

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

Merged
phracek merged 1 commit into from
Mar 5, 2026
Merged

Fix default mails #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build-and-push.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ jobs:
registry_username: ${{ secrets.QUAY_IMAGE_SCLORG_BUILDER_USERNAME }}
registry_token: ${{ secrets.QUAY_IMAGE_SCLORG_BUILDER_TOKEN }}
dockerfile: Dockerfile.daily-tests
tag: "0.8.8"
tag: "0.8.9"
image_name: "upstream-daily-tests"
quay_application_token: ${{ secrets.QUAY_IMAGE_SCLORG_UPDATE_DESC }}
2 changes: 1 addition & 1 deletion Dockerfile.daily-tests
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ FROM quay.io/fedora/fedora:42

ENV SHARED_DIR="/var/ci-scripts" \
VERSION="42" \
RELEASE_UPSTREAM="0.8.8" \
RELEASE_UPSTREAM="0.8.9" \
UPSTREAM_TMT_REPO="https://github.com/sclorg/sclorg-testing-farm" \
UPSTREAM_TMT_DIR="sclorg-testing-farm" \
HOME="/home/nightly" \
Expand Down
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ shellcheck:
./run-shellcheck.sh `git ls-files *.sh`

build_images:
podman build -t quay.io/sclorg/upstream-daily-tests:0.8.8 -f Dockerfile.daily-tests .
podman build -t quay.io/sclorg/upstream-daily-tests:0.8.9 -f Dockerfile.daily-tests .
20 changes: 12 additions & 8 deletions daily_tests/daily_nightly_tests_report.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
from pathlib import Path
from typing import Dict, List

DEFAULT_MAILS: List[str] = []
SCLORG_MAILS = {}
SEND_PASTE_BIN = "/root/ci-scripts/send_to_paste_bin.sh"

Expand Down Expand Up @@ -121,6 +120,7 @@ def __init__(self):
self.full_success = False
self.smtp_port = 25
self.smtp_server = "smtp.redhat.com"
self.default_mails: List[str] = []
if self.args.upstream_tests:
self.available_test_case = TEST_UPSTREAM_CASES
else:
Expand Down Expand Up @@ -184,14 +184,14 @@ def load_mails_from_environment(self):
if "SMTP_PORT" in os.environ:
self.smtp_port = int(os.getenv("SMTP_PORT", 25))
if "DEFAULT_MAILS" in os.environ:
DEFAULT_MAILS = os.environ["DEFAULT_MAILS"].split(",")
self.default_mails = os.environ["DEFAULT_MAILS"].split(",")
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Sanitize DEFAULT_MAILS before storing recipients.

Line 187 can retain empty/whitespace entries (e.g., trailing commas), which may cause recipient validation failures later.

Proposed fix 
-        if "DEFAULT_MAILS" in os.environ:
-            self.default_mails = os.environ["DEFAULT_MAILS"].split(",")
+        if "DEFAULT_MAILS" in os.environ:
+            self.default_mails = [
+                m.strip()
+                for m in os.environ["DEFAULT_MAILS"].split(",")
+                if m.strip()
+            ]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
self.default_mails = os.environ["DEFAULT_MAILS"].split(",")
if "DEFAULT_MAILS" in os.environ:
self.default_mails = [
m.strip()
for m in os.environ["DEFAULT_MAILS"].split(",")
if m.strip()
]
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@daily_tests/daily_nightly_tests_report.py` at line 187, The DEFAULT_MAILS env
parsing currently does self.default_mails =
os.environ["DEFAULT_MAILS"].split(",") which can leave empty or whitespace-only
entries; update the parsing for DEFAULT_MAILS (where self.default_mails is set)
to strip whitespace from each split token and filter out any empty strings so
self.default_mails contains only valid, non-empty addresses (e.g., perform
token.strip() and keep tokens with length > 0 after stripping).

if "NIGHTLY_BUILDS_URL" in os.environ:
self.nightly_builds_url = os.environ("NIGHTLY_BUILDS_URL", "")
self.send_email = os.environ.get("SEND_EMAIL", False)
self.send_email = True

print(f"Loaded mails from environment: '{SCLORG_MAILS}'")
print(f"Default mails: '{DEFAULT_MAILS}'")
print(f"Default mails: '{self.default_mails}'")
print(f"Send email: '{self.send_email}'")

def send_file_to_pastebin(self, log_path, log_name: Path):
Expand Down Expand Up @@ -442,19 +442,23 @@ def generate_emails(self):
"""
Generate email list based on collected data and predefined email lists for each container.
"""
print("generate_emails: ", self.data_dict)
print("GENERATE_MAILS: ", self.data_dict)
print("GENERATE_MAILS: Available test cases: ", self.available_test_case)
for test_case, plan, _ in self.available_test_case:
Comment on lines +445 to 447
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix unused loop variable and reduce noisy debug output.

Line 447 has an unused plan variable (Ruff B007), and the per-item prints on Lines 452-454/461 are very chatty in nested loops.

Proposed fix 
-        print("GENERATE_MAILS: ", self.data_dict)
-        print("GENERATE_MAILS: Available test cases: ", self.available_test_case)
-        for test_case, plan, _ in self.available_test_case:
+        print("GENERATE_MAILS: starting recipient aggregation")
+        for test_case, _plan, _ in self.available_test_case:
             if test_case not in self.data_dict:
                 continue
             for _, name in self.data_dict[test_case]:
                 for cont, mails in SCLORG_MAILS.items():
-                    print(
-                        f"generate_emails: Checking if container name '{name}' starts with '{cont}'"
-                    )
                     if str(Path(name).with_suffix("")) != cont:
                         continue
                     for ml in mails:
                         if ml in self.default_mails:
                             continue
                         self.default_mails.append(ml)
-        print(f"GENERATE_MAILS: Additional emails: {self.default_mails}")
+        print(f"GENERATE_MAILS: total recipients={len(self.default_mails)}")

Also applies to: 452-454, 461-461

🧰 Tools
🪛 Ruff (0.15.2)

[warning] 447-447: Loop control variable plan not used within loop body

Rename unused plan to _plan

(B007)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@daily_tests/daily_nightly_tests_report.py` around lines 445 - 447, The loop
over self.available_test_case is unpacking into (test_case, plan, _) but plan is
unused (Ruff B007) and there are several noisy print statements; change the
unpacking to (test_case, _, _) or (test_case, *_ ) to acknowledge the unused
element, remove or replace per-item print calls with a single higher-level
debug/info log (use the module logger at debug level) or aggregate output
outside the nested loops so the method that contains available_test_case and the
prints only emits concise logging; update references in that method (e.g., where
print("GENERATE_MAILS: ", self.data_dict), print("GENERATE_MAILS: Available test
cases: ", self.available_test_case), and the inner per-item prints) accordingly.

if test_case not in self.data_dict:
continue
for _, name in self.data_dict[test_case]:
for cont, mails in SCLORG_MAILS.items():
print(
f"generate_emails: Checking if container name '{name}' starts with '{cont}'"
)
if str(Path(name).with_suffix("")) != cont:
continue
for ml in mails:
if ml in DEFAULT_MAILS:
if ml in self.default_mails:
continue
DEFAULT_MAILS.append(ml)
print(f"generate_emails: Additional emails: {DEFAULT_MAILS}")
self.default_mails.append(ml)
print(f"GENERATE_MAILS: Additional emails: {self.default_mails}")

def send_emails(self):
"""
Expand All @@ -478,7 +482,7 @@ def send_emails(self):
if self.args.upstream_tests:
send_to = SCLORG_MAILS.get("upstream-tests", [])
else:
send_to = DEFAULT_MAILS
send_to = self.default_mails

self.mime_msg["From"] = send_from
self.mime_msg["To"] = ", ".join(send_to)
Expand Down