add podman socket support for one-click updates#208
Open
Conversation
Detect Podman sockets for one-click updates, connect through the resolved runtime socket, and parse Podman container IDs from mountinfo. Update UI/docs wording so update guidance covers both Docker and Podman.
WalkthroughAdds generic container runtime socket support across update flows and docs (Docker and Podman rootful/rootless). Updates UI copy, implements socket discovery and connection logic, enhances container ID detection, and introduces a context-inspection feature with new tools and APIs for channel context snapshots. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
src/update.rs
Outdated
| "Mount /var/run/docker.sock to enable one-click updates.".to_string(), | ||
| ), | ||
| cannot_apply_reason: Some(format!( | ||
| "Mount a container runtime socket to enable one-click updates (Docker: {DOCKER_ROOTFUL_SOCKET}, Podman: {PODMAN_ROOTFUL_SOCKET})." |
Contributor
There was a problem hiding this comment.
Minor: the missing-socket hint doesn’t mention rootless Podman (where the host socket is usually ${XDG_RUNTIME_DIR}/podman/podman.sock mounted to /run/podman/podman.sock). Might be worth including that in this message since it’s the first thing users see.
Suggested change
| "Mount a container runtime socket to enable one-click updates (Docker: {DOCKER_ROOTFUL_SOCKET}, Podman: {PODMAN_ROOTFUL_SOCKET})." | |
| "Mount a container runtime socket to enable one-click updates (Docker: {DOCKER_ROOTFUL_SOCKET}, Podman: {PODMAN_ROOTFUL_SOCKET}, Podman rootless: $XDG_RUNTIME_DIR/podman/podman.sock -> {PODMAN_ROOTFUL_SOCKET})." |
src/update.rs
Outdated
| }; | ||
|
|
||
| bollard::Docker::connect_with_socket(socket_path, 120, &client_version) | ||
| .map_err(|error| anyhow::anyhow!("failed to connect to container runtime: {error}")) |
Contributor
There was a problem hiding this comment.
Including the socket_path in this error makes the UI/API message a lot more actionable when multiple sockets are present.
Suggested change
| .map_err(|error| anyhow::anyhow!("failed to connect to container runtime: {error}")) | |
| bollard::Docker::connect_with_socket(socket_path, 120, &client_version) | |
| .map_err(|error| anyhow::anyhow!("failed to connect to container runtime at {socket_path}: {error}")) |
src/update.rs
Outdated
| fn parse_container_id_from_mountinfo(content: &str) -> Option<String> { | ||
| for (marker, offset) in [ | ||
| ("/docker/containers/", 19usize), | ||
| ("/overlay-containers/", 20usize), |
Contributor
There was a problem hiding this comment.
Small readability win: you can drop the magic offsets and use marker.len() (less chance of drift if the markers change).
Suggested change
| ("/overlay-containers/", 20usize), | |
| for marker in ["/docker/containers/", "/overlay-containers/"] { | |
| for line in content.lines() { | |
| if let Some(position) = line.find(marker) { | |
| let after_marker = &line[position + marker.len()..]; | |
| if let Some(end_index) = after_marker.find('/') { | |
| let id = &after_marker[..end_index]; | |
| if id.len() == 64 && id.chars().all(|character| character.is_ascii_hexdigit()) { | |
| return Some(id.to_string()); | |
| } | |
| } | |
| } | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docker.md (1)
204-204:⚠️ Potential issue | 🟡 MinorStale parenthetical "(Docker)" — should mention Podman too.
Line 204 still reads "one-click controls (Docker)" but the PR broadens one-click support to Podman as well. Consider updating to "(Docker / Podman)" for consistency with the rest of this section.
Proposed fix
-The web dashboard also includes **Settings → Updates** with status details, one-click controls (Docker), and manual command snippets. +The web dashboard also includes **Settings → Updates** with status details, one-click controls (Docker / Podman), and manual command snippets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docker.md` at line 204, Update the wording in the "Settings → Updates" description: replace the parenthetical "(Docker)" with "(Docker / Podman)" so the line that reads "one-click controls (Docker)" becomes "one-click controls (Docker / Podman)"; ensure the same change is applied to any identical phrasing in the "Settings → Updates" section for consistency with the rest of the doc.
🧹 Nitpick comments (3)
src/update.rs (3)
586-596:.ok()onremove_containerresult — guideline violation in shutdown path.The coding guidelines say "no let _ = on Results … Only exception is .ok() on channel sends." This is on a Docker API call, not a channel send. The shutdown context makes this reasonable, but consider at least logging the error for debuggability.
Proposed fix
docker .remove_container( &container_id, Some(bollard::container::RemoveContainerOptions { force: true, ..Default::default() }), ) .await - .ok(); // Best effort — we're shutting down + .inspect_err(|error| tracing::debug!(%error, "best-effort removal of old container failed")) + .ok(); // Best effort — process is terminatingAs per coding guidelines, "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors. Only exception is .ok() on channel sends where the receiver may be dropped."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/update.rs` around lines 586 - 596, The call to docker.remove_container(...).await.ok() silently discards errors; instead capture the Result from remove_container (the call around container_id in the shutdown/stop path) and log any Err with contextual information (container_id and that we're shutting down) before continuing; do not propagate the error, but ensure it is at least recorded via the project's logger (e.g., warn/error) so failures to remove the container are visible for debugging.
119-123: Minor inconsistency:new_shared_statuserror message differs fromdetect_apply_capability.
new_shared_statussays "Mount a container runtime socket to enable one-click updates." whiledetect_apply_capability(line 327) includes the specific socket paths. Consider using the more helpful message with paths in both places, or extracting a shared constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/update.rs` around lines 119 - 123, The message in new_shared_status ("Mount a container runtime socket to enable one-click updates.") is less informative than detect_apply_capability which lists specific socket paths; unify them by extracting a shared constant string (e.g., CONTAINER_SOCKET_HINT) or by calling the same helper used in detect_apply_capability (or using resolve_container_socket() output) so both new_shared_status and detect_apply_capability return the identical detailed message including the socket paths; update the code in new_shared_status to use that constant/helper (referencing new_shared_status, detect_apply_capability, and resolve_container_socket) so the messages remain consistent.
430-434: Abbreviated error variablee— useerrorper coding guidelines.Multiple closures in this function use
|e|instead of|error|. This pattern repeats on lines 433, 539, 556, 567, 573, and 584.Example fix (apply to all similar closures)
- .map_err(|e| anyhow::anyhow!("failed to inspect container: {}", e))?; + .map_err(|error| anyhow::anyhow!("failed to inspect container: {}", error))?;As per coding guidelines, "Don't abbreviate variable names (use queue not q, message not msg, channel not ch)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/update.rs` around lines 430 - 434, Several map_err closures use the abbreviated closure param `|e|`; update them to `|error|` for readability and standards compliance—e.g., change the inspect call that creates `container_info` (docker.inspect_container(...).await.map_err(|e| anyhow::anyhow!("failed to inspect container: {}", e))?) to use `map_err(|error| anyhow::anyhow!("failed to inspect container: {}", error))?` and apply the same rename for the other map_err closures in this file (the ones around the futures handling at the locations referenced). Ensure each formatted message refers to `error` (not `e`) and compile to verify no unused variable warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/routes/Settings.tsx`:
- Around line 1701-1703: Replace the literal backticks in the JSX paragraph that
currently reads inside the <p className="text-xs text-ink-faint"> element by
wrapping the command names in <code> elements (e.g., use <code>podman
compose</code> and <code>podman run</code>) so they render as inline code in the
browser; update the JSX in Settings.tsx accordingly.
In `@src/update.rs`:
- Around line 623-642: The function parse_container_id_from_mountinfo uses
hardcoded offsets (19, 20) tied to marker strings which is fragile; change the
logic to compute the slice start using the marker length instead (e.g., use
position + marker.len() or let offset = marker.len()) so the code uses the
actual marker length rather than magic numbers; update the inner loop where
after_marker is computed to derive the offset from marker.len() and keep the
existing id extraction and validation in parse_container_id_from_mountinfo.
---
Outside diff comments:
In `@docs/docker.md`:
- Line 204: Update the wording in the "Settings → Updates" description: replace
the parenthetical "(Docker)" with "(Docker / Podman)" so the line that reads
"one-click controls (Docker)" becomes "one-click controls (Docker / Podman)";
ensure the same change is applied to any identical phrasing in the "Settings →
Updates" section for consistency with the rest of the doc.
---
Nitpick comments:
In `@src/update.rs`:
- Around line 586-596: The call to docker.remove_container(...).await.ok()
silently discards errors; instead capture the Result from remove_container (the
call around container_id in the shutdown/stop path) and log any Err with
contextual information (container_id and that we're shutting down) before
continuing; do not propagate the error, but ensure it is at least recorded via
the project's logger (e.g., warn/error) so failures to remove the container are
visible for debugging.
- Around line 119-123: The message in new_shared_status ("Mount a container
runtime socket to enable one-click updates.") is less informative than
detect_apply_capability which lists specific socket paths; unify them by
extracting a shared constant string (e.g., CONTAINER_SOCKET_HINT) or by calling
the same helper used in detect_apply_capability (or using
resolve_container_socket() output) so both new_shared_status and
detect_apply_capability return the identical detailed message including the
socket paths; update the code in new_shared_status to use that constant/helper
(referencing new_shared_status, detect_apply_capability, and
resolve_container_socket) so the messages remain consistent.
- Around line 430-434: Several map_err closures use the abbreviated closure
param `|e|`; update them to `|error|` for readability and standards
compliance—e.g., change the inspect call that creates `container_info`
(docker.inspect_container(...).await.map_err(|e| anyhow::anyhow!("failed to
inspect container: {}", e))?) to use `map_err(|error| anyhow::anyhow!("failed to
inspect container: {}", error))?` and apply the same rename for the other
map_err closures in this file (the ones around the futures handling at the
locations referenced). Ensure each formatted message refers to `error` (not `e`)
and compile to verify no unused variable warnings.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/content/docs/(getting-started)/docker.mdxdocs/docker.mdinterface/src/components/UpdateBanner.tsxinterface/src/routes/Settings.tsxsrc/update.rs
- Updated the web dashboard documentation to include Podman alongside Docker in the one-click update controls. - Added a new Context Inspect Tool for debugging channel behavior in cortex chat, providing detailed insights into the internal context seen by the LLM. - Improved error messages in the update process for better clarity on container runtime connection issues. - Refactored code to streamline socket hint generation and error handling for container operations.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
docs/design-docs/context-inspect-tool.md (1)
1-3: Cross-link this new design doc from existing docs/README.Given this is a net-new markdown file, please also update existing docs entry points so the feature documentation is discoverable from current docs surfaces.
As per coding guidelines: "
**/*.md: Add new features without forgetting to update existing docs; update README.md or docs/ in the same commit/PR rather than creating new doc files"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-docs/context-inspect-tool.md` around lines 1 - 3, Add a cross-link to the new design doc from the project docs entry point so the feature is discoverable: update the main docs README (e.g., README.md or docs/README.md) or the docs index to include an entry linking to "docs/design-docs/context-inspect-tool.md" with a short title like "Context Inspect Tool" and a one-line description; ensure the link is added in the appropriate section (new features / design docs) and commit the README/docs change in the same PR as the new markdown file.src/update.rs (1)
124-128: Avoid optimisticcan_applyat initialization.At Line 125, availability is inferred from socket file existence only, but later logic requires successful connect + ping. This can briefly expose one-click apply as available when it is not actually usable.
Suggested adjustment
- status.can_apply = resolve_container_socket().is_some(); - if !status.can_apply { - status.cannot_apply_reason = Some(container_runtime_socket_hint()); - } + // Start conservative; `check_for_update()` will populate verified capability. + status.can_apply = false; + status.cannot_apply_reason = Some(container_runtime_socket_hint());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/update.rs` around lines 124 - 128, Currently Deployment::Docker sets status.can_apply based solely on resolve_container_socket().is_some(), which is optimistic; change the initialization so status.can_apply is false (and set status.cannot_apply_reason to container_runtime_socket_hint() if the socket is missing), then only set status.can_apply = true (and clear cannot_apply_reason) after the actual connect+ping succeeds (the code path that uses resolve_container_socket() and performs the runtime ping); update the logic around resolve_container_socket(), status.can_apply, and status.cannot_apply_reason to reflect this two-step check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design-docs/context-inspect-tool.md`:
- Line 590: The note warns that extending the read_channel_context tool to
accept arbitrary channel IDs can expose cross-channel data; update the
design/docs and the read_channel_context implementation to require and document
strict authorization: explicitly state required privileged roles/permissions
(e.g., admin or auditor roles), enforce RBAC checks in read_channel_context,
validate and whitelist channel IDs or require an explicit scope parameter, add
immutable audit logging for every access (who, when, target_channel_id), and
document the authz model and auditing requirements in the design doc so this
capability is only enabled for approved, logged, and reviewed principals.
- Around line 29-42: The markdown fenced code blocks in the design doc are
missing language identifiers which trips the linter; update the two affected
fences (the one containing the Cortex Chat flow starting with "Cortex Chat
(opened on channel page)" and the other block around the
ChannelContextInspector/Branch section) to use a language tag such as ```text or
```markdown so the blocks become ```text (or ```markdown) ... ```; ensure both
occurrences are changed so the markdown linter passes.
- Around line 151-180: Add mandatory redaction and authorization requirements
for the generation and access of the full_context_dump output: define and
document a redaction pipeline that strips secrets/PII from system_prompt,
tool_list, and history_summary (e.g., regex/PII detectors, sensitive-field
allowlist/denylist, and a configurable redaction policy), require that memory
recall flow only through curated branches (enforce via branch-level
API/middleware so raw search results are never included in history_summary), and
introduce explicit permission checks and audit logging for the full_context_dump
endpoint (role-based checks for who can invoke/read full_context_dump and
recording of access events). Update the design doc sections that reference
full_context_dump, status_block, tool_list and history_summary to include these
redaction rules, authorization requirements, and a note that implementations
must fail closed if redaction/authorization cannot be verified.
In `@src/update.rs`:
- Around line 806-871: Tests use hardcoded "/tmp/missing-*.sock" paths which can
exist on CI and cause flakes; update each test that calls
resolve_container_socket_with (e.g.,
test_resolve_container_socket_with_podman_rootful,
test_resolve_container_socket_with_podman_rootless,
test_resolve_container_socket_with_tcp_docker_host_falls_back,
test_resolve_container_socket_with_none, etc.) to create a TempDir and use a
unique non-existent path inside it (e.g.,
temp_dir.path().join("missing-docker.sock").to_string_lossy().to_string())
instead of the literal "/tmp/..." so the path is scoped to the test and
guaranteed absent; ensure you convert the PathBuf to &str/String matching the
existing function signatures when passing to resolve_container_socket_with.
---
Nitpick comments:
In `@docs/design-docs/context-inspect-tool.md`:
- Around line 1-3: Add a cross-link to the new design doc from the project docs
entry point so the feature is discoverable: update the main docs README (e.g.,
README.md or docs/README.md) or the docs index to include an entry linking to
"docs/design-docs/context-inspect-tool.md" with a short title like "Context
Inspect Tool" and a one-line description; ensure the link is added in the
appropriate section (new features / design docs) and commit the README/docs
change in the same PR as the new markdown file.
In `@src/update.rs`:
- Around line 124-128: Currently Deployment::Docker sets status.can_apply based
solely on resolve_container_socket().is_some(), which is optimistic; change the
initialization so status.can_apply is false (and set status.cannot_apply_reason
to container_runtime_socket_hint() if the socket is missing), then only set
status.can_apply = true (and clear cannot_apply_reason) after the actual
connect+ping succeeds (the code path that uses resolve_container_socket() and
performs the runtime ping); update the logic around resolve_container_socket(),
status.can_apply, and status.cannot_apply_reason to reflect this two-step check.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/design-docs/context-inspect-tool.mddocs/docker.mdinterface/src/routes/Settings.tsxsrc/llm/model.rssrc/update.rs
✅ Files skipped from review due to trivial changes (1)
- src/llm/model.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/docker.md
- interface/src/routes/Settings.tsx
Comment on lines
+29
to
+42
| ``` | ||
| Cortex Chat (opened on channel page) | ||
| ↓ calls context_inspect tool | ||
| ↓ spawns Branch with special tool server | ||
| ↓ Branch calls read_channel_context | ||
| ↓ ChannelContextInspector builds full snapshot: | ||
| • System prompt (identity + bulletin + skills + capabilities + status) | ||
| • Tool definitions with complete schemas | ||
| • Conversation history (full Vec<Message>) | ||
| • Status block text | ||
| • Context statistics (token counts, usage %) | ||
| ↓ Branch analyzes and returns conclusion | ||
| ↓ Cortex chat receives analysis | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
Both fenced blocks should specify a language (for example, text at Line 29 and text or markdown at Line 564) to satisfy markdown linting.
Also applies to: 564-585
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design-docs/context-inspect-tool.md` around lines 29 - 42, The markdown
fenced code blocks in the design doc are missing language identifiers which
trips the linter; update the two affected fences (the one containing the Cortex
Chat flow starting with "Cortex Chat (opened on channel page)" and the other
block around the ChannelContextInspector/Branch section) to use a language tag
such as ```text or ```markdown so the blocks become ```text (or ```markdown) ...
```; ensure both occurrences are changed so the markdown linter passes.
Comment on lines
+151
to
+180
| The `full_context_dump` field provides a structured markdown report: | ||
|
|
||
| ```markdown | ||
| # Channel Context: #{channel_name} | ||
|
|
||
| **Channel ID:** {channel_id} | ||
| **Context Usage:** {stats.total_tokens} / {stats.context_window} tokens ({stats.usage_percent}%) | ||
|
|
||
| ## System Prompt ({stats.system_prompt_tokens} tokens) | ||
|
|
||
| {system_prompt} | ||
|
|
||
| ## Available Tools ({tool_count} tools, {stats.tool_defs_tokens} tokens) | ||
|
|
||
| {tool_list} | ||
|
|
||
| ## Conversation History ({message_count} messages, {stats.history_tokens} tokens) | ||
|
|
||
| {history_summary} | ||
|
|
||
| ## Status Block | ||
|
|
||
| {status_block} | ||
|
|
||
| ## Analysis | ||
|
|
||
| - Compaction risk: {if usage_percent > 80% then "HIGH" else "Normal"} | ||
| - Active processes: {branch_count} branches, {worker_count} workers | ||
| - Recent completions: {recent_completion_count} | ||
| ``` |
There was a problem hiding this comment.
Define redaction + access-control guarantees for full context dumps.
This design returns full system prompt, full tool schemas, and full history, but it does not define redaction rules (secrets/PII) or explicit authorization boundaries for who can invoke/read this output. Please add mandatory redaction and permission requirements before implementation.
Based on learnings: "Don't dump raw search results into channel context. Memory recall must go through a branch, which curates. The channel gets clean conclusions, not raw database rows"
Also applies to: 504-514
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design-docs/context-inspect-tool.md` around lines 151 - 180, Add
mandatory redaction and authorization requirements for the generation and access
of the full_context_dump output: define and document a redaction pipeline that
strips secrets/PII from system_prompt, tool_list, and history_summary (e.g.,
regex/PII detectors, sensitive-field allowlist/denylist, and a configurable
redaction policy), require that memory recall flow only through curated branches
(enforce via branch-level API/middleware so raw search results are never
included in history_summary), and introduce explicit permission checks and audit
logging for the full_context_dump endpoint (role-based checks for who can
invoke/read full_context_dump and recording of access events). Update the design
doc sections that reference full_context_dump, status_block, tool_list and
history_summary to include these redaction rules, authorization requirements,
and a note that implementations must fail closed if redaction/authorization
cannot be verified.
| ## Notes | ||
|
|
||
| - The inspection branch gets an **empty history** — it doesn't need channel context inheritance since it reads context via the tool | ||
| - The `read_channel_context` tool could be extended to support arbitrary channel IDs (not just associated channel) for cross-channel debugging |
There was a problem hiding this comment.
Avoid “arbitrary channel ID” inspection without strict authz model.
The proposed extension to inspect arbitrary channel IDs can become a cross-channel data exposure path unless explicitly scoped to privileged roles and audited. Please document hard authorization requirements if this ever lands.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design-docs/context-inspect-tool.md` at line 590, The note warns that
extending the read_channel_context tool to accept arbitrary channel IDs can
expose cross-channel data; update the design/docs and the read_channel_context
implementation to require and document strict authorization: explicitly state
required privileged roles/permissions (e.g., admin or auditor roles), enforce
RBAC checks in read_channel_context, validate and whitelist channel IDs or
require an explicit scope parameter, add immutable audit logging for every
access (who, when, target_channel_id), and document the authz model and auditing
requirements in the design doc so this capability is only enabled for approved,
logged, and reviewed principals.
Comment on lines
+806
to
+871
| resolve_container_socket_with(None, &docker_socket, "/tmp/missing-podman.sock", None); | ||
| assert_eq!(resolved.as_deref(), Some(docker_socket.as_str())); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_resolve_container_socket_with_podman_rootful() { | ||
| let temp_dir = tempfile::TempDir::new().expect("temp dir"); | ||
| let podman_socket = create_fake_socket(&temp_dir, "podman.sock"); | ||
|
|
||
| let resolved = | ||
| resolve_container_socket_with(None, "/tmp/missing-docker.sock", &podman_socket, None); | ||
| assert_eq!(resolved.as_deref(), Some(podman_socket.as_str())); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_resolve_container_socket_with_podman_rootless() { | ||
| let temp_dir = tempfile::TempDir::new().expect("temp dir"); | ||
| let runtime_dir = temp_dir.path().to_string_lossy().to_string(); | ||
| let podman_socket = create_fake_socket(&temp_dir, "podman/podman.sock"); | ||
|
|
||
| let resolved = resolve_container_socket_with( | ||
| None, | ||
| "/tmp/missing-docker.sock", | ||
| "/tmp/missing-podman.sock", | ||
| Some(&runtime_dir), | ||
| ); | ||
| assert_eq!(resolved, Some(podman_socket)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_resolve_container_socket_with_docker_host_unix() { | ||
| let temp_dir = tempfile::TempDir::new().expect("temp dir"); | ||
| let custom_socket = create_fake_socket(&temp_dir, "custom.sock"); | ||
| let docker_host = format!("unix://{custom_socket}"); | ||
|
|
||
| let resolved = resolve_container_socket_with( | ||
| Some(&docker_host), | ||
| "/tmp/missing-docker.sock", | ||
| "/tmp/missing-podman.sock", | ||
| None, | ||
| ); | ||
| assert_eq!(resolved.as_deref(), Some(custom_socket.as_str())); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_resolve_container_socket_with_tcp_docker_host_falls_back() { | ||
| let temp_dir = tempfile::TempDir::new().expect("temp dir"); | ||
| let docker_socket = create_fake_socket(&temp_dir, "docker.sock"); | ||
|
|
||
| let resolved = resolve_container_socket_with( | ||
| Some("tcp://127.0.0.1:2376"), | ||
| &docker_socket, | ||
| "/tmp/missing-podman.sock", | ||
| None, | ||
| ); | ||
| assert_eq!(resolved.as_deref(), Some(docker_socket.as_str())); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_resolve_container_socket_with_none() { | ||
| let resolved = resolve_container_socket_with( | ||
| None, | ||
| "/tmp/missing-docker.sock", | ||
| "/tmp/missing-podman.sock", | ||
| None, | ||
| ); |
There was a problem hiding this comment.
Tests may be flaky due to hardcoded /tmp/missing-*.sock paths.
Several tests (e.g., Line 806, Line 816, Line 828, Line 844, Line 868) assume those paths do not exist. On some hosts/CI runners they might, causing nondeterministic failures.
Use tempdir-scoped non-existent paths
- let resolved =
- resolve_container_socket_with(None, &docker_socket, "/tmp/missing-podman.sock", None);
+ let missing_podman = temp_dir.path().join("missing-podman.sock");
+ let resolved = resolve_container_socket_with(
+ None,
+ &docker_socket,
+ missing_podman.to_str().expect("utf8 path"),
+ None,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/update.rs` around lines 806 - 871, Tests use hardcoded
"/tmp/missing-*.sock" paths which can exist on CI and cause flakes; update each
test that calls resolve_container_socket_with (e.g.,
test_resolve_container_socket_with_podman_rootful,
test_resolve_container_socket_with_podman_rootless,
test_resolve_container_socket_with_tcp_docker_host_falls_back,
test_resolve_container_socket_with_none, etc.) to create a TempDir and use a
unique non-existent path inside it (e.g.,
temp_dir.path().join("missing-docker.sock").to_string_lossy().to_string())
instead of the literal "/tmp/..." so the path is scoped to the test and
guaranteed absent; ensure you convert the PathBuf to &str/String matching the
existing function signatures when passing to resolve_container_socket_with.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reimplementation of #120 based on new updater logic.
Summary
DOCKER_HOSTunix socket, Docker rootful socket, Podman rootful socket, then Podman rootless socketCode Example
Testing
cargo test update::testsbun run --cwd interface buildNote
Highlights: This PR extends one-click update support from Docker-only to Docker and Podman (both rootful and rootless). Socket detection now checks multiple paths in order of preference (DOCKER_HOST env var, Docker rootful at /var/run/docker.sock, Podman rootful at /run/podman/podman.sock, then Podman rootless via $XDG_RUNTIME_DIR). Container ID parsing now handles both Docker and Podman mountinfo formats, with fallback to hostname. Updated documentation provides clear guidance for both deployment types, including SELinux considerations for RHEL/Fedora systems. Added 6 comprehensive tests covering socket resolution edge cases and container ID parsing. UI messaging updated throughout to use generic "container runtime" terminology instead of Docker-specific language.
Written by Tembo for commit c15472c.