Skip to content

Wifi-Connect#69

Merged
luandro merged 9 commits intomainfrom
wifi-connect
Mar 13, 2025
Merged

Wifi-Connect#69
luandro merged 9 commits intomainfrom
wifi-connect

Conversation

@luandro
Copy link
Contributor

@luandro luandro commented Jun 17, 2024

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

  • Added a check in start.sh to skip WiFi Connect if already connected.
  • Updated the script to use wifi-connect with the new arguments.
  • Updated the application version in balena.yml from 0.5.0 to 0.5.4.
  • Switched from python-wifi-connect to wifi-connect in docker-compose.yml and updated the service configuration.
  • Removed the old Dockerfile for python-wifi-connect.
  • Added a new Dockerfile template for wifi-connect with architecture-specific handling and latest version fetching.

Changes walkthrough 📝

Relevant files
Enhancement
start.sh
Add connection check and update WiFi Connect usage             

services/wifi-connect/start.sh

  • Added a check to skip WiFi Connect if already connected.
  • Updated the script to use wifi-connect with the new arguments.
  • +8/-2     
    docker-compose.yml
    Switch to wifi-connect and update service configuration   

    docker-compose.yml

  • Switched from python-wifi-connect to wifi-connect.
  • Updated service configuration for wifi-connect.
  • +8/-10   
    Dockerfile.template
    Add Dockerfile template for wifi-connect                                 

    services/wifi-connect/Dockerfile.template

  • Added a new Dockerfile template for wifi-connect.
  • Included architecture-specific handling and latest version fetching.
  • +36/-0   
    Configuration changes
    balena.yml
    Update application version to 0.5.4                                           

    balena.yml

    • Updated the version from 0.5.0 to 0.5.4.
    +1/-1     
    Dockerfile
    Remove old Dockerfile for python-wifi-connect                       

    services/wifi-connect/Dockerfile

    • Removed the old Dockerfile for python-wifi-connect.
    +0/-15   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @github-actions
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ 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.

    @github-actions
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    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]

     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]: 9

    Why: 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 a
    specific version helps avoid unexpected changes due to updates in the base image.

    services/wifi-connect/Dockerfile.template [3]

    -FROM balenalib/$BALENA_ARCH-debian
    +FROM balenalib/$BALENA_ARCH-debian:buster-20210125
     
    Suggestion importance[1-10]: 8

    Why: 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 predictable
    behavior across different versions of Docker Compose.

    docker-compose.yml [39]

    +version: '3.8'
     services:
         io.balena.features.supervisor-api: 1
         io.balena.features.balena-api: 1
     
    Suggestion importance[1-10]: 7

    Why: 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 iwgetid -r output

    Replace the use of $? with a direct test command to check the output of iwgetid -r for a
    more reliable and readable condition check.

    services/wifi-connect/start.sh [43-45]

    -iwgetid -r
    -if [ $? -eq 0 ]; then
    +if iwgetid -r; then
         printf 'Skipping WiFi Connect\n'
     
    Suggestion importance[1-10]: 6

    Why: Using a direct test command for checking iwgetid -r output improves readability and reliability, but it is a minor improvement compared to other suggestions.

    6

    @luandro luandro merged commit d94ae8f into main Mar 13, 2025
    4 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    1 participant