ridesx: support http and insecure https #783
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds HTTP(S) URL handling and an Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Client as RideSXClient
participant HTTP as HTTP(S) Source
participant Storage as Storage/Operator
participant Device as Device
Note over User,Client: User invokes flash with paths and --insecure-tls
User->>Client: flash(path(s), insecure_tls)
Client->>Client: resolve targets & partitions
loop per file/partition
Client->>Client: _is_http_url(path)?
alt HTTP(S) URL
Client->>HTTP: GET (insecure_tls?)
HTTP-->>Client: bytes
Client->>Storage: write file -> storage path
Storage-->>Client: storage URL
else Local/operator path
Client->>Storage: _upload_file_if_needed(path, operator, insecure_tls)
Storage-->>Client: storage URL
end
Client->>Device: flash_images(partitions, operators, insecure_tls)
Device-->>Client: flash result
end
Client-->>User: aggregated results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Files:
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Files:
**/*.py📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Files:
🧠 Learnings (4)📚 Learning: 2025-11-05T13:33:24.741ZApplied to files:
📚 Learning: 2025-11-05T13:31:39.304ZApplied to files:
📚 Learning: 2025-03-14T16:05:14.577ZApplied to files:
📚 Learning: 2025-11-27T09:58:41.875ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (2)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:50.888Z
Learning: The TFTP driver (driver.py) already handles error cases and file path security, so the client (client.py) doesn't need to duplicate these safeguards.
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.13)
🔇 Additional comments (2)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (2)
91-126: LGTM!The
insecure_tlsparameter is properly propagated through the flash workflow.
128-167: LGTM!The
insecure_tlsparameter integration in theflashmethod is correct.
| connector = aiohttp.TCPConnector(ssl=ssl_context) | ||
| async with aiohttp.ClientSession(connector=connector) as session: | ||
| async with session.get(url) as response: | ||
| response.raise_for_status() | ||
| return await response.read() |
There was a problem hiding this comment.
Add timeout to prevent indefinite hangs.
The HTTP request has no timeout configured. If the server is slow or unresponsive, this could block indefinitely.
- connector = aiohttp.TCPConnector(ssl=ssl_context)
- async with aiohttp.ClientSession(connector=connector) as session:
- async with session.get(url) as response:
+ timeout = aiohttp.ClientTimeout(total=300) # 5 minutes
+ connector = aiohttp.TCPConnector(ssl=ssl_context)
+ async with aiohttp.ClientSession(connector=connector, timeout=timeout) as session:
+ async with session.get(url) as response:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| connector = aiohttp.TCPConnector(ssl=ssl_context) | |
| async with aiohttp.ClientSession(connector=connector) as session: | |
| async with session.get(url) as response: | |
| response.raise_for_status() | |
| return await response.read() | |
| timeout = aiohttp.ClientTimeout(total=300) # 5 minutes | |
| connector = aiohttp.TCPConnector(ssl=ssl_context) | |
| async with aiohttp.ClientSession(connector=connector, timeout=timeout) as session: | |
| async with session.get(url) as response: | |
| response.raise_for_status() | |
| return await response.read() |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py around
lines 44 to 48, the aiohttp request uses no timeout and may hang indefinitely;
add a timeout by creating an aiohttp.ClientTimeout (e.g., set total and/or
connect/read timeouts) and supply it to the ClientSession (or pass timeout=...
to session.get) so the request fails fast on slow/unresponsive servers; ensure
you handle the timeout exception (asyncio.TimeoutError or
aiohttp.ClientTimeoutError) appropriately where this function is called.
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (3)
42-48: Critical: Add timeout to prevent indefinite hangs.The aiohttp request lacks a timeout configuration. If the server is unresponsive or slow, the download could block indefinitely.
🔎 Proposed fix
+ timeout = aiohttp.ClientTimeout(total=300) # 5 minutes connector = aiohttp.TCPConnector(ssl=ssl_context) - async with aiohttp.ClientSession(connector=connector) as session: + async with aiohttp.ClientSession(connector=connector, timeout=timeout) as session: async with session.get(url) as response:
210-210: Critical: Invalid keyword argument will cause TypeError.Line 210 uses
target=Nonebut theflashmethod signature expectspartition. This will raiseTypeError: flash() got an unexpected keyword argument 'target'at runtime. Additionally, passingpartition=Nonewill fail validation at lines 145-146.🔎 Proposed fix
Add a
--partitionoption to the CLI and validate it:@base.command() @click.argument("file", nargs=-1, required=False) + @click.option("--partition", "-p", help="Partition name for single file flash") @click.option( "--target", "-t", "target_specs", multiple=True, help="name:file", ) @click.option("--insecure-tls", is_flag=True, help="Skip TLS certificate verification") - def flash(file, target_specs, insecure_tls): + def flash(file, partition, target_specs, insecure_tls): """Flash image to DUT""" if target_specs: mapping: dict[str, str] = {} for spec in target_specs: if ":" not in spec: raise click.ClickException(f"Invalid target spec '{spec}', expected name:file") name, img = spec.split(":", 1) mapping[name] = img self.flash(mapping, insecure_tls=insecure_tls) return if not file: raise click.ClickException("FILE argument is required unless --target/-t is used") - self.flash(file[0], target=None, insecure_tls=insecure_tls) + if not partition: + raise click.ClickException("--partition/-p is required when flashing a single file") + self.flash(file[0], partition=partition, insecure_tls=insecure_tls)
186-210: Minor: Multiple files silently ignored.The CLI accepts multiple files via
nargs=-1but only usesfile[0]. If users provide multiple files without--target, the extra files will be silently ignored, which could be confusing.Consider either:
- Change to
nargs=1if only single file is supported- Raise an error if
len(file) > 1without target specs🔎 Proposed fix (option 1)
- @click.argument("file", nargs=-1, required=False) + @click.argument("file", nargs=1, required=False)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
🧰 Additional context used
📓 Path-based instructions (3)
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
🧠 Learnings (4)
📚 Learning: 2025-11-05T13:33:24.741Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:476-486
Timestamp: 2025-11-05T13:33:24.741Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, the FLS flasher download in _flash_with_fls() hardcodes "aarch64-linux" architecture because the flasher always targets ARM devices currently.
Applied to files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
📚 Learning: 2025-11-05T13:31:39.304Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:100-101
Timestamp: 2025-11-05T13:31:39.304Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, the `flash()` method and its CLI command intentionally have different defaults for `fls_version`: the method defaults to `""` (empty string) while the CLI defaults to `"0.1.5"`. This is by design to provide CLI users with a convenient default while keeping the programmatic API ready for when FLS is included in the flasher images.
Applied to files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
📚 Learning: 2025-03-14T16:05:14.577Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 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)`.
Applied to files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
🔇 Additional comments (6)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (6)
1-13: LGTM!The new imports are appropriate for the HTTP(S) download functionality being added.
30-32: LGTM!The HTTP URL detection logic is straightforward and correct.
57-93: LGTM with a clarification note.The HTTP(S) download logic correctly handles insecure TLS and falls back to opendal for other schemes. Note that HTTP(S) URLs with an explicit operator will skip the aiohttp path and use opendal instead (line 63). This appears intentional but ensure it aligns with expected behavior.
95-130: LGTM!The
insecure_tlsparameter is properly propagated through the flash workflow.
132-171: LGTM!The
insecure_tlsparameter is correctly propagated toflash_images.
182-183: LGTM!Filtering out the generic flash command to provide a custom implementation is appropriate for the RideSX-specific CLI requirements.
| async with aiohttp.ClientSession(connector=connector) as session: | ||
| async with session.get(url) as response: | ||
| response.raise_for_status() | ||
| with tempfile.NamedTemporaryFile(delete=False, dir="/var/tmp") as f: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the client.py file in the jumpstarter-driver-ridesx package
find . -path "*/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py" 2>/dev/null | head -1 | xargs ls -la 2>/dev/null || echo "File path not found, searching by name..."
# Try alternative search
find . -name "client.py" -path "*ridesx*" 2>/dev/null | head -5Repository: jumpstarter-dev/jumpstarter
Length of output: 265
🏁 Script executed:
#!/bin/bash
# List files in the repository to find the right structure
git ls-files | grep -E "ridesx|client\.py" | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 1408
🏁 Script executed:
#!/bin/bash
# Look at the file around line 46
find . -path "*jumpstarter-driver-ridesx*/client.py" -exec cat -n {} + 2>/dev/null | sed -n '40,55p'Repository: jumpstarter-dev/jumpstarter
Length of output: 884
🏁 Script executed:
#!/bin/bash
# Search for all uses of NamedTemporaryFile in the codebase
rg -n "NamedTemporaryFile" --type=py -B2 -A2Repository: jumpstarter-dev/jumpstarter
Length of output: 21514
🏁 Script executed:
#!/bin/bash
# First, let's find the actual file
find . -type f -name "client.py" | xargs grep -l "NamedTemporaryFile" 2>/dev/nullRepository: jumpstarter-dev/jumpstarter
Length of output: 281
🏁 Script executed:
#!/bin/bash
# Search for the ridesx client.py file
rg "NamedTemporaryFile" --type py -lRepository: jumpstarter-dev/jumpstarter
Length of output: 970
🏁 Script executed:
#!/bin/bash
# Find all NamedTemporaryFile usages and their context
rg -n "NamedTemporaryFile" --type=py -B3 -A3 | head -60Repository: jumpstarter-dev/jumpstarter
Length of output: 5456
🏁 Script executed:
#!/bin/bash
# Specifically look for the file mentioned in the review
if [ -f "packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py" ]; then
echo "=== File exists, showing context around line 46 ==="
sed -n '40,55p' "packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py" | cat -n
else
echo "File not found at expected path"
find . -name "client.py" -path "*ridesx*" 2>/dev/null
fiRepository: jumpstarter-dev/jumpstarter
Length of output: 936
🏁 Script executed:
#!/bin/bash
# Check how tempfile is used throughout the codebase
echo "=== All tempfile uses in the codebase ==="
rg "tempfile\." --type=py | head -30Repository: jumpstarter-dev/jumpstarter
Length of output: 3959
Remove hardcoded /var/tmp directory.
The hardcoded dir="/var/tmp" parameter restricts portability unnecessarily. Python's tempfile module automatically searches through /tmp, /var/tmp, and /usr/tmp when no directory is specified, adapting to each deployment environment. Remove the dir parameter to align with the codebase pattern (all other NamedTemporaryFile calls in the repository omit this) and allow the code to work across containers, Windows, macOS, and restricted environments.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py around
line 46, the call to tempfile.NamedTemporaryFile uses a hardcoded
dir="/var/tmp", which reduces portability; remove the dir parameter so
NamedTemporaryFile uses the system default temp directory (i.e., call
tempfile.NamedTemporaryFile(...) without dir) to match the repository pattern
and allow the code to run correctly across containers, Windows, macOS, and
restricted environments.
add --insecure-tls Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
ridesx-insecure
branch
from
b17d46e to
df665fc
Compare
2 participants
add --insecure-tls
Summary by CodeRabbit
New Features
CLI
✏️ Tip: You can customize this high-level summary in your review settings.