use FlasherClient for ridesx#536
Conversation
Also handle hashing and caching Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
WalkthroughThe changes rename the flashing parameter from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant FlasherClient
participant OpendalAdapter
User->>CLI: Run flash command with multiple --target specs (name:file)
CLI->>FlasherClient: flash({target1: file1, target2: file2}, operator, compression)
loop For each target
FlasherClient->>FlasherClient: _flash_single(file, target, operator, compression)
FlasherClient->>OpendalAdapter: open(file)
OpendalAdapter-->>FlasherClient: file stream
FlasherClient->>FlasherClient: _should_upload_file(...)
alt File needs upload
FlasherClient->>OpendalAdapter: upload file
end
FlasherClient->>FlasherClient: call flash RPC with target
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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 (2)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
29-32: Remove redundant Path conversion.
path_bufis already aPathobject after line 29, so converting it again on line 32 is redundant.- filename = Path(path_buf).name + filename = path_buf.namepackages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
584-609: Simplify control flow by removing unnecessary else.The static analysis correctly identifies that the else block is unnecessary after a return statement.
if storage_hash == src_hash: return False - else: - return True + return True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(5 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (2)
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
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.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (2)
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
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.
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
[refactor] 603-606: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[error] 622-623: Context manager 'async_generator' doesn't implement enter and exit.
(E1129)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (6)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (3)
34-42: Good use of hash-based upload detection.The implementation correctly uses the inherited
_should_upload_filemethod to avoid redundant uploads by comparing SHA-256 hashes, which is more reliable than just checking file existence.
77-93: Well-designed flexible flash interface.The method signature and implementation correctly handle both single-image and multi-partition flashing scenarios, aligning with the FlasherClient interface while maintaining backward compatibility.
104-112: Clean CLI delegation pattern.The implementation properly delegates to the parent class CLI, avoiding code duplication and ensuring consistency with the FlasherClient interface.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (3)
625-657: Well-structured multi-partition flash support.The implementation correctly handles both single and multi-partition flashing with proper validation and error handling. The use of type guards and appropriate error messages enhances the API usability.
545-570: Robust CLI implementation for multi-partition flashing.The CLI correctly handles both single-file and multi-partition flashing scenarios with proper validation and clear error messages. The
name:fileformat for partition specifications is intuitive.
730-730: Correct decorator syntax.Good fix - the parentheses are unnecessary when the decorator doesn't take any arguments.
| multiple=True, | ||
| help="name:file", | ||
| ) | ||
| @click.option("--single-partition", type=str, help="Partition name when flashing a single file") |
There was a problem hiding this comment.
is there a way to avoid this extra parameter when flashing with a single file? , may be not all flashers will support it, and that's fine...
| @click.option("--partition", type=str) | ||
| @click.argument("file", nargs=-1, required=False) | ||
| @click.option( | ||
| "--partition", |
There was a problem hiding this comment.
should we also take the chance to call them all targets and not partitions? :)
it will work better in general, and we have that notion in the flasher bundles.
There was a problem hiding this comment.
ah sure, we talked about it and i forgot, sorry!
| results: dict[str, object] = {} | ||
|
|
||
| oper_map = operator if isinstance(operator, dict) else {} | ||
| comp_map = compression if isinstance(compression, dict) else {} |
There was a problem hiding this comment.
| comp_map = compression if isinstance(compression, dict) else {} | |
| comp_map = compression if isinstance(compression, dict) else {} |
In this case I would just take the compression config (single one) and apply it to all targets, so it get's decompressed in runtime on the other side.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
583-608: Simplify the conditional logic.The hash comparison logic is sound, but there's an unnecessary
elseafterreturnthat can be simplified.- if storage_hash == src_hash: - return False - else: - return True + if storage_hash == src_hash: + return False + return True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(3 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(8 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
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.
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (2)
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
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.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (2)
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
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.
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
[refactor] 602-605: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[error] 621-622: Context manager 'async_generator' doesn't implement enter and exit.
(E1129)
[error] 665-666: Context manager 'async_generator' doesn't implement enter and exit.
(E1129)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (12)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
212-212: LGTM - Parameter rename aligns with codebase refactoring.The rename from
partitiontotargetis consistent with the broader refactoring effort to support more flexible flashing scenarios as described in the PR objectives.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (8)
9-9: LGTM - Import addition for type casting.The
castimport is appropriately used later in the code for type safety.
518-525: LGTM - Interface signature updated for multi-target support.The updated signature correctly supports both single image flashing and multi-target flashing with optional operator mappings per target.
546-570: LGTM - Enhanced CLI command for multi-target flashing.The CLI logic properly handles both single file flashing and multi-target specifications using the
name:fileformat. The validation and error handling are appropriate.
609-651: LGTM - Well-structured multi-target flashing implementation.The implementation correctly handles both single and multi-target flashing scenarios with proper validation. The operator mapping logic and error handling for incompatible parameter combinations are well implemented.
657-657: LGTM - Parameter rename for consistency.The
partitiontotargetparameter rename is consistent with the interface changes.
725-725: LGTM - Decorator correction.The change from
@base.command()to@base.commandis correct for the click library.
738-744: LGTM - Proper validation for unsupported features.The error handling correctly prevents the use of
targetparameter in StorageMuxFlasherClient, which doesn't support multiple targets.
761-767: LGTM - Consistent parameter handling in dump method.The same validation pattern is applied to the
dumpmethod for consistency.packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (3)
470-470: LGTM - CLI option rename for consistency.The rename from
--partitionto--targetaligns with the interface changes across the codebase.
482-482: LGTM - Parameter name updated consistently.The parameter name change matches the CLI option rename.
498-498: LGTM - Backward compatibility maintained.The internal call correctly maps the new
targetparameter to the existingpartitionparameter, maintaining compatibility with the internal implementation.
bennyz
force-pushed
the
flasher-interface
branch
from
96f9015 to
67046e5
Compare
2 participants
Also handle hashing and caching
address jumpstarter-dev/jumpstarter#533
jumpstarter-dev/jumpstarter#530
Summary by CodeRabbit
New Features
Improvements
--partitionto--targetfor clarity across CLI commands.Bug Fixes