-
Notifications
You must be signed in to change notification settings - Fork 35
file i/o endpoints #44
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Co-authored-by: rgarcia2009 <[email protected]>
Co-authored-by: rgarcia2009 <[email protected]>
Co-authored-by: rgarcia2009 <[email protected]>
Co-authored-by: rgarcia2009 <[email protected]>
Co-authored-by: rgarcia2009 <[email protected]>
Co-authored-by: rgarcia2009 <[email protected]>
Co-authored-by: rgarcia2009 <[email protected]>
Sayan-
left a 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.
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 |
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.
🔥
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sayan-
left a 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.
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