Use Docker BuildKit to build addons #5974
Conversation
felipecrs
force-pushed
the
use-buildkit
branch
2 times, most recently
from
a34507e to
f380e1c
Compare
📝 WalkthroughWalkthroughThe changes transition the add-on build process from using the Docker SDK to invoking the Docker CLI via a container, introduce explicit Docker command argument construction, and add new helper methods and constants. Test suites are updated to reflect the new build invocation and argument handling. A utility function for list order checking is added for test assertions. Changes
Sequence Diagram(s)sequenceDiagram
participant AddonBuild
participant DockerAddon
participant DockerAPI
participant HostDocker
Note over AddonBuild: Build process starts
AddonBuild->>AddonBuild: get_addon_host_path()
AddonBuild->>AddonBuild: get_docker_args(version, image_tag)
DockerAddon->>DockerAPI: run_command(ADDON_BUILDER_IMAGE, version, command, ...)
DockerAPI->>HostDocker: Start container with Docker CLI
HostDocker-->>DockerAPI: Build output and exit code
DockerAPI-->>DockerAddon: CommandReturn (output, exit code)
DockerAddon->>HostDocker: Retrieve built image by tag
HostDocker-->>DockerAddon: Built image object
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)supervisor/addons/build.py📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ 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. |
|
@agners this PR is ready to be reviewed. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (10)
supervisor/addons/build.py(2 hunks)supervisor/docker/addon.py(3 hunks)supervisor/docker/const.py(1 hunks)supervisor/docker/manager.py(1 hunks)supervisor/resolution/evaluations/container.py(2 hunks)tests/addons/test_addon.py(6 hunks)tests/addons/test_build.py(1 hunks)tests/api/test_addons.py(2 hunks)tests/common.py(1 hunks)tests/conftest.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- supervisor/docker/const.py
- supervisor/docker/manager.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/conftest.py
- tests/api/test_addons.py
- tests/addons/test_build.py
- supervisor/docker/addon.py
- tests/addons/test_addon.py
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in documentation, use a di...
*/**(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'
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/common.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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/common.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"
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/common.py
`*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/common.py
`*/**(html|markdown|md)`: do not comment on HTML used for icons
*/**(html|markdown|md): do not comment on HTML used for icons
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/common.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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/common.py
🧬 Code Graph Analysis (1)
supervisor/addons/build.py (3)
supervisor/addons/model.py (4)
path_location(570-572)version(234-236)arch(541-546)url(506-508)supervisor/coresys.py (4)
sys_config(696-698)arch(265-269)arch(272-276)sys_arch(726-728)supervisor/config.py (4)
path_supervisor(213-215)path_extern_supervisor(218-220)version(112-114)version(117-119)
🪛 Flake8 (7.2.0)
tests/common.py
[error] 116-116: whitespace before ':'
(E203)
🔇 Additional comments (3)
supervisor/addons/build.py (1)
136-192: LGTM! Well-structured Docker CLI command generation.The refactored method correctly constructs Docker CLI build commands with proper:
- Platform specification using
MAP_ARCH- Label and build argument handling
- Volume mounting for Docker socket and addon directory
- Working directory configuration
The approach aligns well with the PR objective of migrating to Docker BuildKit via CLI containers.
supervisor/resolution/evaluations/container.py (2)
8-8: LGTM! Necessary import for builder image integration.The import of
ADDON_BUILDER_IMAGEsupports the new Docker BuildKit approach by making the builder image available for registration in the known images set.
68-68: LGTM! Prevents false positives for builder container.Adding
ADDON_BUILDER_IMAGEto the known images set ensures the system won't flag the temporary builder container as unsupported during addon builds, which is essential for the new BuildKit-based build process.
| def is_in_list(a: list, b: list): | ||
| """Check if all elements in list a are in list b in order. | ||
|
|
||
| Taken from https://stackoverflow.com/a/69175987/12156188. | ||
| """ | ||
|
|
||
| for c in a: | ||
| if c in b: | ||
| b = b[b.index(c) :] | ||
| else: | ||
| return False | ||
| return True | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and improve input validation.
The function lacks type hints and input validation. Consider these improvements:
-def is_in_list(a: list, b: list):
+def is_in_list(a: list, b: list) -> bool:
"""Check if all elements in list a are in list b in order.
Taken from https://stackoverflow.com/a/69175987/12156188.
"""
+ if not a:
+ return True
+ if not b:
+ return False
- for c in a:
- if c in b:
- b = b[b.index(c) :]
- else:
- return False
- return True
+ b_copy = b[:] # Work with a copy to avoid modifying the input
+ for c in a:
+ if c in b_copy:
+ b_copy = b_copy[b_copy.index(c) :]
+ else:
+ return False
+ return True📝 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.
| def is_in_list(a: list, b: list): | |
| """Check if all elements in list a are in list b in order. | |
| Taken from https://stackoverflow.com/a/69175987/12156188. | |
| """ | |
| for c in a: | |
| if c in b: | |
| b = b[b.index(c) :] | |
| else: | |
| return False | |
| return True | |
| def is_in_list(a: list, b: list) -> bool: | |
| """Check if all elements in list a are in list b in order. | |
| Taken from https://stackoverflow.com/a/69175987/12156188. | |
| """ | |
| if not a: | |
| return True | |
| if not b: | |
| return False | |
| b_copy = b[:] # Work with a copy to avoid modifying the input | |
| for c in a: | |
| if c in b_copy: | |
| b_copy = b_copy[b_copy.index(c) :] | |
| else: | |
| return False | |
| return True |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 116-116: whitespace before ':'
(E203)
🤖 Prompt for AI Agents
In tests/common.py around lines 108 to 120, the function is_in_list lacks type
hints and input validation. Add precise type hints to the function signature
specifying the expected types of the input lists and the return type. Also,
include input validation to check that both inputs are lists and raise
appropriate errors if not, ensuring the function handles invalid inputs
gracefully.
There was a problem hiding this comment.
@agners would you like me to handle this? My first preference would be to keep the function as similar as possible to its StackOverflow reference.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
I'm not sure I should adhere to @coderabbitai's comments. |
|
Tip For best results, initiate chat on the files or code changes. @felipecrs You're absolutely right to be cautious! AI review suggestions, including mine, aren't always correct or necessary. You already demonstrated good judgment by pushing back on the Ruff formatter suggestion. Here's how I'd approach evaluating AI suggestions: Follow suggestions that:
Question suggestions that:
Your domain expertise and understanding of the Home Assistant codebase is invaluable. Trust your judgment, especially when you have specific reasons (like the Ruff formatter preference) to disagree. Focus on the suggestions that genuinely improve the code quality or fix real issues, and feel free to dismiss the rest. The human reviewer (@agners) will ultimately make the final call on what's important for this PR. |
|
@agners all comments handled. Also, re-tested. All good from my side. |
There was a problem hiding this comment.
I've tested a bunch of Core add-ons by removing the image tag and let it build using Supervisor itself. Some raised an exception because the args were not strings (e.g. the Let's Encrypt add-on). This is a not a new issue, we need to address that separatly. I guess we should make sure that all build args are actual strings, since that is what Docker passes.
2025-07-02 16:49:14.507 CRITICAL (MainThread) [supervisor.utils.common] Can't parse /data/addons/local/letsencrypt/build.yaml: expected str for dictionary value @ data['args']['CERTBOT_DNS_DREAMHOST_VERSION']. Got 1.0
expected str for dictionary value @ data['args']['CERTBOT_DNS_DUCKDNS_VERSION']. Got 1.5
2025-07-02 16:49:14.507 WARNING (MainThread) [supervisor.utils.common] Resetting /data/addons/local/letsencrypt/build.yaml to default
But with that manually fixed, all add-ons got build flawlessly. Even parallel builds seem to work just fine. Nice 🎉
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Stefan Agner <stefan@agner.ch>
|
@agners thanks a lot for welcoming my contribution. :D |
BTW it would be great to have some mechanism for rebuilding an addon even if Maybe always attempting to build when invoked by the |
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
New Features
Bug Fixes
Tests