added dispatch functionality for scheduler #1447
Conversation
WalkthroughThe updates introduce a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- kairon/events/scheduler/kscheduler.py (2 hunks)
- kairon/events/server.py (1 hunks)
- tests/integration_test/event_service_test.py (2 hunks)
Additional context used
Ruff
kairon/events/server.py
144-144: Do not perform function call
Pathin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
tests/integration_test/event_service_test.py
518-518: f-string without any placeholders
Remove extraneous
fprefix(F541)
Additional comments not posted (4)
kairon/events/scheduler/kscheduler.py (2)
74-76: LGTM! Thedispatch_eventmethod is correctly implemented.The method correctly constructs and dispatches a
JobEventusing the providedevent_id.
74-76: Verify the use of the protected method_dispatch_event.Using a protected method like
_dispatch_eventcan be risky as it may change in future versions of the library. Ensure that this is the intended usage and that it won't cause issues with library updates.Run the following script to verify the usage of
_dispatch_eventin the codebase:kairon/events/server.py (1)
143-146: LGTM! Thedispatch_scheduled_eventendpoint is correctly implemented.The endpoint correctly invokes the
dispatch_eventmethod ofKSchedulerwith the providedevent_id.Tools
Ruff
144-144: Do not perform function call
Pathin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
tests/integration_test/event_service_test.py (1)
516-534: LGTM! Thetest_scheduled_event_request_dispatchfunction is correctly implemented.The test function effectively verifies the dispatching of scheduled events and checks the expected attributes of the dispatched event.
Tools
Ruff
518-518: f-string without any placeholders
Remove extraneous
fprefix(F541)
| @app.get("/api/events/dispatch/{event_id}", response_model=Response) | ||
| def dispatch_scheduled_event(event_id: Text = Path(description="Event id")): | ||
| KScheduler().dispatch_event(event_id) |
There was a problem hiding this comment.
Avoid using Path in argument defaults.
The static analysis tool suggests not using Path directly in the argument defaults. Instead, perform the call within the function or use a module-level variable.
Apply this diff to address the issue:
-def dispatch_scheduled_event(event_id: Text = Path(description="Event id")):
+def dispatch_scheduled_event(event_id: Text):
+ event_id = Path(event_id, description="Event id")Committable suggestion was skipped due to low confidence.
Tools
Ruff
144-144: Do not perform function call
Pathin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
| @patch("apscheduler.schedulers.base.BaseScheduler._dispatch_event", autospec=True) | ||
| def test_scheduled_event_request_dispatch(mock_dispatch_event): | ||
| response = client.get( | ||
| f"/api/events/dispatch/test", |
There was a problem hiding this comment.
Remove the extraneous f prefix from the f-string.
The f-string on line 518 does not contain any placeholders, so the f prefix is unnecessary.
Apply this diff to address the issue:
- f"/api/events/dispatch/test",
+ "/api/events/dispatch/test",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.
| f"/api/events/dispatch/test", | |
| "/api/events/dispatch/test", |
Tools
Ruff
518-518: f-string without any placeholders
Remove extraneous
fprefix(F541)
Summary by CodeRabbit
New Features
Tests