Conversation
Mesa DescriptionTL;DRAdded comprehensive file system I/O and watch capabilities to the API. Why we made these changesTo expose direct file system interaction, including read/write, directory management, and real-time file change monitoring, through the main API. What changed?
|
There was a problem hiding this comment.
What Changed
This PR introduces a new set of file system I/O endpoints under the /fs/ path. Key features include:
- Basic file and directory operations: read, write, create, delete, list, move, and set permissions.
- A real-time file system watch capability using Server-Sent Events (SSE) to stream changes (create, write, delete, rename) for a given path recursively. This is implemented in
server/cmd/api/api/fs.gousing the newfsnotifydependency. - The API service (
ApiService) is now equipped to manage these watches.
All new endpoints are documented in server/openapi.yaml and are accompanied by a comprehensive test suite in server/cmd/api/api/fs_test.go.
Risks / Concerns
This is a well-structured and comprehensive PR. The new functionality is thoroughly tested and the API contract is clearly defined in the OpenAPI spec. I don't see any immediate risks. Excellent work!
6 files reviewed | 0 comments | Review on Mesa | Edit Reviewer Settings
Co-authored-by: rgarcia2009 <rgarcia2009@gmail.com>
Co-authored-by: rgarcia2009 <rgarcia2009@gmail.com>
Co-authored-by: rgarcia2009 <rgarcia2009@gmail.com>
Co-authored-by: rgarcia2009 <rgarcia2009@gmail.com>
Co-authored-by: rgarcia2009 <rgarcia2009@gmail.com>
Co-authored-by: rgarcia2009 <rgarcia2009@gmail.com>
Co-authored-by: rgarcia2009 <rgarcia2009@gmail.com>
Co-authored-by: rgarcia2009 <rgarcia2009@gmail.com>
Co-authored-by: rgarcia2009 <rgarcia2009@gmail.com>
Sayan-
left a comment
There was a problem hiding this comment.
Overall API + flows lgtm! Some errors I think we should fix but nothing super deep.
The main question I have is whether we should configure basic allow list / deny list shaped configuration so folks don't read/mv/delete things that could cause serious issues. I don't think we can prevent event failure mode but I think we could broadly guard against the cases we care about. What do you think?
| $ref: "#/components/responses/BadRequestError" | ||
| "500": | ||
| $ref: "#/components/responses/InternalError" | ||
| # File system operations |
| async def _fetch_remote_downloads(cdp_url: str, remote_dir: str = "/tmp/downloads", local_dir: str = "downloads") -> None: | ||
| """Fetch all files from *remote_dir* (over the filesystem API) and save them to *local_dir*.""" | ||
| parsed = urlparse(cdp_url) | ||
| fs_base_url = f"https://{parsed.hostname}:444" |
There was a problem hiding this comment.
Sayan-
left a comment
There was a problem hiding this comment.
Confirming my allow list / deny list isn't blocking for this pr! Let's go 🚀 !
idea is to expose most if not all of these in the main API