tftp,http: support url sources#272
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update refactors several client classes to centralize file-server functionality. The Changes
Sequence Diagram(s)sequenceDiagram
participant User as Application
participant Client as FileServerClient
participant Server as FileServer
User->>Client: start()
User->>Client: put_file(filename, src_stream, [checksum])
Note right of Client: Optionally compute checksum internally
Client->>Server: Upload file data
User->>Client: list_files()
Client->>Server: Retrieve file list
User->>Client: stop()
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 using PR comments)
Other keywords and placeholders
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: 0
🧹 Nitpick comments (3)
packages/jumpstarter-driver-http/jumpstarter_driver_http/client.py (1)
11-11: Docstring is minimal, but sufficient.The simple docstring adequately describes the purpose of
get_url. If needed, clarify that the method delegates the call to the driver backend.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (2)
79-90: Consider handling server startup and listing errors.
start()andlist_files()methods simply forward RPC calls without explicit error handling.- If driver-level exceptions occur, consider catching them here to provide more context.
113-141: Potential security considerations when uploading from remote URLs.
- Fetching data from an arbitrary
http/httpsendpoint can pose security risks (e.g., malicious files).- Document or verify how untrusted content is handled or sanitized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-driver-http/jumpstarter_driver_http/client.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(2 hunks)packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py (1)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
🔇 Additional comments (9)
packages/jumpstarter-driver-http/jumpstarter_driver_http/client.py (2)
3-3: Inherited base class reference looks good.Importing
FileServerClienthere aligns with the new architecture consolidating file handling logic into a common client.
7-7: Dataclass inheritance approach is coherent.Switching
HttpServerClientto extendFileServerClientis consistent with the shared file server functionalities introduced across protocols. No issues found.packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py (3)
4-4: Aligned import with new base class.Importing
FileServerClientsuggests that TFTP-related file operations are now consolidated in the base class.
11-12: Class inheritance is consistent with TFTP design.Adopting
FileServerClientfollows the principle of keeping the TFTP client simple and delegating core functionality to the driver, as per retrieved learnings.
14-30: Checksum-based optimization is well-structured.
- The optional checksum parameter and conditional skip logic provide a performance boost by avoiding redundant uploads.
- Log message clarifies why the upload is skipped.
- Ensure the TFTP driver implements the
"check_file_checksum"call; otherwise, this logic will fail.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (4)
2-2: Importingurlparsebroadens protocol support.Adding
urlparsefor URL support is a solid approach. No conflicts identified.
76-78: Base class introduction is a good architectural move.Centralizing common file server actions in
FileServerClientprevents duplication and ensures consistent behavior.
91-112: Checksum usage is flexible yet requires driver support.
- Checking
hasattr(self, "check_file_checksum")ensures compatibility with drivers that may not implement this feature.- Ensure that
if "client_checksum" in self.call("put_file").__code__.co_varnames:is robust against changes in the driver. This introspection approach is somewhat fragile if the driver’s method signature evolves.
142-153: Minimal methods for deletion and server info seem sufficient.
- Methods
delete_file,get_host, andget_portare straightforward wrappers with no logic overhead.- Confirm that exceptions raised by the driver are handled consistently upstream.
| path = str(Path(source).resolve()) | ||
| filename = Path(path).name | ||
|
|
||
| with OpendalAdapter(client=self, operator=operator, path=path, mode="rb") as handle: |
There was a problem hiding this comment.
I can be wrong, but I suspect this would make the client download and push it to the exporter through the stream...
Can we verify if there's a way to have the exporter download it directly and save the extra hop?
i.e. the presigned sources.
There was a problem hiding this comment.
so it should be ok for the authless urls i'm testing this with, based on:
https://opendal.apache.org/docs/rust/src/opendal/services/http/backend.rs.html#229
There was a problem hiding this comment.
Yes that's the case here.
|
This is marked as Draft @bennyz is it still Draft? |
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
And use a base FileServer client Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
|
|
||
| CHUNK_SIZE = 4 * 1024 * 1024 | ||
|
|
||
| class FileServerClient(DriverClient): |
There was a problem hiding this comment.
Can we also generalize the driver side common stuff?
@NickCao do you have some ideas around this?
There was a problem hiding this comment.
It works the same way, just factor the file server related methods into a base class.
|
Superseded by jumpstarter-dev/jumpstarter#313 and jumpstarter-dev/jumpstarter#311 |
3 participants
And use a base FileServer client
Summary by CodeRabbit
New Features
Refactor