fix(install): preserve comma in daemon.json on Jetson JP6 setup - #1895
fix(install): preserve comma in daemon.json on Jetson JP6 setup#1895swqa-saiy wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/setup-jetson.sh`:
- Line 53: Replace the brittle line-based deletion of keys (the "${SUDO[@]}" sed
-i '/"iptables": false,/d; /"bridge": "none"/d' invocation) with a JSON-safe,
parser-based edit: use jq to read the daemon.json, delete the "iptables" and
"bridge" keys (e.g. jq 'del(.iptables, .bridge)'), write atomically to a temp
file and move it into place under sudo (preserving permissions), and then
restart Docker; this guarantees valid JSON regardless of key order or trailing
commas.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e63efc3-0cec-48e3-946e-13f38a1f4d95
📒 Files selected for processing (1)
scripts/setup-jetson.sh
|
✨ Thanks for submitting this PR, which proposes a way to fix an issue with the daemon.json file on Jetson JP6 with Docker. Possibly related open issues: |
|
@swqa-saiy Thanks for catching this — the comma-stripping sed is definitely the bug. However #1913 replaces the entire sed approach with a Python JSON parser, which is more robust and fixes the same issue (#1875). Recommending we merge #1913 instead since it eliminates the class of bug entirely rather than patching one instance. Your diagnosis in the issue was spot on though — it helped both PRs land. |
9127ef8 to
3e66b39
Compare
The sed command in setup-jetson.sh stripped the trailing comma from "default-runtime": "nvidia," after removing the iptables and bridge lines, producing malformed JSON that prevented Docker from starting. Remove the comma-stripping substitution and keep only the line deletions so the resulting daemon.json remains valid. Fixes NVIDIA#1875 Signed-off-by: swqa-saiy <swqa-saiy@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/setup-jetson.sh`:
- Around line 89-91: The current cfg.pop("iptables", None) and cfg.pop("bridge",
None) unconditionally remove user values; change this to preserve user-specified
settings by only deleting those keys when they match the old semantics — i.e.,
remove "bridge" only if cfg.get("bridge") == "none" and remove "iptables" only
if cfg.get("iptables") is exactly False; update the logic around the cfg
variable (the two pop calls) to perform these conditional checks before popping
so custom bridge names or true iptables settings are retained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 12f2ff7c-32ac-4ed1-9437-fc3f16eb5b58
📒 Files selected for processing (1)
scripts/setup-jetson.sh
| # --- Remove unwanted keys --- | ||
| cfg.pop("iptables", None) | ||
| cfg.pop("bridge", None) |
There was a problem hiding this comment.
Preserve the old match semantics when deleting Docker keys.
These pop() calls now remove bridge and iptables regardless of value. The previous logic only stripped "bridge": "none" and "iptables": false, so this will also wipe user-defined values like a custom bridge name during JP6 setup.
Suggested fix
-# --- Remove unwanted keys ---
-cfg.pop("iptables", None)
-cfg.pop("bridge", None)
+# --- Remove only the JP6 defaults we intentionally override ---
+if cfg.get("iptables") is False:
+ cfg.pop("iptables", None)
+if cfg.get("bridge") == "none":
+ cfg.pop("bridge", None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-jetson.sh` around lines 89 - 91, The current
cfg.pop("iptables", None) and cfg.pop("bridge", None) unconditionally remove
user values; change this to preserve user-specified settings by only deleting
those keys when they match the old semantics — i.e., remove "bridge" only if
cfg.get("bridge") == "none" and remove "iptables" only if cfg.get("iptables") is
exactly False; update the logic around the cfg variable (the two pop calls) to
perform these conditional checks before popping so custom bridge names or true
iptables settings are retained.
|
Closing in favor of #1913 which covers the same fix with a more thoroughly reviewed implementation. Thanks @BenediktSchackenberg |
wscurran
added
area: packaging
3 participants
The sed command in setup-jetson.sh stripped the trailing comma from "default-runtime": "nvidia," after removing the iptables and bridge lines, producing malformed JSON that prevented Docker from starting.
Remove the comma-stripping substitution and keep only the line deletions so the resulting daemon.json remains valid.
Fixes #1875
Summary
The
sedcommand insetup-jetson.shfor JP6 had three operations: delete theiptablesline, delete thebridgeline, and strip the trailing comma from"default-runtime": "nvidia,". The third substitution is fragile — depending on key ordering indaemon.json, it can produce invalid JSON or silently no-op. Since deleting the two lines is sufficient, the substitution is removed.Related Issue
Fixes #1875
Changes
s/"default-runtime": "nvidia",/"default-runtime": "nvidia"/substitution from the JP6sedcommandd(delete line) operations which are sufficientType of Change
Testing
Verified against sample
daemon.jsonfiles with various key orderings on a JP6 device — JSON remains valid after the fix in all cases.Summary by CodeRabbit