-
Notifications
You must be signed in to change notification settings - Fork 46
[LCORE-462] Fix URLs in REST API sequence diagram #551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Anxhela Coba <[email protected]>
WalkthroughThe REST API documentation in docs/rest_api.uml updates endpoint names: conversation endpoints gain pluralized paths and explicit path parameter names; readiness/liveness endpoints move to root paths. Response types and other endpoints remain unchanged. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/rest_api.pngis excluded by!**/*.pngdocs/rest_api.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
docs/rest_api.uml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e_tests
- GitHub Check: build-pr
🔇 Additional comments (3)
docs/rest_api.uml (3)
21-22: Approved — /v1/conversations and {conversation_id} are used consistently across router, OpenAPI, docs, and tests; diagram is correct.
27-28: Unversioned /readiness is fine — but align docs/tests.
Code and OpenAPI use GET /readiness (src/app/endpoints/health.py:80; docs/openapi.json:618; tests/integration/test_openapi_json.py:72) while README.md documents GET /v1/readiness (README.md:708). Update README to/readinessor make the route/tests/docs versioned consistently; ensure k8s readinessProbe/ingress reference the chosen path.
24-25: DELETE handler correctly wired to /v1/conversations/{conversation_id}.
@router.delete("/conversations/{conversation_id}") exists in src/app/endpoints/conversations.py (lines 341–345) and conversations.router is mounted with prefix "/v1" in src/app/routers.py (line 33). No DELETE route found for a singular /v1/conversation.
| Client->>"API Server": GET /liveness | ||
| "API Server"-->>Client: LivenessResponse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Unversioned /liveness likewise—verify consistency across manifests and monitors.
Confirm livenessProbe, dashboards, and alerts reference /liveness (not /v1/live).
Probe alignment check:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Look for probe paths and any legacy /v1/live:"
rg -nP -C3 '(livenessProbe|/liveness|/v1/live)' || trueLength of output: 13730
Unify liveness path: /liveness vs /v1/liveness.
README.md documents GET /v1/liveness (README.md:732) while code and repo references use GET /liveness (src/app/endpoints/health.py:124, docker-compose.yaml:39, tests/integration/test_openapi_json.py:73, docs/openapi.json). Choose the canonical path and update README or align code/healthchecks/tests/dashboards/alerts to match; verify external monitors/alerts.
🤖 Prompt for AI Agents
In docs/rest_api.uml around lines 30-31 the liveness endpoint is shown as GET
/liveness but README.md documents GET /v1/liveness and other artifacts reference
/liveness; pick a canonical path (either /liveness or /v1/liveness), update this
UML to match the chosen canonical path, and then make a coordinated change
across the repo: update README.md (line ~732) or update
src/app/endpoints/health.py, docker-compose.yaml,
tests/integration/test_openapi_json.py, docs/openapi.json (and any other
references) so they all use the same path; run tests and update external
monitor/alert configurations to point to the canonical endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Fix the URLs in the REST API sequence diagram
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit