Support sending emails for eol-checker#229
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 23 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR contains two independent features: a version bump of the upstream-daily-tests Docker image to 0.10.0 across build files, and a substantial refactoring of eol-checker to support configurable email sending with consolidated Jira ticket and summary reporting. ChangesUpstream daily-tests version 0.10.0
eol-checker email and Jira reporting refactoring
Sequence Diagram(s)sequenceDiagram
participant ContainerEolChecker
participant JiraFetcher
participant summary_for_images as summary_for_images
participant SMTP_Server as SMTP Server
ContainerEolChecker->>ContainerEolChecker: check_enddate(lifecycle)
ContainerEolChecker->>ContainerEolChecker: summary_report()
loop for each OS and EOL state
ContainerEolChecker->>summary_for_images: images dict, os_name, eol_type
summary_for_images->>JiraFetcher: get Jira ticket URL/message
JiraFetcher-->>summary_for_images: ticket details or None
summary_for_images->>summary_for_images: format HTML with Jira/SME emails
summary_for_images-->>ContainerEolChecker: formatted summary text
end
ContainerEolChecker->>ContainerEolChecker: body = summary_report()
ContainerEolChecker->>ContainerEolChecker: log body
alt send_email enabled
ContainerEolChecker->>ContainerEolChecker: send_emails()
ContainerEolChecker->>SMTP_Server: configure SMTP and send HTML body
SMTP_Server-->>ContainerEolChecker: success or exception
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
eol-checker/eol_checker/utils.py (1)
89-107: 💤 Low valueEmpty environment variables produce
[""]instead of[].When an environment variable is not set,
get_env_variablereturns"", and"".split(",")produces[""](a list containing one empty string), not an empty list. While the downstream code inchecker.pyfilters out empty strings withif mail and mail not in self.default_mails, this is subtle and could cause issues if other code iterates over these lists without checking.Also, consider adding a return type annotation for consistency.
Proposed fix
-def load_mails_from_environment(): +def load_mails_from_environment() -> Dict[str, List[str]]: """ Load email addresses from environment variables. + Returns: + A dictionary mapping technology names to lists of email addresses. """ + def split_emails(var_name: str) -> List[str]: + value = get_env_variable(var_name) + return [email.strip() for email in value.split(",") if email.strip()] + sclorg_mails = {} - sclorg_mails["mariadb"] = get_env_variable("DB_SME").split(",") + sclorg_mails["mariadb"] = split_emails("DB_SME") # ... apply to other entries🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eol-checker/eol_checker/utils.py` around lines 89 - 107, load_mails_from_environment currently calls get_env_variable(...).split(",") which yields [""] for empty env vars; change load_mails_from_environment to normalize each SME value by splitting and then filtering out empty strings (e.g., ignore items that are empty or whitespace) so you return [] for unset variables, and add a return type annotation (-> Dict[str, List[str]]) to the function signature for consistency; reference load_mails_from_environment and get_env_variable and ensure compatibility with checker.py's default_mails handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eol-checker/eol_checker/checker.py`:
- Around line 298-307: The try/finally block can call smtp.close() when smtp was
never assigned and also uses a nonexistent .strerror on SMTPRecipientsRefused;
initialize smtp = None before the try, replace the SMTP(...) call and sendmail
in the try as-is, and in the finally only call smtp.close() if smtp is not None;
also change the first except to log e.recipients (or str(e)) instead of
e.strerror (reference symbols: SMTP(), smtp variable, smtp.close(), except
smtplib.SMTPRecipientsRefused).
- Line 284: The current assignment self.send_email =
bool(get_env_variable("SEND_EMAIL", "False")) is incorrect because bool("False")
is True; change it to evaluate the string value explicitly (e.g., call
get_env_variable("SEND_EMAIL", "False").strip().lower() and set self.send_email
= value in ("true","1","yes","y") or use a safe converter like
distutils.util.strtobool to convert the returned string into a real boolean) so
that the SEND_EMAIL environment variable correctly enables/disables email
sending.
- Around line 77-78: The MIMEMultipart instance (self.mime_msg) is created in
__init__ and then mutated in send_emails(), which causes duplicated headers and
accumulating attachments on repeated calls; move creation of the MIMEMultipart
object inside send_emails() (create a fresh local mime_msg each time), attach
the MIMEText body/new parts to that local mime_msg, and use it for sending so
self.mime_msg is not reused across invocations (adjust references in
send_emails() from self.mime_msg to the new local mime_msg and keep self.body
handling unchanged or make body local if appropriate).
- Around line 201-220: The code appends "Jira ticket is already filed:" to
jira_msg before checking jira_id, causing contradictory output when a ticket is
present; fix by moving that append and the report construction into the branch
that handles jira_id != "" and only build the filled-ticket message (use
self.jira_fetcher.is_jira_filled_for_container to get jira_id, then set jira_msg
= self._get_jira_msg(...)+ "Jira ticket is already filed:", compute jira_url via
get_jira_ticket_url(jira_issue_id=jira_id), format url using self.send_email,
append to report and return/continue), and keep the existing branch for jira_id
== "" to use self.jira_fetcher.jira_deprecation_ticket as before; update usages
of jira_msg, get_jira_ticket_url, jira_deprecation_ticket, send_email, and
end_line accordingly.
In `@eol-checker/eol_checker/utils.py`:
- Around line 82-85: The unconditional print of environment variables in the
block checking os.environ (the print(f"Environment variable '{var_name}':
'{value}'") call in the get/default logic) risks leaking secrets; replace that
print with a logger.debug call (e.g., use module logger =
logging.getLogger(__name__) if not present) or remove the output altogether so
the function still returns value but no sensitive data is written to stdout;
ensure you import/configure logging if you add logger.debug and retain the same
control flow around var_name, os.getenv, value and return.
- Around line 44-48: The month comparison logic using arithmetic on today.month
is broken at year boundaries; change the check that currently tests
"enddate.year == today.year and enddate.month == today.month + 1" and the
symmetric "- 1" case to use proper date arithmetic (e.g., compute a month-offset
date from today or compare year/month pairs after normalizing months by rolling
into next/previous year) so January of next year and December of previous year
are correctly detected when comparing enddate to today (use the existing
variables enddate and today and preserve the same return values 2 and 3); also
update the function docstring (the lines around the current docstring at lines
~40-41) to document the new return code 3.
In `@eol-checker/tests/test_jira.py`:
- Around line 54-61: The test uses a hardcoded password literal in the Jira
constructor expectation which triggers lint rule S106; update
test_jira_property_creates_client_once to store the password in a variable
(e.g., pwd = "secret"), use monkeypatch.setenv("JIRA_PASSWORD", pwd) and then
expect jira_module.Jira to be called with password=pwd instead of the literal,
keeping the rest of the flexmock expectation (url=fetcher.jira_url,
username="user@redhat.com") and the mock_client return intact.
---
Nitpick comments:
In `@eol-checker/eol_checker/utils.py`:
- Around line 89-107: load_mails_from_environment currently calls
get_env_variable(...).split(",") which yields [""] for empty env vars; change
load_mails_from_environment to normalize each SME value by splitting and then
filtering out empty strings (e.g., ignore items that are empty or whitespace) so
you return [] for unset variables, and add a return type annotation (->
Dict[str, List[str]]) to the function signature for consistency; reference
load_mails_from_environment and get_env_variable and ensure compatibility with
checker.py's default_mails handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b1264eb-fd5c-4f5a-b8ef-10ca28e61098
📒 Files selected for processing (7)
.github/workflows/build-and-push.ymlDockerfile.daily-testsMakefileeol-checker/eol_checker/checker.pyeol-checker/eol_checker/utils.pyeol-checker/tests/test_checker.pyeol-checker/tests/test_jira.py
Comment on lines
+77
to
+78
| self.mime_msg = MIMEMultipart() | ||
| self.body = "" |
There was a problem hiding this comment.
MIMEMultipart instance reuse may cause issues on repeated sends.
self.mime_msg is initialized once in __init__ but modified in send_emails(). If send_emails() is called multiple times (or if the checker instance is reused), headers like From, To, and Subject will be duplicated, and multiple MIMEText attachments will accumulate. Consider creating the MIMEMultipart instance inside send_emails() instead.
Proposed fix
def __init__(self, send_email: bool = False):
# ... other initialization ...
- self.mime_msg = MIMEMultipart()
self.body = ""
def send_emails(self):
# ... setup code ...
+ mime_msg = MIMEMultipart()
send_from = "phracek@redhat.com"
send_to = self.default_mails
- self.mime_msg["From"] = send_from
- self.mime_msg["To"] = ", ".join(send_to)
- self.mime_msg["Subject"] = "Container EOL Checker Report"
+ mime_msg["From"] = send_from
+ mime_msg["To"] = ", ".join(send_to)
+ mime_msg["Subject"] = "Container EOL Checker Report"
# ... logging ...
- self.mime_msg.attach(MIMEText(self.body, "html"))
+ mime_msg.attach(MIMEText(self.body, "html"))
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())
+ smtp.sendmail(send_from, send_to, mime_msg.as_string())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@eol-checker/eol_checker/checker.py` around lines 77 - 78, The MIMEMultipart
instance (self.mime_msg) is created in __init__ and then mutated in
send_emails(), which causes duplicated headers and accumulating attachments on
repeated calls; move creation of the MIMEMultipart object inside send_emails()
(create a fresh local mime_msg each time), attach the MIMEText body/new parts to
that local mime_msg, and use it for sending so self.mime_msg is not reused
across invocations (adjust references in send_emails() from self.mime_msg to the
new local mime_msg and keep self.body handling unchanged or make body local if
appropriate).
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
phracek
force-pushed
the
support_for_mails
branch
from
3067e33 to
724894d
Compare
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #229 +/- ##
==========================================
+ Coverage 50.69% 60.31% +9.61%
==========================================
Files 7 13 +6
Lines 1006 1333 +327
==========================================
+ Hits 510 804 +294
- Misses 496 529 +33
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Support sending emails for eol-checker
Summary by CodeRabbit
New Features
Tests
Chores