Bump 5.0.0#374
Conversation
Signed-off-by: Sungho Woo <wsh@robotis.com>
Signed-off-by: Sungho Woo <wsh@robotis.com>
- Implemented functions to fetch the current version from the meta package and the latest version from GitHub releases. - Added notifications for available updates when starting or entering the container. - Included a notice regarding the default RMW implementation since version 5.0.0. Signed-off-by: Sungho Woo <wsh@robotis.com>
Set rmw_zenoh as the Default ROS 2 Middleware
There was a problem hiding this comment.
Code Review
This pull request updates the Open Manipulator setup to version 5.0.0, switching the default RMW implementation to rmw_zenoh_cpp and adding update notification logic to the container script. Feedback on these changes suggests using a heredoc for .bashrc setup in the Dockerfile to improve readability. For the container script, recommendations include extracting duplicated update-checking logic into a function, using jq instead of sed for robust JSON parsing, defining the display box width as a constant, renaming variables for consistency, and referencing a tracking ticket for temporary notices.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| RUN echo "source /opt/ros/${ROS_DISTRO}/setup.bash" >> ~/.bashrc && \ | ||
| echo "source ${COLCON_WS}/install/setup.bash" >> ~/.bashrc && \ | ||
| echo "alias cb='colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=Release'" >> ~/.bashrc && \ | ||
| echo "export ROS_DOMAIN_ID=30" >> ~/.bashrc | ||
| echo "alias omy_ai='ros2 launch open_manipulator_bringup omy_ai.launch.py'" >> ~/.bashrc && \ | ||
| echo "alias omx_ai='ros2 launch open_manipulator_bringup omx_ai.launch.py'" >> ~/.bashrc && \ | ||
| echo "alias zenohd='ros2 run rmw_zenoh_cpp rmw_zenohd'" >> ~/.bashrc && \ | ||
| echo "export ROS_DOMAIN_ID=30" >> ~/.bashrc && \ | ||
| echo "export RMW_IMPLEMENTATION=rmw_zenoh_cpp" >> ~/.bashrc |
There was a problem hiding this comment.
To improve readability and maintainability, consider using a cat with a heredoc to append these lines to .bashrc instead of chaining multiple echo commands.
RUN cat <<EOF >> ~/.bashrc
source /opt/ros/${ROS_DISTRO}/setup.bash
source ${COLCON_WS}/install/setup.bash
alias cb='colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=Release'
alias omy_ai='ros2 launch open_manipulator_bringup omy_ai.launch.py'
alias omx_ai='ros2 launch open_manipulator_bringup omx_ai.launch.py'
alias zenohd='ros2 run rmw_zenoh_cpp rmw_zenohd'
export ROS_DOMAIN_ID=30
export RMW_IMPLEMENTATION=rmw_zenoh_cpp
EOF
| print_update_notice "${CURRENT_VER}" "${LATEST_VER}" | ||
| fi | ||
|
|
||
| ### rmw_zenoh notice (remove later) |
There was a problem hiding this comment.
| CURRENT_VER=$(get_current_version) | ||
| GIT_VER=$(get_latest_version) | ||
| if update_available "${CURRENT_VER}" "${GIT_VER}"; then | ||
| print_update_notice "${CURRENT_VER}" "${GIT_VER}" | ||
| fi |
There was a problem hiding this comment.
This logic is duplicated from the start_container function (lines 37-41). To improve maintainability, you should extract this into a new function and call it from both places.
Additionally, the variable GIT_VER should be renamed to LATEST_VER for consistency with start_container.
| CURRENT_VER=$(get_current_version) | |
| GIT_VER=$(get_latest_version) | |
| if update_available "${CURRENT_VER}" "${GIT_VER}"; then | |
| print_update_notice "${CURRENT_VER}" "${GIT_VER}" | |
| fi | |
| CURRENT_VER=$(get_current_version) | |
| LATEST_VER=$(get_latest_version) | |
| if update_available "${CURRENT_VER}" "${LATEST_VER}"; then | |
| print_update_notice "${CURRENT_VER}" "${LATEST_VER}" | |
| fi |
| print_update_notice() { | ||
| local current_ver="$1" | ||
| local latest_ver="$2" | ||
| W=52 |
| get_latest_version() { | ||
| local json tag | ||
| json=$(curl -sL --connect-timeout 5 "${GITHUB_RELEASES_API}" 2>/dev/null) | ||
| tag=$(echo "${json}" | sed -n 's/.*"tag_name":\s*"\([^"]*\)".*/\1/p' | head -1) |
There was a problem hiding this comment.
Parsing JSON with sed is fragile and can break if the API response format changes, even slightly (e.g., with extra whitespace). Using a dedicated JSON parser like jq would be much more robust and readable.
For example:
tag=$(echo "${json}" | jq -r '.tag_name')You may want to add a check to ensure jq is installed on the host system, or add it to the developer environment setup instructions.
| local current="$1" | ||
| local git_ver="$2" | ||
| if [ -z "${git_ver}" ]; then | ||
| return 1 | ||
| fi | ||
| if [ "${git_ver}" = "${current}" ]; then | ||
| return 1 | ||
| fi | ||
| local newer | ||
| newer=$(echo -e "${current}\n${git_ver}" | sort -V | tail -1) | ||
| [ "${newer}" = "${git_ver}" ] |
There was a problem hiding this comment.
For clarity and consistency, consider renaming the local variable git_ver to latest_ver. This makes the function's purpose more generic, as it compares a 'current' version with a 'latest' version, regardless of the source.
| local current="$1" | |
| local git_ver="$2" | |
| if [ -z "${git_ver}" ]; then | |
| return 1 | |
| fi | |
| if [ "${git_ver}" = "${current}" ]; then | |
| return 1 | |
| fi | |
| local newer | |
| newer=$(echo -e "${current}\n${git_ver}" | sort -V | tail -1) | |
| [ "${newer}" = "${git_ver}" ] | |
| local current="$1" | |
| local latest_ver="$2" | |
| if [ -z "${latest_ver}" ]; then | |
| return 1 | |
| fi | |
| if [ "${latest_ver}" = "${current}" ]; then | |
| return 1 | |
| fi | |
| local newer | |
| newer=$(echo -e "${current}\n${latest_ver}" | sort -V | tail -1) | |
| [ "${newer}" = "${latest_ver}" ] |
Update zenoh as default rmw