[tui] Implement #135 for update #246
Conversation
WalkthroughThis pull request introduces enhancements to the Open Voice OS (OVOS) installer, focusing on state management, profile configuration, and installation/uninstallation processes. The changes include adding new directories for backup, implementing dynamic feature and profile management, and introducing new utility functions for state tracking. The modifications aim to improve the installer's flexibility and user experience by providing more granular control over installation settings and preserving user configurations. Changes
Possibly related PRs
Suggested labels
Poem
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: 6
🧹 Nitpick comments (2)
tui/update.sh (1)
9-14: Add validation for TUI window dimensions.The script uses
$TUI_WINDOW_HEIGHTand$TUI_WINDOW_WIDTHwithout validation.Add dimension validation:
+# Validate TUI dimensions +if [ -z "$TUI_WINDOW_HEIGHT" ] || [ -z "$TUI_WINDOW_WIDTH" ]; then + echo "Error: TUI window dimensions not set" >&2 + exit 1 +fi + whiptail --yesno --no-button "$NO_BUTTON" --yes-button "$YES_BUTTON" \ --title "$TITLE" "$CONTENT" "$TUI_WINDOW_HEIGHT" "$TUI_WINDOW_WIDTH"utils/common.sh (1)
551-565: Consider enhancing the state directory function with better error handling and validation.While the implementation is functional, consider the following improvements:
- Add error handling for directory creation and ownership changes.
- Validate the JSON structure of the state file.
Here's a suggested implementation with improved error handling and validation:
function state_directory() { OVOS_LOCAL_STATE_DIRECTORY="$RUN_AS_HOME/.local/state/ovos" export INSTALLER_STATE_FILE="$OVOS_LOCAL_STATE_DIRECTORY/installer.json" if [ ! -d "$OVOS_LOCAL_STATE_DIRECTORY" ]; then - mkdir -p "$OVOS_LOCAL_STATE_DIRECTORY" &>>"$LOG_FILE" - chown -R "$RUN_AS":"$(id -ng "$RUN_AS")" "$OVOS_LOCAL_STATE_DIRECTORY" &>>"$LOG_FILE" + if ! mkdir -p "$OVOS_LOCAL_STATE_DIRECTORY" &>>"$LOG_FILE"; then + echo "Failed to create state directory" &>>"$LOG_FILE" + return 1 + fi + if ! chown -R "$RUN_AS":"$(id -ng "$RUN_AS")" "$OVOS_LOCAL_STATE_DIRECTORY" &>>"$LOG_FILE"; then + echo "Failed to set state directory ownership" &>>"$LOG_FILE" + return 1 + fi fi if [ -f "$INSTALLER_STATE_FILE" ]; then - [ -s "$INSTALLER_STATE_FILE" ] || rm "$INSTALLER_STATE_FILE" &>>"$LOG_FILE" + if [ -s "$INSTALLER_STATE_FILE" ]; then + if ! jq empty "$INSTALLER_STATE_FILE" &>>"$LOG_FILE"; then + echo "Invalid JSON in state file" &>>"$LOG_FILE" + return 1 + fi + else + rm "$INSTALLER_STATE_FILE" &>>"$LOG_FILE" + fi fi }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ansible/roles/ovos_installer/tasks/ovos.yml(1 hunks)ansible/roles/ovos_installer/tasks/virtualenv/systemd.yml(1 hunks)setup.sh(1 hunks)tui/features.sh(2 hunks)tui/locales/en-us/update.sh(1 hunks)tui/profiles.sh(2 hunks)tui/uninstall.sh(1 hunks)tui/update.sh(1 hunks)utils/common.sh(1 hunks)utils/constants.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tui/locales/en-us/update.sh
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: shell-testing
🔇 Additional comments (3)
ansible/roles/ovos_installer/tasks/ovos.yml (1)
13-13: LGTM! The state directory is now properly included in the backup process.The addition of
.local/state/ovos/*to the backup list ensures that state information is preserved during uninstallation, which aligns well with the new state management functionality.Let's verify the integration with other changes:
✅ Verification successful
Verified: State directory is properly managed and consistently implemented
The state directory
.local/state/ovosis correctly set up, managed, and included in the backup process across all relevant parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the state directory is consistently handled across the codebase. # Test 1: Check if the state directory is properly managed in other tasks rg -A 5 "\.local/state/ovos" # Test 2: Check if there are any other state-related paths that should be included rg -A 5 "state.*directory"Length of output: 4873
setup.sh (1)
48-48: LGTM! The state directory initialization is properly sequenced.The
state_directoryfunction is called at an appropriate point in the installation process, ensuring state management is initialized after system checks but before proceeding with the installation.Let's verify the function call sequence:
✅ Verification successful
Verification confirms proper function sequencing
The
state_directoryfunction is correctly positioned in the installation flow:
- After system dependencies and hardware detection
- Before scenario-specific operations
- With proper error handling in place
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the sequence of function calls and error handling. # Test 1: Check the sequence of function calls around state_directory rg -B 5 -A 5 "state_directory" # Test 2: Check if there are any dependencies between the functions ast-grep --pattern 'function $func() { $$$ state_directory $$$ }'Length of output: 1160
ansible/roles/ovos_installer/tasks/virtualenv/systemd.yml (1)
138-138: LGTM! The state directory is properly included in the cleanup process.The addition of
.local/state/ovosto the cleanup list ensures proper removal of state information during uninstallation, complementing the backup functionality.Let's verify the cleanup process:
✅ Verification successful
State directory cleanup implementation is verified and complete
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of directory cleanup during uninstallation. # Test 1: Check if all state-related directories are properly cleaned up rg -A 5 "state: absent.*ovos" # Test 2: Check for any potential leftover directories rg -A 5 "\.local/state"Length of output: 1527
| #!/bin/env bash | ||
|
|
||
| # shellcheck source=locales/en-us/misc.sh | ||
| source "tui/locales/$LOCALE/misc.sh" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for locale file sourcing.
The script assumes locale files exist but doesn't handle cases where they might be missing.
Add error checking:
-source "tui/locales/$LOCALE/misc.sh"
+if [ ! -f "tui/locales/$LOCALE/misc.sh" ]; then
+ echo "Error: Missing locale file tui/locales/$LOCALE/misc.sh" >&2
+ exit 1
+fi
+source "tui/locales/$LOCALE/misc.sh"Also applies to: 7-7
| # shellcheck source=update.sh | ||
| source tui/update.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve user flow clarity.
When uninstall is declined, the script directly sources update.sh without error handling or user notification.
Add error handling and user notification:
- # shellcheck source=update.sh
- source tui/update.sh
+ # shellcheck source=update.sh
+ if [ ! -f "tui/update.sh" ]; then
+ echo "Error: Update script not found" >&2
+ exit 1
+ fi
+ whiptail --msgbox --title "$TITLE" "Proceeding to update menu..." \
+ "$TUI_WINDOW_HEIGHT" "$TUI_WINDOW_WIDTH"
+ source tui/update.sh📝 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.
| # shellcheck source=update.sh | |
| source tui/update.sh | |
| # shellcheck source=update.sh | |
| if [ ! -f "tui/update.sh" ]; then | |
| echo "Error: Update script not found" >&2 | |
| exit 1 | |
| fi | |
| whiptail --msgbox --title "$TITLE" "Proceeding to update menu..." \ | |
| "$TUI_WINDOW_HEIGHT" "$TUI_WINDOW_WIDTH" | |
| source tui/update.sh |
| @@ -31,3 +41,6 @@ if [ -z "$PROFILE" ]; then | |||
| source tui/channels.sh | |||
| source tui/profiles.sh | |||
| fi | |||
|
|
|||
| jq -en --arg profile "$PROFILE" '.profile += $profile' > "$TEMP_PROFILE_FILE" | |||
There was a problem hiding this comment.
Handle potential race conditions with temporary files.
The script writes to a temporary file without proper locking mechanisms.
Add file locking:
-jq -en --arg profile "$PROFILE" '.profile += $profile' > "$TEMP_PROFILE_FILE"
+(
+ flock -w 10 200 || {
+ echo "Error: Failed to acquire lock" >&2
+ exit 1
+ }
+ jq -en --arg profile "$PROFILE" '.profile += $profile' > "$TEMP_PROFILE_FILE"
+) 200>"${TEMP_PROFILE_FILE}.lock"📝 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.
| jq -en --arg profile "$PROFILE" '.profile += $profile' > "$TEMP_PROFILE_FILE" | |
| ( | |
| flock -w 10 200 || { | |
| echo "Error: Failed to acquire lock" >&2 | |
| exit 1 | |
| } | |
| jq -en --arg profile "$PROFILE" '.profile += $profile' > "$TEMP_PROFILE_FILE" | |
| ) 200>"${TEMP_PROFILE_FILE}.lock" |
| if [ -f "$INSTALLER_STATE_FILE" ]; then | ||
| if jq -e 'has("profile")' "$INSTALLER_STATE_FILE" &>>"$LOG_FILE"; then | ||
| current_profile=$(jq -re '.profile' "$INSTALLER_STATE_FILE") | ||
| active_profile="$current_profile" | ||
| available_profiles=("$current_profile") | ||
| fi |
There was a problem hiding this comment.
Add error handling for JSON operations.
The script uses jq without proper error handling, which could lead to silent failures.
Add error handling:
if [ -f "$INSTALLER_STATE_FILE" ]; then
- if jq -e 'has("profile")' "$INSTALLER_STATE_FILE" &>>"$LOG_FILE"; then
- current_profile=$(jq -re '.profile' "$INSTALLER_STATE_FILE")
+ if ! jq -e 'has("profile")' "$INSTALLER_STATE_FILE" &>>"$LOG_FILE"; then
+ echo "Error: Invalid state file format" >&2
+ exit 1
+ fi
+ current_profile=$(jq -re '.profile' "$INSTALLER_STATE_FILE") || {
+ echo "Error: Failed to read profile from state file" >&2
+ exit 1
+ }📝 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.
| if [ -f "$INSTALLER_STATE_FILE" ]; then | |
| if jq -e 'has("profile")' "$INSTALLER_STATE_FILE" &>>"$LOG_FILE"; then | |
| current_profile=$(jq -re '.profile' "$INSTALLER_STATE_FILE") | |
| active_profile="$current_profile" | |
| available_profiles=("$current_profile") | |
| fi | |
| if [ -f "$INSTALLER_STATE_FILE" ]; then | |
| if ! jq -e 'has("profile")' "$INSTALLER_STATE_FILE" &>>"$LOG_FILE"; then | |
| echo "Error: Invalid state file format" >&2 | |
| exit 1 | |
| fi | |
| current_profile=$(jq -re '.profile' "$INSTALLER_STATE_FILE") || { | |
| echo "Error: Failed to read profile from state file" >&2 | |
| exit 1 | |
| } | |
| active_profile="$current_profile" | |
| available_profiles=("$current_profile") | |
| fi |
| if [ "$exit_status" -ne 1 ]; then | ||
| jq -en '.features += $ARGS.positional' --args "${FEATURES_STATE[@]}" > "$TEMP_FEATURES_FILE" | ||
| jq -es '.[0] * .[1]' "$TEMP_PROFILE_FILE" "$TEMP_FEATURES_FILE" > "$INSTALLER_STATE_FILE" | ||
| rm "$TEMP_FEATURES_FILE" "$TEMP_PROFILE_FILE" | ||
| fi |
There was a problem hiding this comment.
Improve temporary file handling and cleanup.
The script needs better error handling and cleanup for temporary files.
Add proper cleanup and error handling:
+cleanup() {
+ rm -f "$TEMP_FEATURES_FILE" "$TEMP_PROFILE_FILE"
+}
+trap cleanup EXIT
+
if [ "$exit_status" -ne 1 ]; then
- jq -en '.features += $ARGS.positional' --args "${FEATURES_STATE[@]}" > "$TEMP_FEATURES_FILE"
- jq -es '.[0] * .[1]' "$TEMP_PROFILE_FILE" "$TEMP_FEATURES_FILE" > "$INSTALLER_STATE_FILE"
- rm "$TEMP_FEATURES_FILE" "$TEMP_PROFILE_FILE"
+ jq -en '.features += $ARGS.positional' --args "${FEATURES_STATE[@]}" > "$TEMP_FEATURES_FILE" || {
+ echo "Error: Failed to create features file" >&2
+ exit 1
+ }
+ jq -es '.[0] * .[1]' "$TEMP_PROFILE_FILE" "$TEMP_FEATURES_FILE" > "$INSTALLER_STATE_FILE" || {
+ echo "Error: Failed to merge state files" >&2
+ exit 1
+ }
fi📝 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.
| if [ "$exit_status" -ne 1 ]; then | |
| jq -en '.features += $ARGS.positional' --args "${FEATURES_STATE[@]}" > "$TEMP_FEATURES_FILE" | |
| jq -es '.[0] * .[1]' "$TEMP_PROFILE_FILE" "$TEMP_FEATURES_FILE" > "$INSTALLER_STATE_FILE" | |
| rm "$TEMP_FEATURES_FILE" "$TEMP_PROFILE_FILE" | |
| fi | |
| cleanup() { | |
| rm -f "$TEMP_FEATURES_FILE" "$TEMP_PROFILE_FILE" | |
| } | |
| trap cleanup EXIT | |
| if [ "$exit_status" -ne 1 ]; then | |
| jq -en '.features += $ARGS.positional' --args "${FEATURES_STATE[@]}" > "$TEMP_FEATURES_FILE" || { | |
| echo "Error: Failed to create features file" >&2 | |
| exit 1 | |
| } | |
| jq -es '.[0] * .[1]' "$TEMP_PROFILE_FILE" "$TEMP_FEATURES_FILE" > "$INSTALLER_STATE_FILE" || { | |
| echo "Error: Failed to merge state files" >&2 | |
| exit 1 | |
| } | |
| fi |
| export TEMP_FEATURES_FILE=/tmp/features.json | ||
| export TEMP_PROFILE_FILE=/tmp/profile.json |
There was a problem hiding this comment.
Use secure temporary file paths.
Using hardcoded paths in /tmp could lead to security issues in multi-user environments.
Use mktemp for secure temporary files:
-export TEMP_FEATURES_FILE=/tmp/features.json
-export TEMP_PROFILE_FILE=/tmp/profile.json
+export TEMP_FEATURES_FILE
+export TEMP_PROFILE_FILE
+TEMP_FEATURES_FILE=$(mktemp -t ovos-features.XXXXXX.json)
+TEMP_PROFILE_FILE=$(mktemp -t ovos-profile.XXXXXX.json)📝 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.
| export TEMP_FEATURES_FILE=/tmp/features.json | |
| export TEMP_PROFILE_FILE=/tmp/profile.json | |
| export TEMP_FEATURES_FILE | |
| export TEMP_PROFILE_FILE | |
| TEMP_FEATURES_FILE=$(mktemp -t ovos-features.XXXXXX.json) | |
| TEMP_PROFILE_FILE=$(mktemp -t ovos-profile.XXXXXX.json) |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes