Refactor git-pull #3875
Refactor git-pull #3875
Conversation
There is a bug present causing git-pull to no longer be able to restart or validate the configuration after updating the git repo. I suspect part of this is related bashio, possibly related to not using with-contenv. In this commit/pr I'm making use of the addon support present in bashio to avoid direct use of jq, and api calls. home-assistant#3714
…x a failure when dealing with checking if files matched
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
|
Warning Rate limit exceeded@raypappa has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 20 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 (1)
📝 WalkthroughWalkthroughThe pull request involves modifications to the Dockerfile and Changes
Sequence DiagramsequenceDiagram
participant Script as run.sh
participant Bashio as Bashio Library
participant Git as Git Repository
Script->>Bashio: Load configuration
Bashio-->>Script: Configuration data
Script->>Git: Check repository exists
alt Repository does not exist
Script->>Git: Clone repository
else Repository exists
Script->>Git: Synchronize changes
end
Script->>Bashio: Validate configuration
alt Configuration valid
Script->>Script: Restart if changes detected
else Configuration invalid
Script->>Bashio: Log error and exit
end
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: 0
🔭 Outside diff range comments (1)
git_pull/data/run.sh (1)
Add file permission restrictions to git credentials file
The verification shows that while the script properly sets secure permissions (600) for SSH-related files, it lacks similar protection for the git credentials file. The original review comment's concern is valid as the credentials file permissions are not explicitly set.
Recommended changes:
- Add
chmod 600 /tmp/git-credentialsafter storing the credentials to restrict read/write access to only the owner, similar to how SSH files are handled in the script.🔗 Analysis chain
Line range hint
79-114: Review security implications of credential storage.While storing credentials in
/tmp/git-credentialsis temporary, consider:
- The file permissions are not explicitly set
- Credentials are stored in plaintext
Consider adding file permission restrictions:
# Use git commands to write the credentials to ~/.git-credentials bashio::log.info "[Info] Saving git credentials to /tmp/git-credentials" # shellcheck disable=SC2259 git credential fill | git credential approve <<< "$cred_data" +# Restrict file permissions +chmod 600 /tmp/git-credentials
Let me verify the security implications by checking the codebase for any existing file permission handling.
Let me check if there are any file permission settings in the initialization or setup parts of the script.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check if there are any file permission settings for git-credentials rg -A 2 -B 2 "git-credentials" # Look for chmod commands related to credentials rg "chmod.*credentials" # Check if there are any other credential helper configurations rg "credential.helper"Length of output: 1004
Script:
#!/bin/bash # Check the entire run.sh file for any umask or permission related settings rg -A 2 -B 2 "umask|chmod" git_pull/data/run.sh # Check if there are any security-related initialization functions ast-grep --pattern 'function init$$$' git_pull/data/run.sh # Look for any file permission related settings in the script rg "0[0-9]{3}" git_pull/data/run.shLength of output: 278
🧹 Nitpick comments (5)
git_pull/data/run.sh (5)
67-77: Enhance error handling in SSH connection check.The SSH connection check could be more robust by:
- Adding explicit error message when SSH check fails
- Handling different SSH connection failure scenarios
Consider this improvement:
if OUTPUT_CHECK=$(ssh -T -o "StrictHostKeyChecking=no" -o "BatchMode=yes" "$DOMAIN" 2>&1) || { [[ $DOMAIN = *"@github.com"* ]] && [[ $OUTPUT_CHECK = *"You've successfully authenticated"* ]]; }; then bashio::log.info "[Info] Valid SSH connection for $DOMAIN" else - bashio::log.warn "[Warn] No valid SSH connection for $DOMAIN" + bashio::log.warn "[Warn] SSH connection failed for $DOMAIN: $OUTPUT_CHECK" add-ssh-key + # Verify connection after adding key + if ! ssh -T -o "StrictHostKeyChecking=no" -o "BatchMode=yes" "$DOMAIN" 2>&1; then + bashio::log.error "[Error] SSH connection still failing after adding key" + fi fi
136-138: Optimize git fetch operation.The current
git fetchoperation fetches all branches. For better performance, especially in repositories with many branches, consider fetching only the required branch.-git fetch "$GIT_REMOTE" || bashio::exit.nok "[Error] Git fetch failed"; +git fetch "$GIT_REMOTE" "$GIT_BRANCH" || bashio::exit.nok "[Error] Git fetch failed";
191-192: Enhance changed files logging.The current logging of changed files could be more readable for long lists of files.
-bashio::log.info "Changed Files: $CHANGED_FILES" +bashio::log.info "Changed Files:" +for file in $CHANGED_FILES; do + bashio::log.info " - $file" +done
Line range hint
235-246: Consider adding error recovery mechanism.The main loop could benefit from error recovery logic to handle temporary failures.
while true; do + # Add retry mechanism for temporary failures + RETRY_COUNT=0 + MAX_RETRIES=3 + while [ $RETRY_COUNT -lt $MAX_RETRIES ]; do check-ssh-key setup-user-password - if git-synchronize ; then + if git-synchronize ; then validate-config + break + else + RETRY_COUNT=$((RETRY_COUNT + 1)) + bashio::log.warning "Attempt $RETRY_COUNT of $MAX_RETRIES failed" + sleep 10 + fi + done fi # do we repeat? if [ ! "$REPEAT_ACTIVE" == "true" ]; then
Line range hint
1-246: Overall: Well-executed refactoring with good error handling.The transition to bashio and improved error handling aligns well with the PR objectives. The script should now handle Home Assistant restarts properly. The code is more maintainable and better integrated with the Home Assistant ecosystem.
A few suggestions have been made for further improvements, but none are blocking issues.
Consider implementing the suggested improvements in a follow-up PR:
- Enhanced SSH error handling
- Secure credential storage
- Optimized git fetch
- Improved change logging
- Error recovery mechanism
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
git_pull/Dockerfile(0 hunks)git_pull/data/run.sh(5 hunks)
💤 Files with no reviewable changes (1)
- git_pull/Dockerfile
🧰 Additional context used
🪛 Shellcheck (0.10.0)
git_pull/data/run.sh
[error] 1-1: This shebang was unrecognized. ShellCheck only supports sh/bash/dash/ksh/'busybox sh'. Add a 'shell' directive to specify.
(SC1008)
🔇 Additional comments (1)
git_pull/data/run.sh (1)
1-20: LGTM! Improved configuration management using bashio.
The transition from jq to bashio::config for configuration management is a good improvement. It provides better integration with Home Assistant's configuration system and more robust error handling.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: This shebang was unrecognized. ShellCheck only supports sh/bash/dash/ksh/'busybox sh'. Add a 'shell' directive to specify.
(SC1008)
… change suggested by the coderabbit to fetch the specific branch from git
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
git_pull/data/run.sh (1)
Line range hint
79-114: Enhance credential securityStoring credentials in plaintext in /tmp poses a security risk. Consider:
- Using memory-based storage (tmpfs)
- Implementing credential cleanup
+# Ensure /tmp/git-credentials is on tmpfs +if ! mountpoint -q /tmp; then + bashio::log.warn "[Warn] /tmp is not a tmpfs mount point" +fi git config --system credential.helper 'store --file=/tmp/git-credentials' + +# Schedule cleanup of credentials +trap 'rm -f /tmp/git-credentials' EXIT
🧹 Nitpick comments (5)
git_pull/data/run.sh (5)
Line range hint
59-62: Improve error handling for file restorationThe silent failure of cp commands could lead to missing files without notification. Consider logging the status of these operations.
-cp "${BACKUP_LOCATION}" "!(*.yaml)" /config 2>/dev/null +if ! cp "${BACKUP_LOCATION}" "!(*.yaml)" /config 2>/dev/null; then + bashio::log.info "[Info] No non-yaml files to restore from backup" +fi -cp "${BACKUP_LOCATION}/secrets.yaml" /config 2>/dev/null +if ! cp "${BACKUP_LOCATION}/secrets.yaml" /config 2>/dev/null; then + bashio::log.info "[Info] No secrets.yaml to restore from backup" +fi🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 43-43: Couldn't parse this brace group. Fix to allow more checks.
(SC1073)
65-77: Enhance domain extraction robustnessThe current domain extraction might not handle all Git URL formats correctly. Consider using git-url-parse or a more robust parsing approach.
-IFS=':' read -ra GIT_URL_PARTS <<< "$REPOSITORY" -DOMAIN="${GIT_URL_PARTS[0]}" +DOMAIN=$(echo "$REPOSITORY" | sed -E 's#^(git@|https?://)([^/:]+).*$#\2#')
129-131: Remove unreachable codeThe return statement after
bashio::exit.nokis unreachable.- bashio::exit.nok "[Error] git origin does not match $REPOSITORY!"; - return + bashio::exit.nok "[Error] git origin does not match $REPOSITORY!"
200-206: Improve error handling in file matchingUsing
set +e/-efor grep could be replaced with a more idiomatic approach.- set +e - restart_required_file=$(echo "${changed_file}" | grep "^${restart_ignored_file}") - set -e + restart_required_file=$(echo "${changed_file}" | grep -E "^${restart_ignored_file}" || true)
Line range hint
232-246: Add validation for repeat intervalConsider validating the repeat interval to ensure it's a positive number.
+# Validate repeat interval +if [ "$REPEAT_ACTIVE" == "true" ] && ! [[ "$REPEAT_INTERVAL" =~ ^[0-9]+$ ]]; then + bashio::exit.nok "[Error] Repeat interval must be a positive number" +fi while true; do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
git_pull/data/run.sh(5 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
git_pull/data/run.sh
[error] 1-1: This shebang was unrecognized. ShellCheck only supports sh/bash/dash/ksh/'busybox sh'. Add a 'shell' directive to specify.
(SC1008)
🔇 Additional comments (3)
git_pull/data/run.sh (3)
1-20: Well-structured configuration setup using bashio!
The transition to bashio::config from jq improves reliability and maintainability. The configuration setup properly handles all necessary parameters including arrays.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: This shebang was unrecognized. ShellCheck only supports sh/bash/dash/ksh/'busybox sh'. Add a 'shell' directive to specify.
(SC1008)
136-170: Well-implemented git synchronization logic!
The git synchronization implementation is robust with proper error handling and support for different sync strategies (pull/reset).
Line range hint 24-41: Consider security implications of disabled StrictHostKeyChecking
While disabling StrictHostKeyChecking makes deployment easier, it bypasses SSH host verification which could expose the system to MITM attacks. Consider:
- Limiting this to specific known hosts
- Adding key validation before writing to the file
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: This shebang was unrecognized. ShellCheck only supports sh/bash/dash/ksh/'busybox sh'. Add a 'shell' directive to specify.
(SC1008)
git_pull/data/run.sh
Outdated
|
|
||
| # remove config folder content | ||
| rm -rf /config/{,.[!.],..?}* || { echo "[Error] Clearing /config failed"; exit 1; } | ||
| rm -rf /config/{,.[!.],..?}* || { bashio::exit.nok "[Error] Clearing /config failed" |
There was a problem hiding this comment.
Fix syntax error in rm command
There's a missing closing brace in the rm command.
-rm -rf /config/{,.[!.],..?}* || { bashio::exit.nok "[Error] Clearing /config failed"
+rm -rf /config/{,.[!.],..?}* || { bashio::exit.nok "[Error] Clearing /config failed"; }📝 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.
| rm -rf /config/{,.[!.],..?}* || { bashio::exit.nok "[Error] Clearing /config failed" | |
| rm -rf /config/{,.[!.],..?}* || { bashio::exit.nok "[Error] Clearing /config failed"; } |
…ho and exit, this is now being handled entirely in the exit.nok function
I noticed that git pull was error'ing out when trying to restart HA due to a lack of supervisor token. I opted to update the script to use bashio contexts and functions to smooth out the use of it.
Summary by CodeRabbit
run.shscript to use thebashiolibrary for improved configuration management.