Add support for eol-checker for Apps & Stacks images#228
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 24 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 (13)
📝 WalkthroughWalkthroughA new end-of-life container checker tool that downloads lifecycle YAML definitions, evaluates version end-dates against today's date, categorizes containers as EOL or approaching-EOL, optionally integrates with JIRA for deprecation ticket tracking, and generates consolidated text summaries with ticket URLs for reporting. ChangesEOL Checker Tool Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #228 +/- ##
===========================================
+ Coverage 35.50% 50.69% +15.18%
===========================================
Files 7 7
Lines 1011 1006 -5
===========================================
+ Hits 359 510 +151
+ Misses 652 496 -156
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Member
Author
|
How to executed? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (1)
eol-checker/tests/test_utils.py (1)
25-29: ⚡ Quick winAdd a December→January boundary test for approaching-EOL.
Current tests miss the month/year rollover case, which is a key edge case for
is_eol_version.Suggested test
def test_is_eol_version_different_year_returns_zero(): today = date(2025, 5, 15) enddate = date(2026, 5, 1) assert is_eol_version(enddate, today) == 0 + + +def test_is_eol_version_december_to_january_returns_two(): + today = date(2025, 12, 15) + enddate = date(2026, 1, 1) + assert is_eol_version(enddate, today) == 2🤖 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_utils.py` around lines 25 - 29, Add a test covering the December→January month/year rollover for the approaching-EOL logic: create a test in eol-checker/tests/test_utils.py that calls is_eol_version with today in early January and an enddate in late December of the previous year (or vice versa depending on the semantics) to verify the function returns the expected approaching-EOL value (e.g., 1 for approaching, 0 for not). Reference the is_eol_version function to implement the assertion and mirror the style of existing tests like test_is_eol_version_different_year_returns_zero so the new case exercises the month/year boundary correctly.
🤖 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 218-221: The current condition is inverted: it logs "Connection to
Jira failed" when self.jira_fetcher.jira is not None and then calls
get_jira_deprecation_details() and check_if_jira_is_filled(); change the logic
so that logger.error("Connection to Jira failed") runs only when
self.jira_fetcher.jira is None, and ensure get_jira_deprecation_details() and
check_if_jira_is_filled() are invoked only when self.jira_fetcher.jira is not
None, using the same symbols (self.jira_fetcher.jira, logger.error,
get_jira_deprecation_details, check_if_jira_is_filled) to locate the code.
- Around line 83-94: The current logic for eol_images and approaching_eol_images
overwrites a single record per (os_name, container_to_analyze) so multiple
streams are lost; change the assignments in checker.py to append stream entries
instead (e.g., ensure self.eol_images[self.os_name][self.container_to_analyze]
and self.approaching_eol_images[...] hold lists and use .append({"name":
application_stream_name, ...})) and update summary_for_eol_images and
summary_for_approaching_eol_images to iterate the per-container lists of stream
dicts rather than expecting a single dict so all lifecycle entries are preserved
and summarized.
- Around line 64-69: The code assumes lifecycle contains
"application_stream_name" and that lifecycle["enddate"] is a valid "%Y%m%d"
string; replace the direct accesses with safe checks and guarded parsing: use
lifecycle.get("application_stream_name") and if it's missing log/skip, then wrap
datetime.strptime(lifecycle["enddate"], "%Y%m%d") in a try/except ValueError
(and also check for presence of "enddate" with lifecycle.get or "in" as
currently done) to catch malformed dates, log the bad entry (including the
lifecycle payload or key) and return/skip instead of letting KeyError/ValueError
propagate; update code paths that use application_stream_name and enddate
accordingly so they only run when parsing succeeded.
- Line 39: Remove the global TLS warning suppression and re-enable certificate
verification: delete the
urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) call in
checker.py, update download_yaml() in yaml_loader.py to stop forcing
verify=False (use the default verify=True or accept a configurable CA bundle
parameter and pass it to requests.get), and adjust
eol-checker/tests/test_yaml_loader.py to mock/expect verified HTTPS calls (or
supply a test CA bundle/configuration) rather than relying on suppressed
warnings and insecure requests.
In `@eol-checker/eol_checker/constants.py`:
- Around line 26-45: CONTAINER_NAMES contains a duplicate entry "varnish" which
can cause duplicate processing; remove the redundant "varnish" string from the
CONTAINER_NAMES list (keep only one occurrence) so the list has unique container
names and downstream logic using CONTAINER_NAMES (the constant) no longer
processes varnish twice.
In `@eol-checker/eol_checker/jira.py`:
- Around line 49-52: Guard against a missing Jira client before calling
self.jira.issue by checking self.jira is not None (and optionally that
self.jira_deprecation_ticket is set) before invoking issue(); if self.jira is
None, skip the call and set self.jira_details to a safe default (e.g., empty
list or None) so downstream logic won't hit AttributeError. Update the block
referencing self.jira, self.jira_deprecation_ticket and self.jira_details to
perform this conditional check and handle the absence of a client gracefully.
- Around line 93-101: The method check_if_jira_is_filled() currently appends to
the instance list jira_deprecated_opened_issues on every call, causing
duplicates; before you start repopulating that list at the top of
check_if_jira_is_filled(), clear or reinitialize jira_deprecated_opened_issues
(e.g., set to [] or call .clear()) so each invocation starts with an empty
collection, then proceed with the existing append logic and subsequent
logger.info that reports the list.
In `@eol-checker/eol_checker/utils.py`:
- Around line 57-59: The current branch that checks isinstance(data, dict) and
returns data["lifecycles"] can return None or non-list types; update it to
validate the payload by retrieving lifecycles = data.get("lifecycles") and only
return it if isinstance(lifecycles, list) (otherwise return []); this prevents
downstream iteration errors when lifecycles is None, a string, or a dict while
keeping the existing behavior for proper lists.
- Around line 41-44: The current month-check uses today.month + 1 which breaks
on December; change the logic to compute next_month = (today.month % 12) + 1 and
next_year = today.year + (1 if today.month == 12 else 0) and then check if
enddate.year == next_year and enddate.month == next_month (the block with
enddate and today variables in eol_checker/utils.py). This preserves the
existing return values (1 for same month, 2 for next month) while correctly
handling year rollover.
In `@eol-checker/eol_checker/yaml_loader.py`:
- Line 58: The code disables TLS verification by calling requests.get(url,
timeout=30, verify=False); remove verify=False so requests.get uses the system
certificate store (e.g., requests.get(url, timeout=30)), and if you need to
support custom CA bundles expose an optional parameter (e.g., verify or
ca_bundle) on the loader function that calls requests.get and pass that through
to requests.get instead of hard-coding False; update the function that
constructs the request (the caller that assigns to response) to accept and
forward the verify option and ensure no code path sets verify=False.
In `@eol-checker/tests/test_checker.py`:
- Around line 247-253: The test test_run_fetches_jira_and_analyzes_containers
doesn't set checker.jira_fetcher.jira, so the code path that calls
get_jira_deprecation_details and check_if_jira_is_filled is never taken; update
the test to set checker.jira_fetcher.jira to a non-None value (e.g., a simple
mock object or True) before calling checker.run() so the mocked methods on
checker.jira_fetcher (get_jira_deprecation_details, check_if_jira_is_filled) are
exercised.
- Around line 126-136: The test test_summary_for_eol_images_without_jira_ticket
is not exercising the branch that calls is_jira_filled_for_container because
checker.jira_fetcher.jira is not set; update the test to set
checker.jira_fetcher.jira to a non-None/truthy value before stubbing
is_jira_filled_for_container so the code in summary_for_eol_images will call
jira_fetcher.is_jira_filled_for_container (refer to checker.jira_fetcher.jira
and the summary_for_eol_images function) and then assert the expected report
contents.
- Around line 112-123: The test
test_summary_for_eol_images_with_existing_jira_ticket doesn't set
checker.jira_fetcher.jira, so summary_for_eol_images returns early and never
calls is_jira_filled_for_container; update the test to set
checker.jira_fetcher.jira to a non-None value (e.g., a dummy object) before
calling summary_for_eol_images("RHEL9") so the code path that invokes
checker.jira_fetcher.is_jira_filled_for_container(stream_name="nodejs-18") is
exercised and the assertions about the existing Jira ticket and URL can be
validated.
- Around line 170-188: The test
test_summary_report_includes_eol_and_approaching_sections fails to exercise the
EOL path because checker.jira_fetcher.jira is left as None; set
checker.jira_fetcher.jira to a non-None value (e.g., a simple object or a
flexmock) before calling summary_report so summary_for_eol_images will call
is_jira_filled_for_container; ensure the setup is added alongside the existing
flexmock expectations for is_jira_filled_for_container so the mocked calls on
checker.jira_fetcher.is_jira_filled_for_container (stream_name="nodejs-18") are
actually invoked.
- Line 18: The tests call ContainerEolChecker(today=...), but
ContainerEolChecker.__init__ currently doesn't accept that kwarg; update
ContainerEolChecker.__init__ to accept a today parameter (e.g., def
__init__(self, today: date | None = None, **kwargs): self.today = today or
date.today()) and/or accept **kwargs so tests can pass additional fixtures, then
use self.today inside methods that compute EOL; ensure the constructor signature
and attribute name match references in eol_checker/checker.py and tests.
In `@eol-checker/tests/test_jira.py`:
- Around line 9-12: The test fixture "fetcher" creates a JiraFetcher but doesn't
seed credentials so JiraFetcher.jira returns None in CI; update the pytest
fixture "fetcher" to set JIRA_USERNAME and JIRA_PASSWORD (e.g., via os.environ
or pytest monkeypatch) before instantiating JiraFetcher and ensure you clean
up/restore them after the test; reference the JiraFetcher class and its jira
property to make the client-creation deterministic across environments.
In `@eol-checker/tests/test_yaml_loader.py`:
- Around line 6-13: The test imports a nonexistent DEFAULT_YAML_URL and assumes
a base URL while YamlLoader.get_yaml_url actually uses LIFECYCLE_DEFS_URL (or
the LIFECYCLE_DEFS_URL env); update the test to use the correct symbol or
explicitly set/mock the env value used by YamlLoader.get_yaml_url: replace
DEFAULT_YAML_URL with LIFECYCLE_DEFS_URL imported from eol_checker.constants (or
set os.environ["LIFECYCLE_DEFS_URL"] to the expected base before calling
YamlLoader.get_yaml_url) so the asserted URL matches the loader's behavior.
---
Nitpick comments:
In `@eol-checker/tests/test_utils.py`:
- Around line 25-29: Add a test covering the December→January month/year
rollover for the approaching-EOL logic: create a test in
eol-checker/tests/test_utils.py that calls is_eol_version with today in early
January and an enddate in late December of the previous year (or vice versa
depending on the semantics) to verify the function returns the expected
approaching-EOL value (e.g., 1 for approaching, 0 for not). Reference the
is_eol_version function to implement the assertion and mirror the style of
existing tests like test_is_eol_version_different_year_returns_zero so the new
case exercises the month/year boundary correctly.
🪄 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: f2a692dd-d174-499f-a1b0-4323c68e6aa7
📒 Files selected for processing (13)
eol-checker/eol_checker/__init__.pyeol-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/eol_checker/yaml_loader.pyeol-checker/tests/__init__.pyeol-checker/tests/test_checker.pyeol-checker/tests/test_jira.pyeol-checker/tests/test_utils.pyeol-checker/tests/test_yaml_loader.pyeol-checker/tox.ini
Comment on lines
+9
to
+12
| @pytest.fixture | ||
| def fetcher(): | ||
| return JiraFetcher() | ||
|
|
There was a problem hiding this comment.
Seed Jira credentials in tests to make client-creation tests deterministic.
The fixture does not set JIRA_USERNAME/JIRA_PASSWORD, but JiraFetcher.jira returns None without them, so tests that assert client creation are environment-dependent.
Suggested fix
`@pytest.fixture`
-def fetcher():
+def fetcher(monkeypatch):
+ monkeypatch.setenv("JIRA_USERNAME", "user")
+ monkeypatch.setenv("JIRA_PASSWORD", "pass")
return JiraFetcher()🤖 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_jira.py` around lines 9 - 12, The test fixture
"fetcher" creates a JiraFetcher but doesn't seed credentials so JiraFetcher.jira
returns None in CI; update the pytest fixture "fetcher" to set JIRA_USERNAME and
JIRA_PASSWORD (e.g., via os.environ or pytest monkeypatch) before instantiating
JiraFetcher and ensure you clean up/restore them after the test; reference the
JiraFetcher class and its jira property to make the client-creation
deterministic across environments.
The pull request contains several analysis. * Analysis lifecycle-defs * Analysis Jira issues if the deprecation was already filed * Summary report for maintainers. * Some variables needs to be defined before running the checker, like LIFECYCLE_DEFS_URL, JIRA_USERNAME, JIRA_PASSWORD
mentioned in lifecycle otherwise continue Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
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.
The pull request contains several analysis.
Summary by CodeRabbit
Release Notes
New Features
Tests