Clean up hassio build scripts #4394
Conversation
Extract some of the parts of the "image import" to the script creating the data partition to separate concerns. The Docker data directory is now passed as a daemon option, instead of only mounting the data partition's folder to the default directory, to be closer to the deployment setup. Also trap the exit and error signals to remove the build container and unmount the data partition, as failed or cancelled build have been leaking the containers/mounts when building interactively (attached to the build container shell).
📝 WalkthroughWalkthroughcreate-data-partition.sh: switches setup to a persistent Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host script
participant DinD as Docker-in-Docker container
participant Filesystem as data_dir (host)
rect rgb(240,248,255)
Host->>Filesystem: create data_dir, supervisordirs, apparmor dir
Host->>DinD: run DinD container mounting Filesystem -> /mnt/data\n--data-root /mnt/data/docker
note right of DinD `#D0F0C0`: Docker daemon uses /mnt/data/docker
end
rect rgb(255,250,240)
Host->>DinD: exec initialisation commands (heredoc)
DinD->>Filesystem: persist supervisor/apparmor files and updater.json
end
rect rgb(255,240,245)
Host->>DinD: cleanup trap on EXIT/ERR -> remove container, unmount Filesystem
end
sequenceDiagram
participant ImportScript as dind-import-containers.sh
participant Docker as Docker daemon
participant Images as Local image tarballs
ImportScript->>Docker: wait for daemon ready
Docker-->>ImportScript: ready
ImportScript->>Images: load images into Docker
ImportScript->>Docker: tag supervisor image
note right of ImportScript: AppArmor steps and channel arg removed
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Potential focus areas:
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
buildroot-external/package/hassio/create-data-partition.sh (1)
35-45: Consider adding error context for network-dependent operations.Line 41 uses
wgetto download AppArmor configuration. While the-eflag in the bash invocation will catch failures, consider adding explicit error handling or retry logic to provide better diagnostics if the AppArmor URL is unreachable.For example, you could add error checking:
sudo bash -ex <<EOF # Indicator for docker-prepare.service to use the containerd snapshotter touch "${data_dir}/.docker-use-containerd-snapshotter" # Setup AppArmor mkdir -p "${data_dir}/supervisor/apparmor" -wget -O "${data_dir}/supervisor/apparmor/hassio-supervisor" "${APPARMOR_URL}" +if ! wget -O "${data_dir}/supervisor/apparmor/hassio-supervisor" "${APPARMOR_URL}"; then + echo "Failed to download AppArmor configuration from ${APPARMOR_URL}" >&2 + exit 1 +fi # Persist build-time updater channel jq -n --arg channel "${channel}" '{"channel": \$channel}' > "${data_dir}/supervisor/updater.json" EOF
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
buildroot-external/package/hassio/create-data-partition.sh(1 hunks)buildroot-external/package/hassio/dind-import-containers.sh(0 hunks)
💤 Files with no reviewable changes (1)
- buildroot-external/package/hassio/dind-import-containers.sh
sairon
force-pushed
the
hassio-clean-up
branch
from
ed4dcb8 to
b4572d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
buildroot-external/package/hassio/create-data-partition.sh(1 hunks)
🔇 Additional comments (4)
buildroot-external/package/hassio/create-data-partition.sh (4)
10-12: Good consolidation of constants for maintainability.Centralizing
data_dirandAPPARMOR_URLimproves code organization and makes future changes easier.
20-21: Correct mount sequence.Creating the directory before mounting is the right order, and the
-o discardflag enables TRIM support for the loop device.
32-32: Verifydind-import-containers.shwas updated per PR scope.Per the PR summary, AppArmor setup and channel handling were moved to this script, so
dind-import-containers.shshould be updated to remove those responsibilities. Please confirm that file has been modified accordingly.
34-43: Good consolidation of initialization tasks.Moving AppArmor profile download, containerd snapshotter flag creation, and updater channel persistence into a single heredoc improves clarity and centralizes setup logic. The use of
bash -exensures errors are caught and commands are echoed for debugging.
| mkdir -p "${data_dir}" | ||
| sudo mount -o loop,discard "${data_img}" "${data_dir}" | ||
|
|
||
| trap 'docker rm -f ${container} > /dev/null; sudo umount ${data_dir} || true' ERR EXIT |
There was a problem hiding this comment.
Critical: Trap uses single quotes, preventing variable expansion—variables won't be cleaned up.
The trap on line 23 uses single quotes, which prevent bash variable expansion. When the trap executes, it will try to run literal commands like docker rm -f ${container} (not expanded), which fails silently. This defeats the PR's goal of ensuring cleanup on failure.
The earlier review discussion already identified this issue and suggested using a cleanup function where variables are evaluated at execution time.
Apply this fix:
# Cleanup function to ensure resources are freed
cleanup() {
if [[ -n "${container:-}" ]]; then
docker rm -f "$container"
fi
sudo umount "${data_dir}"
}
# Set trap early to ensure cleanup on any failure
trap cleanup ERR EXIT
# Use official Docker in Docker images
# We use the same version as Buildroot is using to ensure best compatibility
container=$(docker run --privileged -e DOCKER_TLS_CERTDIR="" \
-v "${data_dir}":/mnt/data \
-v "${build_dir}":/build \
-d "docker:${docker_version}-dind" --feature containerd-snapshotter --data-root /mnt/data/docker)This ensures:
- The trap is set before
docker run, so cleanup runs even if the command fails - Variables are evaluated when
cleanup()executes, not when the trap is set - If
docker runfails,$containerwill be empty and only umount runs - If it succeeds, both container removal and umount run
Also applies to: 27-30
🤖 Prompt for AI Agents
In buildroot-external/package/hassio/create-data-partition.sh around lines 23
and 27-30, the trap currently uses single quotes so variables are not expanded
at execution time causing cleanup to fail; replace the inline single-quoted trap
with a cleanup() function defined before the docker run that checks if container
is non-empty and removes it (docker rm -f "$container") and always attempts sudo
umount "$data_dir", then set trap to call cleanup on ERR and EXIT (trap cleanup
ERR EXIT) before running docker so variables are evaluated when cleanup runs;
ensure variable expansions are quoted and the trap is set early enough to catch
docker run failures.
Extract some of the parts of the "image import" to the script creating the data partition to separate concerns. The Docker data directory is now passed as a daemon option, instead of only mounting the data partition's folder to the default directory, to be closer to the deployment setup. Also trap the exit and error signals to remove the build container and unmount the data partition, as failed or cancelled build have been leaking the containers/mounts when building interactively (attached to the build container shell).
Summary by CodeRabbit