RFC: Add Makefile variable for Supervisor channel #3618
Conversation
Allow to set the release channel pre-installed Home Assistant components like Supervisor and add-on are fetched from. This channel is then also used at runtime.
agners
changed the title
|
I was considering making this a package config, like so: --- a/buildroot-external/package/hassio/Config.in
+++ b/buildroot-external/package/hassio/Config.in
@@ -18,4 +18,10 @@ config BR2_PACKAGE_HASSIO_MACHINE
help
Machine to pull containers for (used for landing page).
+config BR2_PACKAGE_HASSIO_CHANNEL
+ string "Default Channel"
+ default "beta"
+ help
+ Channel to use by default.
+
endifBut then it can't be overriden from Makefile (at least it didn't work for me 🤔 ). I think we should rename the variable to |
📝 WalkthroughWalkthroughThe pull request introduces a new variable Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Outside diff range and nitpick comments (6)
buildroot-external/package/hassio/create-data-partition.sh (2)
6-6: LGTM! Consider adding parameter validation.The addition of the
channelparameter aligns with the PR objectives. However, to improve robustness, consider adding parameter validation and a default value.You could modify the script to include validation and a default value like this:
channel=$3 + +# Set default channel if not provided +channel=${channel:-stable} + +# Validate channel +if [[ ! "$channel" =~ ^(stable|beta|dev)$ ]]; then + echo "Error: Invalid channel. Must be 'stable', 'beta', or 'dev'." + exit 1 +fiThis ensures that the script always has a valid channel value, even if it's not explicitly provided.
28-28: LGTM! Consider adding a comment for clarity.The modification correctly passes the
channelparameter to thedind-import-containers.shscript, aligning with the PR objectives.For improved readability, consider adding a comment explaining the purpose of this line:
+# Execute container import script with specified channel docker exec "${container}" sh /build/dind-import-containers.sh "${channel}"This addition would make the script's intention clearer to future maintainers.
buildroot-external/package/hassio/dind-import-containers.sh (3)
4-4: LGTM! Consider adding input validation.The addition of the
channelvariable aligns well with the PR objectives, allowing users to set the release channel. This change enhances the script's flexibility.Consider adding input validation or a default value for the
channelvariable. This would prevent potential issues if the script is called without an argument. For example:channel=${1:-stable}This sets 'stable' as the default if no argument is provided.
33-33: LGTM! Consider adding a directory check.The creation of the JSON file with the channel information is a good addition. It provides a structured way to store and access the channel information, which aligns with the PR objectives.
Consider adding a check to ensure the
/data/supervisordirectory exists before creating the file. This would prevent potential errors if the directory structure is not as expected. For example:mkdir -p /data/supervisor echo "{ \"channel\": \"${channel}\" }" > /data/supervisor/updater.jsonThis ensures the directory exists before attempting to create the file.
Line range hint
1-33: Overall, the changes look good and align with the PR objectives.The addition of the
channelvariable and the creation of the JSON file with channel information successfully implement the feature to set and store the release channel. The changes are well-integrated into the existing script structure.To further improve the script's robustness and maintainability, consider:
- Implementing error handling for the channel parameter.
- Adding comments to explain the purpose of the new additions, especially the JSON file creation.
- If this channel information is used elsewhere in the system, ensure consistent naming and structure across different components.
Makefile (1)
34-35: LGTM: Effective integration of channel variable into build process.The modification to include
BR2_PACKAGE_HASSIO_CHANNEL=$(CHANNEL)in the make command effectively integrates the new channel variable into the build process. This change ensures that the specified channel is used during the build, aligning with the PR objectives.For improved readability, consider adding a comment explaining the purpose of the new parameter:
$(MAKE) -C $(BUILDROOT) O=$(O) BR2_EXTERNAL=$(BUILDROOT_EXTERNAL) \ + # Pass the channel information to Buildroot BR2_PACKAGE_HASSIO_CHANNEL=$(CHANNEL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- Makefile (2 hunks)
- buildroot-external/package/hassio/create-data-partition.sh (2 hunks)
- buildroot-external/package/hassio/dind-import-containers.sh (2 hunks)
- buildroot-external/package/hassio/hassio.mk (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
buildroot-external/package/hassio/create-data-partition.sh (1)
Line range hint
1-32: Overall, the changes effectively implement the channel selection feature.The modifications to this script successfully incorporate the new
channelparameter, allowing for flexible release channel selection as per the PR objectives. The changes are minimal yet impactful, maintaining the overall structure and readability of the script.To further enhance the script:
- Consider adding parameter validation and a default value for the
channelvariable.- A brief comment explaining the purpose of the
dind-import-containers.shcall would improve clarity.These suggestions would make the script more robust and easier to maintain in the future.
buildroot-external/package/hassio/hassio.mk (2)
Line range hint
1-33: Overall assessment: Changes align well with PR objectivesThe modifications to this Makefile effectively implement the feature allowing users to set the release channel for pre-installed Home Assistant components. The changes are consistent and well-integrated into the existing structure.
Key points:
- The HASSIO_VERSION_URL now dynamically uses the specified channel.
- The channel information is passed to the create-data-partition.sh script during installation.
These changes provide the desired flexibility in component sourcing, both during installation and at runtime. The verification tasks will help ensure the completeness of the implementation across the codebase.
33-33: LGTM: Channel information passed to create-data-partition.shThe update to pass
$(BR2_PACKAGE_HASSIO_CHANNEL)as an argument to the create-data-partition.sh script is consistent with the PR objective and the previous change. This allows the script to use the specified channel during the installation process.Let's verify if the create-data-partition.sh script has been updated to handle this new argument:
#!/bin/bash # Description: Verify the create-data-partition.sh script for the new channel argument # Find the create-data-partition.sh script script_path=$(fd -t f "create-data-partition.sh") if [ -n "$script_path" ]; then echo "Found create-data-partition.sh at: $script_path" echo "\nChecking for channel argument handling:" rg -n "channel" "$script_path" else echo "create-data-partition.sh not found. Please verify the script location." fiMakefile (2)
6-6: LGTM: Well-implemented channel variable.The addition of the
CHANNELvariable with a default value of "stable" is a good implementation. Using the?=operator allows for easy overriding during the build process, providing flexibility for users to specify different channels as needed. This change aligns well with the PR objective of allowing users to set the release channel for pre-installed Home Assistant components.
Line range hint
1-53: Overall: Well-implemented feature for channel specification.The changes to the Makefile effectively implement the desired feature of allowing channel specification for pre-installed Home Assistant components. The addition of the
CHANNELvariable and its integration into the build process are minimal, focused, and maintain the existing structure and style of the Makefile.These modifications align well with the PR objectives and provide the flexibility for users to specify different channels during the build process. The implementation is clean and doesn't introduce any apparent issues or conflicts with the existing code.
| @@ -9,7 +9,7 @@ HASSIO_LICENSE = Apache License 2.0 | |||
| # HASSIO_LICENSE_FILES = $(BR2_EXTERNAL_HASSOS_PATH)/../LICENSE | |||
| HASSIO_SITE = $(BR2_EXTERNAL_HASSOS_PATH)/package/hassio | |||
| HASSIO_SITE_METHOD = local | |||
| HASSIO_VERSION_URL = "https://version.home-assistant.io/stable.json" | |||
| HASSIO_VERSION_URL = "https://version.home-assistant.io/$(BR2_PACKAGE_HASSIO_CHANNEL).json" | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: BR2_PACKAGE_HASSIO_CHANNEL is used but not defined.
- The variable
BR2_PACKAGE_HASSIO_CHANNELis utilized inbuildroot-external/package/hassio/create-data-partition.shbut its definition is missing in the Makefiles. - Ensure that
BR2_PACKAGE_HASSIO_CHANNELis properly defined and set within the appropriate configuration files to avoid potential build or runtime issues.
🔗 Analysis chain
LGTM: Dynamic channel selection implemented.
The change to use $(BR2_PACKAGE_HASSIO_CHANNEL) in the HASSIO_VERSION_URL allows for dynamic selection of the version URL based on the specified channel. This aligns well with the PR objective of allowing users to set the release channel for pre-installed Home Assistant components.
Let's verify the existence and setting of the BR2_PACKAGE_HASSIO_CHANNEL variable:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and setting of BR2_PACKAGE_HASSIO_CHANNEL
# Search for the definition of BR2_PACKAGE_HASSIO_CHANNEL
echo "Searching for BR2_PACKAGE_HASSIO_CHANNEL definition:"
rg -n "BR2_PACKAGE_HASSIO_CHANNEL" --type make
# Check if it's used in other places
echo "\nChecking usage of BR2_PACKAGE_HASSIO_CHANNEL in other files:"
rg -n "BR2_PACKAGE_HASSIO_CHANNEL" --type-not make
Length of output: 637
There was a problem hiding this comment.
Since this only affects the hassio package, I'll prefer having it as a (choice) config option there, so instead of passing a variable when running make, changing it would either mean to set it in the menuconfig or replace it in the .config (or just simply append it to the end). It's a little bit more complicated but IMO more appropriate.
Allow to set the release channel pre-installed Home Assistant components like Supervisor and add-on are fetched from. This channel is then also used at runtime.
Summary by CodeRabbit
New Features
Improvements