Open luandro opened 2 weeks ago
โฑ๏ธ Estimated effort to review [1-5] | 3 |
๐งช Relevant tests | No |
๐ Security concerns | No |
โก Key issues to review |
Possible Bug: The script in start.sh uses iwgetid -r to check if WiFi is connected, but does not handle different exit statuses which might not be reliable across all environments or might fail silently. |
Enhancement Suggestion: Consider adding error handling or a more robust method to verify the WiFi connection status. |
Category | Suggestion | Score |
Possible issue |
Add a check to ensure the SSID is set before starting WiFi Connect___ **Add a check for the SSID environment variable before attempting to start the WiFi Connect.This ensures that the SSID is set and prevents the script from failing if it's not provided.** [services/wifi-connect/start.sh [45-49]](https://github.com/digidem/edt-offline/pull/69/files#diff-0061d0502b48b8aab8c0cbeadc6070b38327f86672437cebe676aaca376af781R45-R49) ```diff if [ $? -eq 0 ]; then printf 'Skipping WiFi Connect\n' +elif [ -z "$SSID" ]; then + printf 'Error: SSID is not set.\n' + exit 1 else printf 'Starting Earth Defenders Toolkit Hotspot and Captive-Portal\n' ./wifi-connect --portal-ssid "$SSID" fi ``` Suggestion importance[1-10]: 9Why: This suggestion adds a crucial check for the SSID environment variable, which prevents potential runtime errors if the SSID is not set. This is a significant improvement in terms of robustness and reliability. | 9 |
Best practice |
Pin the version of the Debian base image to ensure consistent builds___ **Pin the version of the base image to ensure consistent, reproducible builds. Using aspecific version helps avoid unexpected changes due to updates in the base image.** [services/wifi-connect/Dockerfile.template [3]](https://github.com/digidem/edt-offline/pull/69/files#diff-f11609dd7c1b1ed41438aaba1a00e2ce0fc35e9a1d8dc481004ed3c06b198bf9R3-R3) ```diff -FROM balenalib/$BALENA_ARCH-debian +FROM balenalib/$BALENA_ARCH-debian:buster-20210125 ``` Suggestion importance[1-10]: 8Why: Pinning the base image version is a best practice that ensures consistent and reproducible builds, reducing the risk of unexpected changes due to updates in the base image. | 8 |
Add a version specification to the Docker Compose file___ **Specify a version for the Docker Compose file to ensure compatibility and predictablebehavior across different versions of Docker Compose.** [docker-compose.yml [39]](https://github.com/digidem/edt-offline/pull/69/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R39-R39) ```diff +version: '3.8' services: io.balena.features.supervisor-api: 1 io.balena.features.balena-api: 1 ``` Suggestion importance[1-10]: 7Why: Adding a version specification to the Docker Compose file is a good practice that ensures compatibility and predictable behavior, although it is not as critical as some other changes. | 7 | |
Use direct test command for checking
___
**Replace the use of | 6 |
User description
Python wifi isn't maintained anymore and is having problems. Use latest wifi-connect instead of python-wifi.
PR Type
Enhancement, Configuration changes
Description
start.sh
to skip WiFi Connect if already connected.wifi-connect
with the new arguments.balena.yml
from 0.5.0 to 0.5.4.python-wifi-connect
towifi-connect
indocker-compose.yml
and updated the service configuration.python-wifi-connect
.wifi-connect
with architecture-specific handling and latest version fetching.Changes walkthrough ๐
start.sh
Add connection check and update WiFi Connect usage
services/wifi-connect/start.sh
wifi-connect
with the new arguments.docker-compose.yml
Switch to wifi-connect and update service configuration
docker-compose.yml
python-wifi-connect
towifi-connect
.wifi-connect
.Dockerfile.template
Add Dockerfile template for wifi-connect
services/wifi-connect/Dockerfile.template
wifi-connect
.balena.yml
Update application version to 0.5.4
balena.yml - Updated the version from 0.5.0 to 0.5.4.
Dockerfile
Remove old Dockerfile for python-wifi-connect
services/wifi-connect/Dockerfile - Removed the old Dockerfile for `python-wifi-connect`.