Conversation
…. Read only. No auth needed. endpoints:
- GET raw_files/ with filtering (date, instrument_id, string includes)
1.1 Should this be a FastAPI service in a new `rest_api/` directory (similar to `mcp_server/`), with its own Dockerfile and docker-compose entry? → FastAPI + new service (Recommended)
· 1.2 For metrics: the MCP server already returns them flattened alongside each raw file (as a `metrics` dict). Should the REST API do the same, or would you prefer a separate `/metrics/`
endpoint? → Inline with raw files (Recommended)
· 1.3 Do you need pagination, or will the filtering (date, instrument, name) keep result sets small enough? → Simple limit/offset
· 1.4 For 'string includes' filtering — should this search the raw file ID/name (like the MCP server's `name_search_string`), or also other fields like `project_id`? → Separate params for
each
|
test comment - please ignore - will be replaced |
|
PLACEHOLDER - full review will follow |
|
Code Review placeholder - to be replaced |
|
Deleting test comments - sorry for the noise |
|
LICENSE.txt |
|
/home/runner/work/alphakraken/alphakraken/LICENSE.txt |
TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION
END OF TERMS AND CONDITIONS APPENDIX: How to apply the Apache License to your work. Copyright 2020 MannLabs Licensed under the Apache License, Version 2.0 (the "License"); Unless required by applicable law or agreed to in writing, software |
1 similar comment
TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION
END OF TERMS AND CONDITIONS APPENDIX: How to apply the Apache License to your work. Copyright 2020 MannLabs Licensed under the Apache License, Version 2.0 (the "License"); Unless required by applicable law or agreed to in writing, software |
Code Review: Add REST APINice addition! The overall structure is clean and follows patterns established by Bugs1. Dockerfile:
Fix: add a curl install step to the Dockerfile, or switch to a Python-based healthcheck: test: ["CMD", "python", "-c", "import urllib.request; urllib.request.urlopen('http://localhost:8090/health')"]2. Dockerfile: non-root user created after The 3. The docstring says "Metrics entries are identified by having a Code Quality4. Dead code: double legacy-field harmonization
5. Redundant
6.
Security7. No authentication The API exposes raw file metadata and metrics with no access control. If access is restricted to VPN members only, document this explicitly in 8. Port 8090 is bound on the host directly, making the service reachable without going through nginx. If nginx is the intended sole entry-point (for future auth or rate-limiting), remove the Minor / Nits9. Commented-out schema fields ( Delete the commented-out fields ( 10. FastAPI and uvicorn are pinned but pydantic is not, which could cause unexpected breakage on a fresh install. 11. No tests Even a basic unit test for Summary
Most important fixes before merging: 1 (healthcheck will always fail on slim image), 3 (incorrect metrics detection logic), 2 (Dockerfile layer ordering). The rest are improvements but not blockers. |
5909b93 to
42155c1
Compare
Add simple REST API