Add CI workflow and test script for CasaOS Docker fix#68
Conversation
Introduces a GitHub Actions workflow to automate testing of the CasaOS Docker API version fix script. Adds a comprehensive test-script.sh for simulating Docker upgrades, verifying the fix, and testing edge cases. Enhances run.sh with non-interactive mode support and colored output, and updates confirmation prompts to respect the NON_INTERACTIVE environment variable.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a new GitHub Actions workflow and a large Bash test harness; updates the fix script to support NON_INTERACTIVE mode with a new print_info helper and automated prompt handling for CI runs. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant TST as test-script.sh
participant RUN as run.sh
participant DO as Docker
participant CS as CasaOS
GH->>TST: run `test-script.sh full` (NON_INTERACTIVE=true)
TST->>TST: detect_os(), check_casaos_installed()
TST->>DO: get_docker_version()/get_docker_api_version()
alt perform upgrade simulation
TST->>DO: install_docker_version()/upgrade_docker_to_latest()
DO-->>TST: new version/API reported
end
TST->>RUN: invoke `run.sh` (NON_INTERACTIVE)
RUN->>RUN: print_info(), non-interactive branches
RUN->>DO: apply fixes / override API settings
RUN-->>TST: exit status
TST->>DO: verify API compatibility
TST->>CS: restart_casaos()/check services
TST-->>GH: emit test outcome / exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
casaos-fix-docker-api-version/run.sh (1)
27-59: Define or remove the unusedprint_info()function.The
print_info()function is defined but never called withinrun.sh. The script continues to use plainechostatements for all output. Either refactor to use this function throughout (for consistency and colored output), or remove it if it's not needed.Note:
test-script.shdefines its ownprint_info()with a different signature (1 arg vs. 2), so these are not shared utilities..github/workflows/test-casaos-fix.yml (1)
26-29: Consider adding error handling and early exit on test failure.The workflow runs the full test but doesn't explicitly fail the GitHub Actions job if the test exits with an error code. The
sudo -E ./test-script.sh fullline will propagate the exit code, but adding an explicit exit check or usingset -ewould make intent clearer.- name: Run full test working-directory: casaos-fix-docker-api-version run: | set -e sudo -E ./test-script.sh fullThis is optional for the Chill review level but recommended for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
.github/workflows/test-casaos-fix.yml(1 hunks)casaos-fix-docker-api-version/run.sh(3 hunks)casaos-fix-docker-api-version/test-script.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
casaos-fix-docker-api-version/test-script.sh
[warning] 63-63: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 73-73: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 97-97: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 98-98: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 99-99: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 242-242: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 243-243: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 359-359: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 360-360: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 367-367: Decimals are not supported. Either use integers only, or use bc or awk to compare.
(SC2072)
[warning] 383-383: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 384-384: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 409-409: This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten.
(SC2320)
[warning] 419-419: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 420-420: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 421-421: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 472-472: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 575-575: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 613-613: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 614-614: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 720-720: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 819-819: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 820-820: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 939-939: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 942-942: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1023-1023: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1076-1076: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1091-1091: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1110-1110: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1354-1354: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1357-1357: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1437-1437: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1438-1438: func_exit appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (2)
casaos-fix-docker-api-version/test-script.sh (1)
1-14: Approve overall structure and test coverage.The test harness provides comprehensive coverage of Docker API versions (27.x, 28.x, 29.x), GPG key conflicts, download resilience, timeout handling, and network namespace cleanup. NON_INTERACTIVE mode integration is well-implemented, with conditional prompts throughout. The modular function design makes individual test scenarios easy to execute and debug.
.github/workflows/test-casaos-fix.yml (1)
1-29: Approve GitHub Actions workflow structure.The workflow correctly:
- Triggers on relevant file changes and manual dispatch
- Uses
sudo -Eto preserveNON_INTERACTIVEenvironment variable through elevated commands- Sets working directory for test-script.sh execution
- Makes scripts executable before running
The
fulltest mode enables automated upgrade + fix validation without interactive prompts.
Corrects the YIGHLIGHT variable to YELLOW for output coloring. Updates Docker API version comparison to use awk for proper numeric comparison. Also improves error reporting in test_fix_script by capturing and returning the actual exit code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
casaos-fix-docker-api-version/test-script.sh (1)
1437-1442: Remove unused variablefunc_exit.Line 1439 captures
func_exit=$?but it is never used in the subsequent logic (line 1446 only checksduration). Remove the assignment or use it for validation.# Run with mocked docker export PATH="$mock_dir:$PATH" start_time=$(date +%s) local output=$($test_script 2>&1) - local func_exit=$? end_time=$(date +%s) duration=$((end_time - start_time)) export PATH="$old_path"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
casaos-fix-docker-api-version/test-script.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
casaos-fix-docker-api-version/test-script.sh (1)
casaos-fix-docker-api-version/run.sh (7)
get_docker_api_version(324-353)print_info(28-59)detect_os(111-122)show_usage(1466-1489)apply_docker_api_override(550-627)remove_docker_api_override(630-668)main(1566-2160)
🪛 Shellcheck (0.11.0)
casaos-fix-docker-api-version/test-script.sh
[warning] 63-63: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 73-73: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 97-97: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 98-98: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 99-99: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 242-242: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 243-243: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 359-359: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 360-360: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 383-383: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 384-384: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 420-420: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 421-421: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 422-422: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 473-473: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 576-576: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 614-614: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 615-615: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 721-721: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 820-820: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 821-821: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 940-940: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 943-943: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1024-1024: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1077-1077: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1092-1092: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1111-1111: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1355-1355: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1358-1358: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1438-1438: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1439-1439: func_exit appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (9)
casaos-fix-docker-api-version/test-script.sh (9)
28-33: [Past issue resolved] Color definitions corrected.The YIGHLIGHT typo (flagged in the previous review at line 31) has been fixed. Color definitions are now correct and properly use standard ANSI color names (YELLOW, RED, GREEN, BLUE, etc.).
60-92: LGTM — Defensive timeout handling for Docker queries.Functions properly use 10-second timeouts to prevent hanging on unresponsive daemons. The fallback patterns ensure safe defaults.
359-376: [Past issue resolved] Decimal comparison for API versions corrected.Line 367 now correctly uses
awkfor decimal comparison of API versions (e.g.,ver >= 1.45) instead of string comparison. This ensures proper numeric ordering and consistency with other version checks in the script.
402-412: [Past issue resolved] Exit code capture improved.The script now properly captures the exit code at line 407 (
local exit_code=$?) before callingprint_error(), avoiding the masking issue flagged in the previous review. The captured code is then correctly passed to the error message.
914-1056: LGTM — Comprehensive snap daemon hang test.The test properly simulates hang scenarios, verifies timeout behavior, and tests the actual function. Cleanup is thorough and mock setup is well-isolated.
1463-1566: LGTM — Well-orchestrated test suite.The function properly sequences all bug fix tests, respects NON_INTERACTIVE mode, accumulates results, and reports a comprehensive summary.
1568-1631: LGTM — Comprehensive command dispatcher.The main function properly handles all test modes with convenient aliases and good error reporting. Unknown commands trigger help output appropriately.
708-857: LGTM — Defensive GPG key testing.Both functions properly simulate failure scenarios, use backup/restore patterns for safety, and verify fixes. File validation for PGP keys is thorough.
Also applies to: 859-912
1058-1172: LGTM — Thorough network namespace test with defensive cleanup.The function properly handles container creation failures, simulates "device busy" scenarios, and verifies Docker remains functional after cleanup.
Introduces timeout handling for 'casaos -v' in run.sh to prevent script hangs if CasaOS is in a broken state. Updates test-script.sh to add a dedicated test for CasaOS version check hang handling and includes it in the bug fix test suite. Updates workflow to run the new bug fix tests. Script version updated to 2025.11.0.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
casaos-fix-docker-api-version/test-script.sh (1)
1571-1571: Minor: Unused variablefunc_exitat line 1571.The variable
func_exit=$?is assigned at line 1571 but never used afterward. It can be safely removed.- local output=$($test_script 2>&1) - local func_exit=$? - end_time=$(date +%s) + local output + output=$($test_script 2>&1) + end_time=$(date +%s)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
.github/workflows/test-casaos-fix.yml(1 hunks)casaos-fix-docker-api-version/run.sh(5 hunks)casaos-fix-docker-api-version/test-script.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test-casaos-fix.yml
🧰 Additional context used
🧬 Code graph analysis (1)
casaos-fix-docker-api-version/run.sh (1)
casaos-fix-docker-api-version/test-script.sh (2)
apply_docker_api_override(193-260)remove_docker_api_override(263-293)
🪛 Shellcheck (0.11.0)
casaos-fix-docker-api-version/test-script.sh
[warning] 63-63: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 73-73: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 97-97: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 98-98: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 99-99: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 242-242: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 243-243: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 359-359: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 360-360: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 383-383: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 384-384: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 420-420: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 421-421: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 422-422: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 473-473: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 576-576: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 614-614: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 615-615: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 721-721: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 820-820: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 821-821: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 940-940: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 943-943: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1024-1024: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1077-1077: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1092-1092: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1111-1111: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1358-1358: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1361-1361: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1418-1418: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1487-1487: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1490-1490: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1570-1570: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1571-1571: func_exit appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-fix-script
- GitHub Check: test-fix-script
🔇 Additional comments (20)
casaos-fix-docker-api-version/run.sh (5)
22-25: Approve: NON_INTERACTIVE mode detection is clear and early.The early check and echo banner improve visibility in CI logs when automated testing is running.
27-59: Approve: print_info function provides consistent, colored output.The function implements a clean helper for colored terminal output with sensible defaults. Color support includes standard ANSI codes (red, green, yellow, blue, magenta, cyan).
284-286: Approve: timeout wrapping of casaos command prevents hanging.Line 286 wraps the
casaos -vcommand with a 5-second timeout to gracefully handle unresponsive CasaOS processes. This aligns with the fix objectives and is consistent with timeout patterns used elsewhere in the script (e.g., line 335, 368).
1391-1396: Approve: NON_INTERACTIVE mode auto-confirms Docker reset prompt.When
NON_INTERACTIVE="true", the prompt for Docker reset is automatically approved. This enables unattended CI execution while preserving the interactive prompt for manual runs.
1622-1627: Approve: NON_INTERACTIVE mode auto-confirms network connectivity check.Consistent with the Docker reset prompt above, network connectivity warnings are auto-approved in non-interactive mode, enabling full automation in CI environments.
casaos-fix-docker-api-version/test-script.sh (15)
28-58: Approve: Output helper functions are well-structured.The print_* functions provide consistent colored output with clear semantics (info/success/warning/error).
print_headernicely frames section titles. Color definitions at lines 29–33 are clean and standard.
367-367: Approve: API version comparison uses awk for proper decimal handling.Correctly uses awk to compare API versions as decimals (
awk -v ver="$new_api" 'BEGIN {exit !(ver >= 1.45)}'), avoiding string comparison pitfalls. This is consistent with similar checks elsewhere in the codebase (e.g., line 447).
402-412: Approve: Exit code captured before print_error to avoid masking.Line 407 captures the exit code into a local variable before calling
print_error, ensuring the original failure status is preserved and passed to the caller (line 411). This fixes the issue noted in past reviews.
504-514: Approve: NON_INTERACTIVE mode auto-approves full test prompt.When
NON_INTERACTIVE="true", the user confirmation prompt is automatically approved, allowing the full test workflow (upgrade + fix) to run without interaction. This aligns with the PR objectives for CI automation.
1331-1459: Approve: CasaOS version check timeout handling is thorough.The
test_casaos_hangfunction comprehensively verifies that the 5-second timeout oncasaos -v(mirroring the change in run.sh at line 286) prevents hanging. The test validates timeout exit code (124), duration, and graceful fallback to "version unknown".
1461-1593: Approve: Docker daemon timeout testing is well-implemented.The
test_unresponsive_daemonfunction validates that the 10-second timeout ondocker version(used in run.sh lines 335, 350, etc.) prevents hangs. Exit code and duration checks confirm timeout behavior works as intended.
1717-1783: Approve: Main function routing is comprehensive and well-organized.The case statement handles all test scenarios (status, upgrade, test, full, api-., test-all, test-gpg, test-download, test-snap, test-netns, test-casaos, test-bugfixes, apply-override, remove-override, help) with appropriate aliases for ease of use. Unknown commands trigger the help message.
653-706: Approve: API-specific test functions follow consistent patterns.Functions
test_api_1_45,test_api_1_46, andtest_api_1_47each install a specific Docker version and invoketest_fix_script. The interactive prompt between installation and test verification (lines 664–666, 682–684, 700–702) respectsNON_INTERACTIVEmode.
61-82: Approve: Version detection functions include robust error handling.Functions
get_docker_version,get_docker_api_version, andget_dockerd_binary_versionuse timeouts (lines 63, 73, 87) and fallback logic (lines 74–77) to gracefully handle unresponsive daemons or missing commands. Returning "not_installed" or empty strings prevents script breakage.
379-493: Approve: test_fix_script comprehensive verification is thorough.The function verifies multiple aspects: Docker version (28.x check), API version (1.47–1.51 range for Docker 28.x before breaking change at 1.52), dockerd binary version, service status, and package holds. Clear success/failure reporting guides the user on what succeeded or failed.
1595-1715: Approve: test_all_bugfixes orchestrates six distinct bug fix validations.The function runs all bug fix tests (GPG conflicts, GPG download resilience, snap hang, netns cleanup, unresponsive daemon, CasaOS hang) with clear section headers, exit code tracking, and a summary. Respects
NON_INTERACTIVEmode for automated CI runs.
709-857: Approve: test_gpg_key_conflicts realistically simulates and validates the fix.The test creates conflicting GPG key locations, verifies the conflict error occurs, runs the fix script, and confirms resolution (docker.asc in correct location, docker.gpg removed, apt update succeeds, Docker 28.x installed). Comprehensive coverage of the reported Debian/OMV issue.
860-912: Approve: test_gpg_download_resilience validates download robustness.Removes the GPG key to force a fresh download, runs the fix script, and verifies the key was re-downloaded and is a valid PGP public key. Tests the retry and HTTP/1.1 fallback logic indirectly through successful script completion.
914-1056: Approve: test_snap_hang validates timeout handling with mocked snap command.Creates a mock snap command that sleeps 10 seconds, verifies
timeout 3correctly triggers exit code 124, and validates the function incheck_and_remove_snap_docker(extracted from run.sh) correctly detects and handles the timeout. Thorough coverage of the snap daemon unresponsiveness issue.
1058-1172: Approve: test_netns_cleanup validates network namespace cleanup logic.Creates test containers to generate network namespaces, stops Docker, runs cleanup logic (find with -delete), and verifies Docker restarts successfully and can run containers. Tests error suppression for "device busy" errors during netns cleanup.
Refactored the assignment of the 'output' variable to avoid combining declaration and command substitution, improving compatibility and clarity in the script.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
casaos-fix-docker-api-version/test-script.sh (1)
1-1783: Large test harness is well-organized; minor code quality improvements available.The test-script.sh is comprehensive and methodically structured. Previous critical issues (string-based version comparison at line 367, exit code masking) have been correctly fixed using awk-based decimal comparison and immediate
$?capture. The script properly integrates NON_INTERACTIVE mode throughout and uses timeout commands defensively to prevent hangs.Shellcheck reports 24 SC2155 warnings (declaring and assigning in one statement). Most are low-risk when capturing simple command output. However, following best practice of separating declaration from assignment would improve robustness against future refactoring. Consider this a long-term cleanup opportunity rather than a blocking issue.
Consider refactoring the three near-identical test API functions (
test_api_1_45,test_api_1_46,test_api_1_47at lines 654–706) into a single parameterized function to reduce duplication and maintenance burden:test_api_version() { local version=$1 local description=$2 print_header "Testing with API $description" if ! install_docker_version "$version" "$description"; then return 1 fi echo "" print_info "Docker $version installed. Testing fix script..." if [ "$NON_INTERACTIVE" != "true" ]; then read -p "Press Enter to run the fix script..." -r echo "" fi test_fix_script } # Usage: test_api_version "27." "API 1.45 (Docker 27.x)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
casaos-fix-docker-api-version/test-script.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
casaos-fix-docker-api-version/test-script.sh (1)
casaos-fix-docker-api-version/run.sh (5)
get_docker_api_version(327-356)print_info(28-59)detect_os(111-122)apply_docker_api_override(553-630)remove_docker_api_override(633-671)
🪛 Shellcheck (0.11.0)
casaos-fix-docker-api-version/test-script.sh
[warning] 63-63: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 73-73: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 97-97: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 98-98: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 99-99: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 242-242: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 243-243: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 359-359: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 360-360: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 383-383: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 384-384: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 420-420: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 421-421: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 422-422: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 473-473: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 576-576: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 614-614: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 615-615: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 721-721: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 820-820: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 821-821: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 940-940: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 943-943: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1024-1024: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1077-1077: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1092-1092: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1111-1111: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1358-1358: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1361-1361: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1487-1487: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1490-1490: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1570-1570: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1571-1571: func_exit appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-fix-script
- GitHub Check: test-fix-script
🔇 Additional comments (1)
casaos-fix-docker-api-version/test-script.sh (1)
402-412: I'll help you verify the exit code capture behavior in the bash script. Let me start by examining the actual file and testing the bash behavior described in the review comment.
<function_calls>
#!/bin/bashFirst, find and examine the test-script.sh file
find . -name "test-script.sh" -type f 2>/dev/null | head -5
</function_calls><function_calls>
#!/bin/bashRun the test case provided in the review comment to verify exit code behavior
test_exit_capture() {
Simulate success
if true; then
echo "Success branch"
else
local code=$?
echo "Failed with code: $code"
fiSimulate failure
if false; then
echo "Success branch"
else
local code=$?
echo "Failed with code: $code"
fi
}test_exit_capture
</function_calls>Now let me examine the actual test-script.sh file around the lines in question:
<function_calls>
#!/bin/bashLocate and read the relevant section of test-script.sh
cat -n casaos-fix-docker-api-version/test-script.sh | sed -n '395,420p'
</function_calls><function_calls>
#!/bin/bashTest more precisely to understand $? behavior in else blocks
When the if condition fails, what is $? in the else block?
echo "=== Test 1: Direct check of $? in else block ==="
if false; then
echo "True branch"
else
code=$?
echo "False branch - $? is: $code"
fiecho ""
echo "=== Test 2: Simulating the actual pattern from test-script.sh ==="
if bash -c 'exit 42'; then
echo "Success"
else
code=$?
echo "Command failed with exit code: $code"
fiecho ""
echo "=== Test 3: Simulating failure case ==="
if bash -c 'exit 42'; then
echo "Success"
else
code=$?
echo "Command failed with exit code: $code"
fi
</function_calls>
Separated declaration and assignment of the 'output' variable to avoid issues with local scoping in subshells. This improves compatibility and reliability of the test script.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
casaos-fix-docker-api-version/test-script.sh (1)
1-58: Shellcheck SC2155 warnings throughout: consider separating declaration and assignment.Static analysis flagged 26 instances of SC2155 (declare and assign separately to avoid masking return values) at lines: 63, 73, 97–99, 242–243, 359–360, 383–384, 420–422, 473, 576, 614–615, 721, 820–821, 940, 943, 1077, 1092, 1111, 1358, 1361, 1487, 1490.
While these warnings are technically valid, the impact is low in this script because:
- Functions use
|| fallbackpatterns and echo default values on error- Timeouts are properly wrapped
- The exit-code masking affects informational helpers, not critical logic
However, for production robustness and to satisfy shellcheck, consider refactoring examples like:
- Line 63:
local version=$(timeout 10 $SUDO docker version --format '{{.Server.Version}}' 2>/dev/null || echo "")→ separate declaration and assignment- Line 576:
local full_version=$(apt-cache madison docker-ce 2>/dev/null | grep "$version" | head -n1 | awk '{print $3}')→ separate declaration and assignmentThis is an optional refactor that would improve code quality without changing behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
casaos-fix-docker-api-version/test-script.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
casaos-fix-docker-api-version/test-script.sh
[warning] 63-63: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 73-73: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 97-97: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 98-98: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 99-99: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 242-242: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 243-243: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 359-359: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 360-360: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 383-383: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 384-384: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 420-420: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 421-421: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 422-422: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 473-473: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 576-576: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 614-614: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 615-615: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 721-721: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 820-820: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 821-821: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 940-940: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 943-943: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1077-1077: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1092-1092: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1111-1111: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1358-1358: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1361-1361: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1487-1487: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1490-1490: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-fix-script
- GitHub Check: test-fix-script
🔇 Additional comments (19)
casaos-fix-docker-api-version/test-script.sh (19)
36-58: Print helper functions are well-implemented.The color-coded output functions provide good user experience and follow a consistent pattern. No issues identified.
61-92: Version detection helpers use defensive timeout patterns.Functions properly timeout after 10 seconds to prevent hangs on unresponsive Docker daemon, and echo fallback values on error. Good defensive programming.
Regarding Shellcheck SC2155 warnings (declare and assign separately): while technically valid, the impact here is minimal because all functions use proper
|| fallbackpatterns and echo default values on error. These are low-risk patterns in informational helper functions.
137-190: OS detection and CasaOS helpers are straightforward and correct.OS detection safely sources
/etc/os-release, and the CasaOS restart logic properly handles service lifecycle with appropriate error suppression.
192-293: Docker API override functions are well-implemented.Both
apply_docker_api_overrideandremove_docker_api_overrideproperly handle systemd service management, include verification steps, and have clear error handling.
295-376: Docker upgrade flow is correct; past decimal-comparison issue is resolved.Line 367 now properly uses
awkfor decimal comparison instead of string comparison (addressed in commit dbd48ca). Restart sequence includes safety measures (daemon-reload, stop, pkill, sleep, start). Upgrade logic is sound.
379-493: Test fix script function correctly captures exit code and validates results.Line 407 properly captures
local exit_code=$?before callingprint_error()(addressed in past review). Version checks use correctawkdecimal comparisons. Verification includes version match, API range (1.47–1.51), package hold status, and Docker service state. Test logic is comprehensive and correct.
496-535: Full test orchestration properly handles non-interactive mode and error propagation.Both upgrade and test steps return their exit codes, which are checked before proceeding. Non-interactive mode is correctly integrated (lines 504–509, 523–526). Workflow is sound.
537-652: Docker version installation is well-structured with proper fallbacks and optional CasaOS restart.Repository setup, version querying, and installation all have proper error handling. CasaOS restart respects non-interactive mode (lines 632–644). Function returns correct exit code based on version verification.
654-706: API-specific test functions follow a clean, consistent pattern.
test_api_1_45,test_api_1_46, andtest_api_1_47properly delegate to helpers and respect non-interactive mode. Good code reuse.
708-857: GPG key conflict test has robust backup/restore and comprehensive verification.Test simulates the Debian/OMV issue by creating conflicting GPG key locations, verifies the conflict is reproduced (lines 755–760), runs the fix script, and validates resolution. Backup/restore pattern is sound: if the fix fails, backups are restored before returning error (lines 777–786). Final cleanup removes backups (line 844). Verification is thorough (docker.asc exists, docker.gpg removed, apt works).
859-912: GPG download resilience test properly validates timeout and fallback behavior.Removes key to force download, runs fix script, and verifies key is downloaded and valid. Backup/restore pattern handles failure gracefully.
914-1056: Snap daemon hang test correctly validates timeout behavior and mocking.Mock
snapcommand sleeps 10 seconds to simulate hang. Direct timeout test verifies 3-second timeout triggers (exit code 124). Function extraction and re-testing ofcheck_and_remove_snap_dockeris clever; verifies function completes in ~3 seconds and detects timeout correctly. PATH manipulation includes proper restoration (line 1028).
1058-1172: Network namespace cleanup test handles device-busy scenario and validates Docker recovery.Creates containers to generate netns entries, stops Docker to trigger cleanup scenario, attempts cleanup gracefully (errors ignored), restarts Docker, cleans up containers, and verifies Docker functionality with
hello-world. Proper Docker lifecycle handling.
1331-1459: CasaOS version check hang test follows the snap hang pattern correctly.Mock
casaoscommand sleeps 15 seconds. Direct timeout test and function extraction verify timeout is detected and handled gracefully. PATH restoration is correct (line 1422).
1461-1593: Unresponsive Docker daemon test validates timeout handling in version detection.Mock
dockercommand sleeps 20 seconds. Tests both direct timeout (exit code 124) and the actualget_docker_api_versionfunction. Function properly avoids fallback if timeout occurs (line 1551–1554), preventing cascading timeouts.
1174-1270: All API version orchestrator is well-structured and respects non-interactive mode.Tests API 1.45, 1.46, 1.47 in sequence, collects results, and prints clear summary. Non-interactive prompts are properly handled throughout.
1595-1715: Bug fix test orchestrator is comprehensive and properly sequences six distinct tests.Runs GPG conflicts, download resilience, snap hang, netns cleanup, unresponsive daemon, and CasaOS hang tests. Results collection and summary reporting are clear. Non-interactive mode support is thorough.
1272-1329: Usage documentation is comprehensive and well-organized.Commands are grouped logically (status/upgrade/test, API tests, bug fix tests, override options). Aliases (e.g.,
api-1.45|api-1-45|api145) improve usability.
1717-1783: Main dispatcher is clean and handles unknown commands appropriately.Case statement covers all command variants, and unknown commands trigger an error message and show usage.
1 participant
This pull request introduces improvements to the
casaos-fix-docker-api-versionscripts and adds a new GitHub Actions workflow for automated testing. The main changes focus on enabling non-interactive mode for easier automation and enhancing script output readability.Automation and usability improvements:
.github/workflows/test-casaos-fix.ymlto automatically test the fix script on pushes and pull requests, including making scripts executable and running the test script in a CI environment.run.shto support non-interactive mode by checking theNON_INTERACTIVEenvironment variable and automatically confirming prompts when enabled, which streamlines automation and CI usage. [1] [2]User experience enhancements:
print_infofunction torun.shfor colored output, improving readability of script messages in the terminal.