-
Notifications
You must be signed in to change notification settings - Fork 34
Merge V2.14+fall2025 into main #791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… the start-reload script. (#465) This solution exchanges the start-reload script with a direct call of the main.py file from the docker compose file
* fix command to execute sdk container (cherry picked from commit 8df12b8) * Reset pdf file to spring2025
Update nrutil link Update dongle instructions
Update the Ubuntu version reference to 24.04.1 as this is now the only available option in the Raspberry Pi Imager for Ubuntu Server 24.04 LTS.
* UG- Added Reuse commissioning information section * Update docs/Matter_TH_User_Guide/Matter_TH_User_Guide.adoc * Update docs/Matter_TH_User_Guide/Matter_TH_User_Guide.adoc * updated UG PDF
fix typo in SD card description
* Updating docker's network connectivity on installation script * Fixing identation of file
* add log stuff * fix auto install * fix auto install * ignore instalation logs * fix auto install
…759) * Install additional dependencies during update and fresh installation * Updated additional-dependency-list.txt file * Update scripts/ubuntu/1.2-install-additional-dependencies.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Added missing UBUNTU_SCRIPT_DIR to internal-auto-update.sh script file * Debug error * Removed echo debug * Updated script to run the updated script in case the file is updated after Update Test Harness code step * Reverting some changes * Update auto-update logic --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* Changes for v2.14-beta3+fall205 version * Updated backend SHA in docker-compose.yml * Update docs/Matter_TH_User_Guide/Matter_TH_User_Guide.adoc Co-authored-by: antonio-amjr <[email protected]> * Code review * Code review * Updated backend --------- Co-authored-by: antonio-amjr <[email protected]>
* Pin docker-ce to version <28.0.0 to prevent Traefik incompatibility * Updated docker-ce version * Fix Docker installation to exclude incompatible version 29.x * Update scripts/ubuntu/1-install-dependencies.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update scripts/ubuntu/1-install-dependencies.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Romulo Quidute Filho seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Summary of ChangesHello @rquidute, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request merges the V2.14+fall2025 release branch into main, introducing significant updates. Key changes include the addition of documentation for Wi-Fi PAF Commissioning and Matter Test Harness Cameras, updates to various dependencies and configurations in docker-compose.yml, and extensive refactoring of installation and update scripts to improve logging and robustness.
My review focuses on the correctness and maintainability of these changes. I've identified a critical issue in a refactored update script that will cause it to fail, a high-severity security concern in the docker-compose.yml configuration, and several medium-severity issues in the documentation and a new utility script. These points are detailed in the review comments with suggestions for improvement.
| print_script_step "Update Test Harness code" | ||
| $SCRIPT_DIR/update-th-code.sh "$BRANCH_NAME" | ||
| verify_return_code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script will fail because it calls print_script_step and verify_return_code without sourcing utils.sh where these functions are defined.
The refactoring to introduce logging seems incomplete. To fix this and make it consistent with other scripts like pi-setup/auto-install.sh, all the operational logic should be moved into internal-auto-update.sh. The auto-update.sh script should only be a wrapper that handles argument checking and calls the internal script while piping its output to a log file.
Specifically, the update-th-code.sh call should be moved into internal-auto-update.sh.
| # insecureSkipVerify is enabled for push_av_server as it uses a self-signed certificate. | ||
| # This ensures Traefik can proxy requests HTTP -> HTTPS without failing certificate validation. | ||
| # service specific transport is not supported in v2.2 and was introduced in v2.4 hence enabling it here. | ||
| - --serversTransport.insecureSkipVerify=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using serversTransport.insecureSkipVerify=true as a global command-line flag disables TLS certificate validation for all backend services, which poses a security risk. While the comment explains this is for push_av_server with a self-signed certificate, it's better to apply this setting only to the specific service that needs it. The comment correctly notes that service-specific transport configurations were introduced in Traefik v2.4. Consider upgrading Traefik from v2.2 to v2.4 or newer. This would allow you to configure insecureSkipVerify on a per-service basis within your dynamic configuration file (traefik_dynamic.yml), improving the overall security of your deployment.
| ** TH does not need any special setting, just need to connect to AP. It's ok even using ethernet. But Wi-Fi should be on that I always test by using Wi-Fi connecting to AP. Test wpa_supplicant configure file script is below: | ||
| *** ctrl_interface=DIR=/run/wpa_supplicant | ||
| *** update_config=1 | ||
| *** network={ | ||
| + | ||
| ssid="n_m_2g" | ||
| + | ||
| key_mgmt=WPA-PSK | ||
| + | ||
| psk="nxp12345" | ||
| + | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This configuration example for wpa_supplicant is not formatted as a code block, and the *** and + characters make it difficult to read and copy. For better clarity and usability, it should be formatted using an AsciiDoc source block.
** TH does not need any special setting, just need to connect to AP. It's ok even using ethernet. But Wi-Fi should be on that I always test by using Wi-Fi connecting to AP. Test wpa_supplicant configure file script is below:
+
[source,ini]
----
ctrl_interface=DIR=/run/wpa_supplicant
update_config=1
network={
ssid="n_m_2g"
key_mgmt=WPA-PSK
psk="nxp12345"
}
----
| ```shell | ||
| sudo apt-get install \ | ||
| libgstreamer1.0-dev \ | ||
| libgstreamer-plugins-base1.0-dev \ | ||
| libgstreamer-plugins-bad1.0-dev \ | ||
| gstreamer1.0-plugins-base \ | ||
| gstreamer1.0-plugins-good \ | ||
| gstreamer1.0-plugins-bad \ | ||
| gstreamer1.0-plugins-ugly \ | ||
| gstreamer1.0-libav \ | ||
| gstreamer1.0-tools \ | ||
| gstreamer1.0-x \ | ||
| gstreamer1.0-alsa \ | ||
| gstreamer1.0-gl \ | ||
| gstreamer1.0-gtk3 \ | ||
| gstreamer1.0-qt5 \ | ||
| gstreamer1.0-pulseaudio | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code block for installing GStreamer is using Markdown syntax (```shell). Since this is an AsciiDoc file, it should be updated to use the correct source block syntax for proper rendering.
[source,shell]
----
sudo apt-get install \
libgstreamer1.0-dev \
libgstreamer-plugins-base1.0-dev \
libgstreamer-plugins-bad1.0-dev \
gstreamer1.0-plugins-base \
gstreamer1.0-plugins-good \
gstreamer1.0-plugins-bad \
gstreamer1.0-plugins-ugly \
gstreamer1.0-libav \
gstreamer1.0-tools \
gstreamer1.0-x \
gstreamer1.0-alsa \
gstreamer1.0-gl \
gstreamer1.0-gtk3 \
gstreamer1.0-qt5 \
gstreamer1.0-pulseaudio
----
| exec > >(tee -a "$SIDELOAD_LOGFILE_PATH") 2>&1 | ||
|
|
||
| # backend container | ||
| CONTAINER_NAME="certification-tool-backend-1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding the container name certification-tool-backend-1 is brittle. The name is generated by docker compose and depends on the project's directory name. It's more robust to look up the container ID using its service name from docker-compose.yml, which is backend.
Consider retrieving the container ID at the beginning of the script and using it for all subsequent docker commands.
Example:
# At the top of the script
BACKEND_SERVICE_NAME="backend"
CONTAINER_ID=$(docker compose ps -q "$BACKEND_SERVICE_NAME")
if [ -z "$CONTAINER_ID" ]; then
echo "Container for service '$BACKEND_SERVICE_NAME' not found."
exit 1
fi
# Then use $CONTAINER_ID for all docker commands, e.g.:
docker exec -i "$CONTAINER_ID" ...
docker restart "$CONTAINER_ID"|
|
1 similar comment
|
|
21bb5c3 to
73ed1a2
Compare
* Updating user guide for Matter 1.5 finale version * Updating the PDF file with the changes
Merge V2.14+fall2025 into main