Use Docker BuildKit to build addons #5974
Conversation
felipecrs
force-pushed
the
use-buildkit
branch
2 times, most recently
from
a34507e to
f380e1c
Compare
|
Warning Rate limit exceeded@felipecrs has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes refactor the add-on build process to construct and execute Docker CLI commands directly, replacing the previous use of the Docker Python SDK for building images. The new approach assembles full Docker build commands with environment variables, volume bindings, and working directory. Related tests are updated to verify the new command-based build logic, including squash build options. Minor adjustments are made to Docker command execution and mocking in tests. Changes
Sequence Diagram(s)sequenceDiagram
participant AddonBuild
participant DockerAddon
participant DockerAPI
Note over AddonBuild: Build process starts
AddonBuild->>AddonBuild: get_docker_args(version, image_tag)
AddonBuild->>DockerAddon: Returns Docker CLI command, volumes, working_dir
DockerAddon->>DockerAPI: run_command("docker", command, volumes, working_dir, env)
DockerAPI->>Docker: Executes Docker CLI build command
Docker->>DockerAPI: Returns build output and exit code
alt Build successful
DockerAPI->>DockerAddon: Return image and build log
else Build failed
DockerAPI->>DockerAddon: Raise DockerException with log
end
sequenceDiagram
participant Test
participant AddonBuild
Test->>AddonBuild: get_docker_args(version, image_tag)
AddonBuild->>Test: Returns command list, volumes, working_dir
Note over Test: Test asserts presence of flags (e.g., --platform, --file) and env (DOCKER_BUILDKIT)
✨ 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 (
|
![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)
tests/addons/test_addon.py (1)
970-981: Useful helper function with minor formatting issue.The
is_in_listfunction is a good addition for verifying command argument order in lists. However, there's a formatting issue that should be addressed.Apply this diff to fix the formatting issue flagged by static analysis:
- if c in b: - b = b[b.index(c) :] + if c in b: + b = b[b.index(c):]supervisor/docker/addon.py (1)
677-703: Consider enhancing error handling and logging.The refactor to use Docker CLI is well-implemented. A few suggestions:
- The error message on line 697 could include more context about what might have gone wrong
- Consider adding progress logging for long builds since you have access to the build output
For better error diagnostics, consider parsing the build output for specific error patterns:
if result.exit_code != 0: - error_message = f"The docker build command for {addon_image_tag} failed with exit code {result.exit_code}. Output:\n{log}" + error_message = f"Docker build failed for {addon_image_tag} (exit code: {result.exit_code})" + # Extract the last few meaningful lines for the error message + log_lines = log.strip().split('\n') + if log_lines: + error_details = '\n'.join(log_lines[-10:]) # Last 10 lines + error_message += f"\nBuild output (last 10 lines):\n{error_details}" raise docker.errors.DockerException(error_message)
📜 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 (8)
supervisor/addons/build.py(1 hunks)supervisor/docker/addon.py(2 hunks)supervisor/docker/homeassistant.py(1 hunks)supervisor/docker/manager.py(3 hunks)tests/addons/test_addon.py(3 hunks)tests/addons/test_build.py(5 hunks)tests/api/test_addons.py(1 hunks)tests/conftest.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure t...
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
tests/conftest.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - If "" are used to mark UI strings, replace them by bold.
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
tests/conftest.py
`*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
tests/conftest.py
`*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
tests/conftest.py
`*/**(html|markdown|md)`: do not comment on HTML used for icons
*/**(html|markdown|md): do not comment on HTML used for icons
tests/conftest.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
tests/conftest.py
🪛 Flake8 (7.2.0)
tests/addons/test_addon.py
[error] 978-978: whitespace before ':'
(E203)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Run tests Python 3.13.3
🔇 Additional comments (18)
supervisor/docker/homeassistant.py (1)
211-211: LGTM! Parameter rename aligns with updated method signature.The change from
versiontotagis consistent with the updatedrun_commandmethod signature insupervisor/docker/manager.py. This maintains consistency across the Docker command interface.tests/api/test_addons.py (1)
242-244: Enhanced mocking supports the new Docker addon behavior.The additional patch on
DockerAddon.healthcheckproperly mocks the healthcheck presence during addon rebuilds, supporting the updated Docker CLI command flow introduced in the broader refactor.tests/conftest.py (2)
47-47: Enhanced imports support the new command execution interface.Adding
CommandReturnimport enables proper mocking of the Docker CLI command execution flow.
134-138: Improved Docker fixture mocking aligns with refactored command execution.The changes provide more accurate version representation using
AwesomeVersionand mock the newrun_commandmethod with a successfulCommandReturnobject, supporting the transition from Docker SDK to CLI commands.tests/addons/test_addon.py (2)
855-867: Test correctly updated to verify Docker CLI command execution.The test properly checks
run_commandcalls instead of Docker SDKbuildcalls, verifying the correct Docker CLI command structure including platform and tag arguments. This aligns with the new build process using Docker CLI commands.
884-896: Consistent test update maintains coverage for the new build flow.Similar to the previous test, this correctly verifies the Docker CLI command invocation with proper platform and tag arguments, ensuring test coverage for the refactored build process.
supervisor/docker/manager.py (5)
298-298: Enhanced parameter flexibility supports diverse command inputs.Expanding the
commandparameter to accept strings, lists, or None provides better flexibility for different types of Docker CLI commands and aligns with Docker's command execution patterns.
308-310: Improved image handling and logging consistency.Constructing the full
image_tagupfront and using it consistently in logging and container execution provides better clarity and consistency throughout the method.
317-317: Detached mode default improves command execution reliability.Setting
detach=Trueby default ensures consistent container execution behavior and allows for proper wait/cleanup handling in the command execution flow.
333-333: Volume cleanup prevents resource leaks.Adding
v=Trueto container removal ensures associated volumes are cleaned up, preventing potential resource leaks during temporary container operations.
335-335: Robust error handling with sensible default.Defaulting the exit code to 1 when
StatusCodeis missing provides a sensible fallback that indicates failure, improving the reliability of command execution result handling.tests/addons/test_build.py (5)
12-31: LGTM! Test correctly adapted to new CLI interface.The test properly verifies that the platform argument is included in the Docker build command with the correct value.
33-57: LGTM! Dockerfile path verification updated correctly.The test properly checks that the
--fileargument is included in the command with the expected Dockerfile path.
59-83: LGTM! Architecture-specific Dockerfile test updated appropriately.The test correctly verifies that the architecture-specific Dockerfile path is used when available.
113-140: Excellent test coverage for squash/BuildKit interaction!This test thoroughly verifies the key behavior that squash option disables BuildKit and adds the necessary flags for legacy builder compatibility.
142-169: LGTM! Completes the test coverage for BuildKit behavior.This test ensures that BuildKit remains enabled by default when squash is not used, which is the expected behavior for the new feature.
supervisor/addons/build.py (2)
129-200: Well-structured refactor to CLI-based build approach!The conversion from Docker SDK parameters to CLI command construction is comprehensive and handles all the necessary build arguments, labels, and environment variables correctly. The BuildKit/squash handling aligns perfectly with the PR objectives.
186-189: ```shell
#!/bin/bashDisplay lines 160-200 around the build_path manipulation in build.py
sed -n '160,200p' supervisor/addons/build.py
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/addons/test_addon.py (1)
970-981: Fix formatting issue in the helper function.The helper function logic is correct and useful for verifying command argument order. However, there's a whitespace formatting issue that should be addressed.
Apply this diff to fix the formatting:
- if c in b: - b = b[b.index(c) :] + if c in b: + b = b[b.index(c):]
📜 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 (8)
supervisor/addons/build.py(1 hunks)supervisor/docker/addon.py(2 hunks)supervisor/docker/homeassistant.py(1 hunks)supervisor/docker/manager.py(4 hunks)tests/addons/test_addon.py(3 hunks)tests/addons/test_build.py(5 hunks)tests/api/test_addons.py(1 hunks)tests/conftest.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- supervisor/docker/homeassistant.py
- tests/api/test_addons.py
- tests/addons/test_build.py
- tests/conftest.py
- supervisor/docker/addon.py
- supervisor/docker/manager.py
🧰 Additional context used
🪛 Flake8 (7.2.0)
tests/addons/test_addon.py
[error] 978-978: whitespace before ':'
(E203)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armhf supervisor
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.3
🔇 Additional comments (4)
tests/addons/test_addon.py (2)
855-867: LGTM! Test correctly updated for CLI-based builds.The test changes properly reflect the transition from Docker SDK
images.build()calls torun_command()calls for the new CLI-based build process. The assertions correctly verify that the Docker CLI command includes the required platform specification and image tag.
884-896: LGTM! Consistent test update for missing image scenario.The test correctly mirrors the changes in the previous test, properly verifying the CLI-based build approach for the missing image case.
supervisor/addons/build.py (2)
124-128: LGTM! Good code organization.Moving the
_fix_labelmethod aboveget_docker_argsimproves readability since it's used within that method.
129-198: Excellent refactoring to support CLI-based builds!This is a well-implemented transformation from Docker SDK parameters to a complete Docker CLI command structure. The implementation correctly:
- Constructs the full
docker buildcommand with all necessary flags- Handles the squash option by disabling BuildKit and adding legacy builder flags
- Sets up proper volume bindings for Docker socket access and addon files
- Converts labels and build arguments to CLI format
The path mapping logic (removing
/data/prefix and prepending/mnt/supervisor/) aligns with the container-based build approach.Please verify that the path mapping transformation is correct for all deployment scenarios:
#!/bin/bash # Description: Verify addon path locations and the /data/ to /mnt/supervisor/ mapping # Find addon path usage patterns echo "=== Searching for addon path usage patterns ===" rg -A 3 "path_location" --type py echo -e "\n=== Searching for /data/ path references ===" rg "/data/" --type py echo -e "\n=== Searching for /mnt/supervisor/ references ===" rg "/mnt/supervisor" --type py
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/addons/test_addon.py (1)
970-981: Fix whitespace formatting issueThe helper function logic is correct for checking ordered list elements, but there's a formatting issue flagged by static analysis.
Apply this diff to fix the whitespace issue:
- if c in b: - b = b[b.index(c) :] + if c in b: + b = b[b.index(c):]
📜 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 (5)
tests/addons/test_addon.py(4 hunks)tests/homeassistant/test_core.py(3 hunks)tests/plugins/test_plugin_base.py(1 hunks)tests/resolution/fixup/test_addon_execute_rebuild.py(1 hunks)tests/resolution/fixup/test_core_execute_rebuild.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/homeassistant/test_core.py
🧰 Additional context used
🪛 Flake8 (7.2.0)
tests/addons/test_addon.py
[error] 978-978: whitespace before ':'
(E203)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build armv7 supervisor
- GitHub Check: Run tests Python 3.13.3
🔇 Additional comments (6)
tests/plugins/test_plugin_base.py (1)
366-366: LGTM: Enhanced container cleanup with volume removalThe test correctly verifies that container removal now includes volume cleanup (
v=True), which prevents volume accumulation and follows Docker best practices.tests/resolution/fixup/test_addon_execute_rebuild.py (1)
79-81: LGTM: Consistent volume cleanup in addon rebuildThe assertion correctly verifies that addon container removal during rebuild includes volume cleanup (
v=True), maintaining consistency with the enhanced cleanup behavior.tests/resolution/fixup/test_core_execute_rebuild.py (1)
68-70: LGTM: Volume cleanup for core container removalThe test properly verifies that Home Assistant container removal includes volume cleanup (
v=True), ensuring consistent cleanup behavior across all container types.tests/addons/test_addon.py (3)
846-846: LGTM: Container cleanup includes volume removalThe test correctly verifies that container removal during addon loading includes volume cleanup (
v=True), consistent with the enhanced cleanup behavior.
855-867: LGTM: Updated tests for Docker CLI command executionThe test assertions have been correctly updated to verify Docker CLI command execution instead of Docker SDK calls. The use of
is_in_listhelper ensures that the required arguments (--platform,--tag) are present in the correct order within the command list.
884-896: LGTM: Consistent CLI command testing for missing image caseThe test properly verifies Docker CLI command execution for the missing image scenario, maintaining consistency with the refactored build process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/addons/test_addon.py (1)
970-981: Fix whitespace formatting issue in list slicing.The helper function logic is correct, but there's a formatting issue that needs to be addressed.
Apply this diff to fix the whitespace issue:
- b = b[b.index(c) :] + b = b[b.index(c):]The function correctly implements the logic to check if all elements of one list appear in another list in order, which is useful for verifying command argument order in the tests.
📜 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 (4)
supervisor/docker/addon.py(2 hunks)supervisor/docker/homeassistant.py(0 hunks)supervisor/docker/manager.py(4 hunks)tests/addons/test_addon.py(4 hunks)
💤 Files with no reviewable changes (1)
- supervisor/docker/homeassistant.py
🚧 Files skipped from review as they are similar to previous changes (2)
- supervisor/docker/manager.py
- supervisor/docker/addon.py
🧰 Additional context used
🪛 Flake8 (7.2.0)
tests/addons/test_addon.py
[error] 978-978: whitespace before ':'
(E203)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run tests Python 3.13.3
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build i386 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (3)
tests/addons/test_addon.py (3)
846-846: LGTM! Container removal now includes volume cleanup.The addition of
v=Truecorrectly reflects the new behavior where volumes are also removed when containers are removed, aligning with the Docker CLI approach.
855-867: LGTM! Test correctly verifies the new Docker CLI command approach.The updated assertions properly verify that the build process now uses
run_commandwith the correct Docker CLI arguments, including platform specification and image tag.
884-896: LGTM! Consistent test updates for missing image scenario.The test updates follow the same pattern as the previous test, correctly verifying the Docker CLI command construction for the missing image case.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/addons/test_addon.py (1)
988-999: Fix whitespace formatting issue.The helper function logic is correct, but there's a formatting issue flagged by static analysis.
Apply this diff to fix the whitespace issue:
- b = b[b.index(c) :] + b = b[b.index(c):]
📜 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 (4)
supervisor/addons/build.py(1 hunks)tests/addons/test_addon.py(5 hunks)tests/api/test_addons.py(2 hunks)tests/conftest.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/api/test_addons.py
- tests/conftest.py
- supervisor/addons/build.py
🧰 Additional context used
🪛 Flake8 (7.2.0)
tests/addons/test_addon.py
[error] 996-996: whitespace before ':'
(E203)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Build i386 supervisor
- GitHub Check: Build amd64 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build armv7 supervisor
- GitHub Check: Run tests Python 3.13.3
🔇 Additional comments (5)
tests/addons/test_addon.py (5)
21-21: LGTM: Import aligns with the architectural change.The import of
CommandReturnis consistent with the refactoring to userun_commandwhich returnsCommandReturnobjects instead of directly using Docker SDK methods.
844-851: LGTM: Test mocking updated correctly for CLI approach.The patching has been appropriately updated to mock
coresys.docker.run_commandinstead of Docker SDK methods, consistent with the architectural change to use Docker CLI commands for building addons.
854-855: LGTM: Volume cleanup enhancement implemented.The addition of
v=Truetocontainer.remove()calls aligns with the PR objective to ensure anonymous and ephemeral volumes are properly cleaned up, preventing leftover volumes on the system.
866-878: LGTM: CLI command verification is thorough.The assertions properly verify that the Docker CLI is invoked with the correct arguments including platform, tag, and version parameters. The use of
is_in_listhelper function ensures the command arguments maintain proper ordering.
892-914: LGTM: Consistent test pattern applied.The same testing pattern for CLI commands is appropriately applied to the second test function, maintaining consistency across the test suite.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/addons/test_addon.py (1)
988-999: Fix formatting issue in helper function.The helper function logic is correct for checking ordered presence of elements, but there's a whitespace formatting issue flagged by static analysis.
Apply this diff to fix the formatting:
- b = b[b.index(c) :] + b = b[b.index(c):]
📜 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)
tests/addons/test_addon.py(5 hunks)tests/api/test_addons.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/api/test_addons.py
🧰 Additional context used
🪛 Flake8 (7.2.0)
tests/addons/test_addon.py
[error] 996-996: whitespace before ':'
(E203)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build amd64 supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Run tests Python 3.13.3
🔇 Additional comments (5)
tests/addons/test_addon.py (5)
21-21: Import addition looks good.The new import for
CommandReturnis correctly added to support the updated test mocking approach.
844-851: Test mocking updated appropriately for CLI command approach.The test correctly switches from mocking
images.buildtorun_command, returning aCommandReturnobject that simulates successful Docker CLI execution. This aligns with the architectural change to use Docker BuildKit via CLI commands.
854-857: Container removal updated to include volume cleanup.The addition of
v=Trueto thecontainer.removecall is consistent with the PR objective to ensure anonymous and ephemeral volumes are also removed, preventing leftover volumes on the system.
866-878: Docker CLI command verification is well-structured.The assertions properly verify that the
run_commandmethod is called with correct Docker CLI arguments including platform specification and image tagging. The use of theis_in_listhelper function ensures the command arguments are present in the expected order.
892-914: Consistent test pattern applied to second test function.The
test_addon_loads_missing_imagefunction follows the same updated pattern as the first test, ensuring consistent verification of the Docker CLI command approach across both test scenarios.
|
@coderabbitai this nitpick you mentioned is not correct. Ruff formatter actually prefers the other way. |
|
@agners I'm done with the changes. This PR is ready for review if you are ok with the approach. I can also write a Home Assistant developer blog post for announcing this as well. |
|
I also attached a testing demonstration to the PR description. |
|
Can we add the image here: supervisor/supervisor/resolution/evaluations/container.py Lines 59 to 66 in 8378d43 That way if the check happens to run while a build is in progress and sees the container it won't mark the system as unsupported. Looks good though, really like the idea. |
Nice finds 🤩 ! Can you make this drive-by changes separate PR(s)? We'll likely have to iterate and test this PR a bit before it can go in. But these changes are rather small and can go in quickly. Also, it is better if these fixes are separate commits just in case we run into problems (bisect)/or have to revert. |
|
Absolutely, I'll work on all the requests. Thank you both. |
…nto use-buildkit
…nto use-buildkit
Proposed change
Use Docker BuildKit to build addons by delegating the build to a temporary container.
Type of change
Additional information
Tests made manually:
chrome_R7mgN0sYbp.mp4
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
Bug Fixes
Refactor
Tests