Fix sending mail about EOL status.#234
Merged
Merged
Conversation
Several typos fixed. Also added information in case EOL ticket is present Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Contributor
📝 WalkthroughWalkthroughThis PR bumps the container image version from 0.10.4 to 0.10.5 and refactors the eol-checker module to improve environment variable handling, fix Jira terminology ("filled" → "filed"), extend the Jira message method signature, and expand test coverage to validate the updated behavior. ChangesContainer Release and Jira Integration Refactoring
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
eol-checker/tests/test_checker.py (1)
208-221:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest does not catch the empty-string recipient added by
summary_for_images.
eol_sme_mails = {"nodejs": ["sme@redhat.com", ""]}exercises the path that appends""intodefault_mails(see checker.py Lines 193-201). The assertions only check"sme@redhat.com" in default_mailsand thedefault@redhat.comcount, so the spurious empty recipient goes unnoticed. Once the dedup fix is applied, consider asserting"" not in checker.default_mails.🤖 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/tests/test_checker.py` around lines 208 - 221, The test exposes that summary_for_images is appending empty-string recipients from eol_sme_mails into default_mails; update the implementation in summary_for_images to filter out empty or falsy recipients (e.g., when extending/adding eol_sme_mails["nodejs"] into default_mails) so only non-empty strings are appended, and then update the test (test_summary_for_images_adds_sme_mails_when_sending_email) to assert that "" not in checker.default_mails in addition to the existing assertions; reference summary_for_images, eol_sme_mails, and default_mails to locate the change.eol-checker/eol_checker/checker.py (1)
283-293:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
finally: smtp.close()can raiseUnboundLocalErrorand mask the real error.Only
SMTPRecipientsRefused/SMTPExceptionare caught, butSMTP(self.smtp_server, ...)can raise other errors (e.g.socket.gaierror,OSError) on connection failure. When the constructor raises,smtpis never bound, so thefinallyblock dereferences an undefined name and throwsUnboundLocalError, hiding the original exception.🐛 Proposed fix
+ smtp = None try: smtp = SMTP(self.smtp_server, int(self.smtp_port)) smtp.set_debuglevel(5) smtp.sendmail(send_from, self.default_mails, self.mime_msg.as_string()) except smtplib.SMTPRecipientsRefused as e: logger.error("Error sending email(SMTPRecipientsRefused): %s", e.strerror) except smtplib.SMTPException as e: logger.error("Error sending email(SMTPException): %s", e) finally: - smtp.close() + if smtp is not None: + smtp.close()🤖 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 283 - 293, The try/finally can raise UnboundLocalError if SMTP(self.smtp_server, ...) fails because smtp is never assigned; to fix, initialize smtp = None before the try (or use a context manager with smtplib.SMTP) and in the finally block only call smtp.close() if smtp is not None (or has been successfully created), ensuring the original exception is not masked; update the block around the SMTP(...) construction, the smtp variable, and the finally: smtp.close() to implement this guard.
🧹 Nitpick comments (2)
Dockerfile.eol-checker (1)
5-5: ⚡ Quick winRemove duplicate HOME environment variable definition.
The
HOMEenvironment variable is defined twice with the same value (lines 5 and 9). Remove one of the duplicate definitions to improve clarity.♻️ Proposed fix to remove duplicate
ENV VERSION="42" \ RELEASE_UPSTREAM="0.10.5" \ - HOME="/home/eol-checker" \ SUMMARY="EOL checker for SCL org projects" \ DESCRIPTION="This image is used to run EOL checker for SCL org projects in CI." \ NAME="eol-checker" \ HOME="/home/eol-checker" \ WORK_DIR="/home/nightly/ci-scripts"Also applies to: 9-9
🤖 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 `@Dockerfile.eol-checker` at line 5, The Dockerfile defines the HOME environment variable twice; remove one of the duplicate ENV entries so HOME="/home/eol-checker" appears only once. Locate the duplicate ENV definition that sets HOME (the line containing HOME="/home/eol-checker") and delete the redundant occurrence, ensuring any other ENV or RUN lines that depend on HOME remain unchanged.eol-checker/eol_checker/utils.py (1)
75-86: 💤 Low value
default_valueis now a dead, misleading parameter.After the return-type change,
default_valuecan never affect the result: whenvar_nameis set,os.getenvreturns the real value; when it is unset, the function returns[]regardless.test_get_env_variable_returns_default_when_emptyeven passes"fallback@redhat.com"and asserts[]. Consider dropping the parameter (and updating the one test call) to avoid implying a fallback that never happens.♻️ Proposed simplification
-def get_env_variable(var_name: str, default_value: str = "") -> List[str]: +def get_env_variable(var_name: str) -> List[str]: """ Get environment variable value as a list, splitting on commas. :param var_name: Name of the environment variable - :param default_value: Default value to return if environment variable is not set - :return: Value of the environment variable or default value + :return: List of values, or an empty list if unset/empty """ - if var_name in os.environ: - value = os.getenv(var_name, default_value) - if value: - return value.split(",") - return [] + value = os.getenv(var_name, "") + return value.split(",") if value else []🤖 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 75 - 86, The function get_env_variable currently has a misleading default_value parameter that never affects the result; remove the default_value parameter from the signature (change def get_env_variable(var_name: str, default_value: str = "") -> List[str] to def get_env_variable(var_name: str) -> List[str]), keep the implementation that returns env value.split(",") when var_name in os.environ and [] otherwise, and update all call sites and the test test_get_env_variable_returns_default_when_empty to stop passing the unused fallback (assert expecting [] instead). Ensure references to get_env_variable throughout the codebase and tests are updated to the new single-argument form.
🤖 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 163-167: The message string uses "not filled" while the other
branch says "already filed"—change the not-filed branch to use "Jira ticket is
not filed. Use this template: " instead of "not filled"; update the logic around
jira_id, get_jira_ticket_url, url construction and the msg variable in
eol_checker/checker.py (the send_email branch using self.send_email remains the
same) and then update the user-facing assertions in tests (test_checker.py and
any test_summary_for_images_* files) to expect the corrected "filed" wording.
- Around line 193-201: The current aggregation builds mails as a list-of-lists
(from self.eol_sme_mails[group]) and then extends self.default_mails with whole
lists, which causes inner empty strings to be added and prevents per-address
deduplication; update the loop that builds/extends self.default_mails to iterate
each address in each mail list (for mail_list in mails: for addr in mail_list),
skip blank/empty addresses (addr.strip() == ""), and only append/extend unique
addresses (check addr not in self.default_mails) so self.default_mails becomes a
flat deduplicated list of strings.
- Line 73: The constructor currently calls
os.getenv("DEFAULT_EMAILS").split(",") which crashes when DEFAULT_EMAILS is
unset; update the initialization of self.default_mails in __init__ to read the
env var with a safe default (e.g. os.getenv("DEFAULT_EMAILS", "")), split on
commas, strip whitespace from each entry and filter out any empty strings so
self.default_mails is always a list (possibly empty); reference the existing
symbol self.default_mails to locate and replace the line.
---
Outside diff comments:
In `@eol-checker/eol_checker/checker.py`:
- Around line 283-293: The try/finally can raise UnboundLocalError if
SMTP(self.smtp_server, ...) fails because smtp is never assigned; to fix,
initialize smtp = None before the try (or use a context manager with
smtplib.SMTP) and in the finally block only call smtp.close() if smtp is not
None (or has been successfully created), ensuring the original exception is not
masked; update the block around the SMTP(...) construction, the smtp variable,
and the finally: smtp.close() to implement this guard.
In `@eol-checker/tests/test_checker.py`:
- Around line 208-221: The test exposes that summary_for_images is appending
empty-string recipients from eol_sme_mails into default_mails; update the
implementation in summary_for_images to filter out empty or falsy recipients
(e.g., when extending/adding eol_sme_mails["nodejs"] into default_mails) so only
non-empty strings are appended, and then update the test
(test_summary_for_images_adds_sme_mails_when_sending_email) to assert that ""
not in checker.default_mails in addition to the existing assertions; reference
summary_for_images, eol_sme_mails, and default_mails to locate the change.
---
Nitpick comments:
In `@Dockerfile.eol-checker`:
- Line 5: The Dockerfile defines the HOME environment variable twice; remove one
of the duplicate ENV entries so HOME="/home/eol-checker" appears only once.
Locate the duplicate ENV definition that sets HOME (the line containing
HOME="/home/eol-checker") and delete the redundant occurrence, ensuring any
other ENV or RUN lines that depend on HOME remain unchanged.
In `@eol-checker/eol_checker/utils.py`:
- Around line 75-86: The function get_env_variable currently has a misleading
default_value parameter that never affects the result; remove the default_value
parameter from the signature (change def get_env_variable(var_name: str,
default_value: str = "") -> List[str] to def get_env_variable(var_name: str) ->
List[str]), keep the implementation that returns env value.split(",") when
var_name in os.environ and [] otherwise, and update all call sites and the test
test_get_env_variable_returns_default_when_empty to stop passing the unused
fallback (assert expecting [] instead). Ensure references to get_env_variable
throughout the codebase and tests are updated to the new single-argument form.
🪄 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: 7020ed95-59fc-4b2c-ad0a-c04565ab0ee5
📒 Files selected for processing (13)
.github/workflows/build-and-push.ymlDockerfile.daily-testsDockerfile.eol-checkerMakefileeol-checker/eol-checkereol-checker/eol_checker/checker.pyeol-checker/eol_checker/constants.pyeol-checker/eol_checker/custom_logger.pyeol-checker/eol_checker/jira.pyeol-checker/eol_checker/utils.pyeol-checker/tests/test_checker.pyeol-checker/tests/test_jira.pyeol-checker/tests/test_utils.py
💤 Files with no reviewable changes (1)
- eol-checker/eol-checker
| self.approaching_eol_images: dict = {} | ||
| self.os_name: str = "" | ||
| self.default_mails: List[str] = os.getenv("DEFAULT_EMAILS", "").split(",") | ||
| self.default_mails: List[str] = os.getenv("DEFAULT_EMAILS").split(",") |
Contributor
There was a problem hiding this comment.
Crash when DEFAULT_EMAILS is unset.
os.getenv("DEFAULT_EMAILS") returns None when the variable is missing, so .split(",") raises AttributeError and __init__ fails. The tests always set DEFAULT_EMAILS, masking this in CI, but the OpenShift CronJob will crash if it is not configured. Provide a default and drop empty entries.
🐛 Proposed fix
- self.default_mails: List[str] = os.getenv("DEFAULT_EMAILS").split(",")
+ self.default_mails: List[str] = [
+ mail for mail in os.getenv("DEFAULT_EMAILS", "").split(",") if mail
+ ]🤖 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` at line 73, The constructor currently
calls os.getenv("DEFAULT_EMAILS").split(",") which crashes when DEFAULT_EMAILS
is unset; update the initialization of self.default_mails in __init__ to read
the env var with a safe default (e.g. os.getenv("DEFAULT_EMAILS", "")), split on
commas, strip whitespace from each entry and filter out any empty strings so
self.default_mails is always a list (possibly empty); reference the existing
symbol self.default_mails to locate and replace the line.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #234 +/- ##
==========================================
+ Coverage 62.16% 62.45% +0.28%
==========================================
Files 13 13
Lines 1348 1345 -3
==========================================
+ Hits 838 840 +2
+ Misses 510 505 -5
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.
Several typos fixed.
Also added information in case EOL ticket is present
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests