Refs #38953 - Remove Candlepin Event Listener and supporting code#11652
Conversation
|
Only partially related (but Jeremy said it's best to ask you directly): do you have any plans to completely get rid of the custom Event Daemon in https://github.com/Katello/katello/blob/master/app/lib/katello/event_daemon/runner.rb, or just the Candlepin parts of it? |
Between you and me: yes ;) It could happen very quickly once this part is finished, too. |
455ad40 to
88c26e1
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
💤 Files with no reviewable changes (15)
📝 WalkthroughWalkthroughThis pull request removes the entire STOMP-based Candlepin event messaging system from Katello, including the messaging infrastructure layer, event processing services, engine configuration, and all associated tests. The stomp gem dependency is also removed from the gemspec. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Now that
Katello::Messagingand the STOMP connection have been removed, search the codebase for any remaining references toKatello::Messaging,stomp, orcandlepin_events(including settings and initializers) to avoid leaving behind dead configuration or constants. - With
candlepin_eventsremoved fromKatello::EventDaemon::Runnerservices and ping, verify there are no remaining assumptions about its presence in any generic service lists or status aggregation helpers, and clean up those references if they exist.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `Katello::Messaging` and the STOMP connection have been removed, search the codebase for any remaining references to `Katello::Messaging`, `stomp`, or `candlepin_events` (including settings and initializers) to avoid leaving behind dead configuration or constants.
- With `candlepin_events` removed from `Katello::EventDaemon::Runner` services and ping, verify there are no remaining assumptions about its presence in any generic service lists or status aggregation helpers, and clean up those references if they exist.
## Individual Comments
### Comment 1
<location path="test/models/ping_test.rb" line_range="92-101" />
<code_context>
- def test_ping_candlepin_events
</code_context>
<issue_to_address>
**suggestion (testing):** Add an automated ping test to assert that `candlepin_events` is no longer exposed as a service
Since the PR description specifies that `candlepin_events` should no longer appear in ping output, this should be enforced by an automated test rather than only manual steps. Please add or update a test (here or in a higher-level ping API spec) that calls `Katello::Ping.services`/`Katello::Ping.ping` and asserts that `:candlepin_events` is not included in the returned services, so the main behavioral change is covered by a regression test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@jeremylenz this final piece is ready for review now :) |
|
Thanks @jturel! Ping output and logs look good. Found a couple stray references: |
That's actually unrelated (and totally dead) code - that file relates to an old Katello::TaskStatus model which oddly still exists. It should all be removed, but not here :) If all else looks good, please merge when ready and I can go bother other folks to review my installer and packaging PRs! |
|
Okay, sounds good, but let's make sure there's an issue somewhere to clean up the task status stuff. |
|
@jeremylenz meta-question: you've noticed how I split this work up (the events cleanup, not the entitlement mode cleanup to be clear) against a single Redmine that you created. My intention was to break things down to make review easier but I have some observations:
Did this work for you, or would you rather have gotten this all in one big-bang PR? In any case thanks for all the reviews on this! |
|
I don't really have a strong preference either way, but I do have to admit if this had been a single PR, it probably would have been merged close to the time we merged the first PR. Internally we have been creating Jira cards for review of each, and those each come with their own overhead. But in general if the change only makes sense logically as separate PRs, don't let that stop you from doing separate ones. As far as Redmines, though, it does make things a bit simpler if it's one PR per Redmine. |
Katello is removing the Candlepin event listener infrastructure, which means candlepin_events will no longer appear in the /api/v2/ping API response. Changes: - Delete test_positive_candlepin_events_processed_by_stomp() which validated STOMP event processing (functionality no longer exists) - Update documentation comment in test_ping.py to reflect new response structure without candlepin_events Related: Katello/katello#11652 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Noted. I'll consider this an experiment I likely won't repeat given the process overhead of one Jira ticket per PR, even though they belong to the same issue. |
Katello is removing the Candlepin event listener infrastructure, which means candlepin_events will no longer appear in the /api/v2/ping API response. Changes: - Delete test_positive_candlepin_events_processed_by_stomp() which validated STOMP event processing (functionality no longer exists) - Update documentation comment in test_ping.py to reflect new response structure without candlepin_events Related: Katello/katello#11652 Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Depends on #11647 #11645
What are the changes introduced in this pull request?
Now that Katello doesn't consume events from Candlepin, all supporting code and infra can be removed
Considerations taken when implementing this change?
None, really
What are the testing steps for this pull request?
Ensure candlepin_events doesn't show in ping output
curl localhost:3000/api/v2/ping | jqConfirm
Subscribed to katello.candlepin.candlepin_eventsdoesn't show up in app logs anymoreSummary by Sourcery
Remove Candlepin event listener support and associated messaging infrastructure.
Enhancements:
Documentation:
Tests:
Summary by Sourcery
Remove Candlepin event listener support and related messaging infrastructure now that Candlepin events are no longer consumed.
Enhancements:
Tests:
Summary by CodeRabbit
Release Notes
Documentation
Chores