Merged
Conversation
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
📝 WalkthroughWalkthroughUpdates image tag from 0.8.7 to 0.8.8 in build tooling, augments nightly report logic to handle failed TMT runs via a new flag, adds tests for the TestingFarm log downloader and test deps, and removes a commented-out helper from the nightly script. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
daily_tests/daily_nightly_tests_report.py (4)
190-191:⚠️ Potential issue | 🟠 Major
self.send_emailis immediately overwritten.Line 190 reads from the environment, but line 191 unconditionally sets it to
True, making the environment variable ineffective. This appears to be debug code left in.🐛 Proposed fix—remove the hardcoded override
self.send_email = os.environ.get("SEND_EMAIL", False) - self.send_email = True🤖 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 190 - 191, The code currently reads SEND_EMAIL into self.send_email then immediately overwrites it with True; remove the hardcoded override so that self.send_email remains set from os.environ.get("SEND_EMAIL", False) (i.e., delete the line that unconditionally sets self.send_email = True) and ensure any downstream logic uses self.send_email as configured from the environment; check references to self.send_email in this class/function to confirm behavior remains correct.
186-187:⚠️ Potential issue | 🟠 MajorLocal variable shadows global
DEFAULT_MAILS.This assignment creates a local variable instead of modifying the module-level
DEFAULT_MAILSlist. The global list remains empty, so emails won't be added correctly.🐛 Proposed fix
+ global DEFAULT_MAILS if "DEFAULT_MAILS" in os.environ: DEFAULT_MAILS = os.environ["DEFAULT_MAILS"].split(",")Alternatively, use
DEFAULT_MAILS.extend()orDEFAULT_MAILS[:] = ...to modify in place.🤖 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 186 - 187, The code inside the if "DEFAULT_MAILS" in os.environ block creates a new local DEFAULT_MAILS instead of updating the module-level list; change the assignment to update the global list in place (e.g., use DEFAULT_MAILS[:] = os.environ["DEFAULT_MAILS"].split(",") or DEFAULT_MAILS.extend(os.environ["DEFAULT_MAILS"].split(","))) so the module-level DEFAULT_MAILS is actually modified; locate the if block that checks "DEFAULT_MAILS" in os.environ and replace the local assignment with one of these in-place updates.
189-189:⚠️ Potential issue | 🔴 CriticalBug:
os.environ()is not callable—useos.environ.get().
os.environis a mapping, not a function. Calling it will raiseTypeError: 'os._Environ' object is not callable.🐛 Proposed fix
- if "NIGHTLY_BUILDS_URL" in os.environ: - self.nightly_builds_url = os.environ("NIGHTLY_BUILDS_URL", "") + if "NIGHTLY_BUILDS_URL" in os.environ: + self.nightly_builds_url = os.environ.get("NIGHTLY_BUILDS_URL", "")🤖 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 189, The assignment to self.nightly_builds_url uses os.environ as a callable which raises TypeError; update the code that sets nightly_builds_url (the line initializing self.nightly_builds_url) to read the environment variable using os.environ.get("NIGHTLY_BUILDS_URL", "") or os.getenv("NIGHTLY_BUILDS_URL", "") instead so it safely returns the default empty string when the variable is missing.
489-498:⚠️ Potential issue | 🟠 Major
smtpmay be undefined infinallyblock.If the
SMTP()constructor on line 490 raises an exception,smtpis never assigned, andsmtp.close()in thefinallyblock will raiseNameError.🐛 Proposed fix
+ smtp = None try: smtp = SMTP(self.smtp_server, int(self.smtp_port)) smtp.set_debuglevel(5) smtp.sendmail(send_from, send_to, self.mime_msg.as_string()) except smtplib.SMTPRecipientsRefused as e: print(f"Error sending email(SMTPRecipientsRefused): {e.strerror}") except smtplib.SMTPException as e: print(f"Error sending email(SMTPException): {e}") finally: - smtp.close() + if smtp: + smtp.close()Or use a context manager:
with SMTP(...) as smtp:.🤖 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 489 - 498, The finally block can reference an undefined smtp if SMTP(self.smtp_server, int(self.smtp_port)) raises; update the send logic in daily_nightly_tests_report.py to either use a context manager (with SMTP(self.smtp_server, int(self.smtp_port)) as smtp:) around the sendmail call or initialize smtp = None before the try and in finally check if smtp is not None before calling smtp.close(); adjust the block that catches SMTPRecipientsRefused and SMTPException to still work with the chosen approach and ensure smtp.close() is only called when smtp exists.
🧹 Nitpick comments (1)
daily_tests/daily_nightly_tests_report.py (1)
264-267:dictionary_keymay be undefined.If
is_runningisFalseandis_failedisFalse,dictionary_keyis never set, but it's used on line 275. However, line 274 guards against this case. Consider usingeliffor clarity to make the mutual exclusion explicit.♻️ Suggested improvement
if is_running: dictionary_key = "tmt_running" - if is_failed: + elif is_failed: dictionary_key = "tmt_failed"🤖 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 264 - 267, The code sets dictionary_key based on is_running/is_failed but uses two separate ifs so dictionary_key may remain unset; change the second conditional to an elif (i.e., replace the second `if is_failed:` with `elif is_failed:`) or initialize dictionary_key to a safe default before these checks so that `dictionary_key` (used later) is always defined; update the block around the is_running/is_failed assignments (the variables `is_running`, `is_failed`, and `dictionary_key`) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@daily_tests/daily_nightly_tests_report.py`:
- Around line 248-263: The code block that builds and appends msg uses mutually
independent ifs (checking not_exists, is_running, is_failed) but never defines
msg when all flags are False, causing a NameError at
self.data_dict["tmt"]["msg"].append(msg); change the control flow to either (a)
chain the conditions with if/elif/elif and provide a final else that sets a
default message, or (b) add an early return when no flag is set; update the
block around the variables not_exists, is_running, is_failed and the append to
ensure msg is always defined before calling
self.data_dict["tmt"]["msg"].append(msg).
- Around line 328-330: The call to
store_tmt_logs_to_dict(self.store_tmt_logs_to_dict(path_dir=path_dir,
test_case=test_case, is_failed=False)) passes is_failed=False while other
boolean flags/defaults remain False, which triggers a NameError for an undefined
msg; fix by removing this call entirely if we don't want to log successful
cases, or alternatively call store_tmt_logs_to_dict with the appropriate
parameters including a valid msg (e.g., msg="passed") and any other required
flags so msg is defined (refer to the store_tmt_logs_to_dict function signature
to supply the missing argument(s)).
---
Outside diff comments:
In `@daily_tests/daily_nightly_tests_report.py`:
- Around line 190-191: The code currently reads SEND_EMAIL into self.send_email
then immediately overwrites it with True; remove the hardcoded override so that
self.send_email remains set from os.environ.get("SEND_EMAIL", False) (i.e.,
delete the line that unconditionally sets self.send_email = True) and ensure any
downstream logic uses self.send_email as configured from the environment; check
references to self.send_email in this class/function to confirm behavior remains
correct.
- Around line 186-187: The code inside the if "DEFAULT_MAILS" in os.environ
block creates a new local DEFAULT_MAILS instead of updating the module-level
list; change the assignment to update the global list in place (e.g., use
DEFAULT_MAILS[:] = os.environ["DEFAULT_MAILS"].split(",") or
DEFAULT_MAILS.extend(os.environ["DEFAULT_MAILS"].split(","))) so the
module-level DEFAULT_MAILS is actually modified; locate the if block that checks
"DEFAULT_MAILS" in os.environ and replace the local assignment with one of these
in-place updates.
- Line 189: The assignment to self.nightly_builds_url uses os.environ as a
callable which raises TypeError; update the code that sets nightly_builds_url
(the line initializing self.nightly_builds_url) to read the environment variable
using os.environ.get("NIGHTLY_BUILDS_URL", "") or
os.getenv("NIGHTLY_BUILDS_URL", "") instead so it safely returns the default
empty string when the variable is missing.
- Around line 489-498: The finally block can reference an undefined smtp if
SMTP(self.smtp_server, int(self.smtp_port)) raises; update the send logic in
daily_nightly_tests_report.py to either use a context manager (with
SMTP(self.smtp_server, int(self.smtp_port)) as smtp:) around the sendmail call
or initialize smtp = None before the try and in finally check if smtp is not
None before calling smtp.close(); adjust the block that catches
SMTPRecipientsRefused and SMTPException to still work with the chosen approach
and ensure smtp.close() is only called when smtp exists.
---
Nitpick comments:
In `@daily_tests/daily_nightly_tests_report.py`:
- Around line 264-267: The code sets dictionary_key based on
is_running/is_failed but uses two separate ifs so dictionary_key may remain
unset; change the second conditional to an elif (i.e., replace the second `if
is_failed:` with `elif is_failed:`) or initialize dictionary_key to a safe
default before these checks so that `dictionary_key` (used later) is always
defined; update the block around the is_running/is_failed assignments (the
variables `is_running`, `is_failed`, and `dictionary_key`) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7978c893-a1eb-4e01-bfb9-7f2786727a4e
📒 Files selected for processing (7)
.github/workflows/build-and-push.ymlDockerfile.daily-testsMakefiledaily_tests/daily_nightly_tests_report.pydaily_tests/tests/test_tf_log_downloader.pydaily_tests/tox.inirun_nightly_tests.sh
in case nothing is set only send to pastebin log Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@daily_tests/daily_nightly_tests_report.py`:
- Around line 163-167: The function load_mails_from_environment currently prints
the entire process environment via print(os.environ), exposing secrets; remove
that full environment dump and instead read only the specific expected variables
(e.g., lookup AUTHORIZED_EMAILS, MAIL_LIST, or whatever keys the function needs)
using os.getenv or os.environ.get, validate/parse them, and if logging is
necessary emit minimal non-sensitive info (like which keys were found or counts)
rather than the full os.environ; update references in
load_mails_from_environment to use those specific env var names and avoid
printing the whole environment.
- Around line 329-332: The loop iterates over success_logs but never uses the
loop variable suc, causing store_tmt_logs_to_dict to be called repeatedly with
the same arguments; either move the call to
store_tmt_logs_to_dict(path_dir=path_dir, test_case=test_case, is_failed=False)
out of the for suc in success_logs loop so it runs once, or if you intended
per-success-log behavior, change store_tmt_logs_to_dict to accept the individual
success entry (e.g., suc) and pass it in (update call sites and the function
signature inside store_tmt_logs_to_dict accordingly); locate and modify the loop
containing success_logs and the store_tmt_logs_to_dict invocation to implement
one of these fixes.
- Around line 254-277: The code can reference an uninitialized dictionary_key
when only not_exists is true; update the conditional logic around
not_exists/is_running/is_failed (the block that sets msg and dictionary_key) to
ensure dictionary_key is always assigned — e.g., add an explicit assignment for
the not_exists case (such as "tmt_missing") or convert the separate ifs to an
if/elif/elif chain so exactly one branch sets dictionary_key before
self.data_dict["tmt"][dictionary_key].append(test_case) is used; keep references
to test_case, msg, and log_name handling the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da049c14-ade7-493b-9e8c-0ee50eaa6273
📒 Files selected for processing (1)
daily_tests/daily_nightly_tests_report.py
Comment on lines
329
to
332
| for suc in success_logs: | ||
| self.store_tmt_logs_to_dict( | ||
| path_dir=path_dir, test_case=test_case | ||
| path_dir=path_dir, test_case=test_case, is_failed=False | ||
| ) |
There was a problem hiding this comment.
Avoid repeated pastebin uploads in success loop.
Lines [329]-[332] call store_tmt_logs_to_dict once per success log, but the call doesn’t use suc and re-sends the same TMT log repeatedly.
♻️ Suggested fix
- for suc in success_logs:
- self.store_tmt_logs_to_dict(
- path_dir=path_dir, test_case=test_case, is_failed=False
- )
+ self.store_tmt_logs_to_dict(
+ path_dir=path_dir,
+ test_case=test_case,
+ )📝 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
| for suc in success_logs: | |
| self.store_tmt_logs_to_dict( | |
| path_dir=path_dir, test_case=test_case | |
| path_dir=path_dir, test_case=test_case, is_failed=False | |
| ) | |
| self.store_tmt_logs_to_dict( | |
| path_dir=path_dir, | |
| test_case=test_case, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 329-329: Loop control variable suc not used within loop body
Rename unused suc to _suc
(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 329 - 332, The loop
iterates over success_logs but never uses the loop variable suc, causing
store_tmt_logs_to_dict to be called repeatedly with the same arguments; either
move the call to store_tmt_logs_to_dict(path_dir=path_dir, test_case=test_case,
is_failed=False) out of the for suc in success_logs loop so it runs once, or if
you intended per-success-log behavior, change store_tmt_logs_to_dict to accept
the individual success entry (e.g., suc) and pass it in (update call sites and
the function signature inside store_tmt_logs_to_dict accordingly); locate and
modify the loop containing success_logs and the store_tmt_logs_to_dict
invocation to implement one of these fixes.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
Tests
Documentation