Ros2 add docker #469
Ros2 add docker #469
Conversation
WalkthroughThis pull request introduces significant changes to the project's Docker and GitHub Actions configurations. Modifications include enhancements to the Docker build process through new Dockerfiles for hardware and simulation environments, updates to multiple workflow files with renamed jobs and refined triggers, and the addition of utility scripts for environment setup. The changes aim to streamline the continuous integration and deployment strategies of the project. Changes
Sequence DiagramsequenceDiagram
participant Workflow as GitHub Actions
participant Docker as Docker Build
participant Config as Configuration Scripts
Workflow->>Docker: Trigger Docker build
Docker->>Config: Run setup_environment.sh
Config->>Config: Check and update config directory
Config-->>Docker: Configuration ready
Docker->>Docker: Build image
Docker-->>Workflow: Image build complete
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
* Update workflow * Clean up * Update workflow version * update token * Update tags in compose files * Fixes * Fix * fix * Fix * Change action name * Update docker/Dockerfile.hardware Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com> * Add envs to compose * Add needs * Update docker/demo/compose.simulation.yaml Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com> --------- Co-authored-by: action-bot <action-bot@action-bot.com> Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com>
|
Merged without aprove because it's a secondary branch to see the differences between panther-docker after the port. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
.github/workflows/unit-tests.yaml (1)
Line range hint
4-9: Enable pull request trigger for automated testingThe pull request trigger is currently disabled with a TODO comment. This means tests aren't automatically run on PRs, which could lead to quality issues.
Remove the TODO comment and uncomment the pull request trigger:
on: workflow_dispatch: - # TODO: ENABLE WHEN READY - # pull_request: - # branches: - # - ros2-devel + pull_request: + branches: + - ros2-devel.github/workflows/release-repository.yaml (1)
Line range hint
52-122: Add error handling for critical GitHub CLI operations.The workflow performs critical operations (merging PRs, creating releases) without proper error handling. Consider adding error checking and fallback mechanisms.
Example improvement:
- gh pr merge ${{ github.event.inputs.release_candidate }} \ - --merge --delete-branch + if ! gh pr merge ${{ github.event.inputs.release_candidate }} \ + --merge --delete-branch; then + echo "::error::Failed to merge PR" + exit 1 + fi
🧹 Nitpick comments (19)
docker/demo/.env.cpu (1)
2-3: Document performance implications of software rendering.Consider adding a comment explaining that
LIBGL_ALWAYS_SOFTWARE=1forces software rendering, which might impact performance but ensures compatibility in CPU-only environments.docker/utils/setup_environment.sh (1)
1-11: Consider adding security measures for config management.The script should:
- Validate file permissions after copying
- Consider using checksums for copied files
- Add logging for audit trail
Would you like me to provide an example implementation with these security measures?
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 8-8: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
docker/demo/compose.rviz.yaml (1)
19-29: Consider documenting X11 prerequisites and version update strategy.The configuration is well-structured, but consider:
- Document X11 setup requirements for users
- Define a strategy for updating the pinned image version
Consider adding a comment in the compose file or documentation about:
- Required X11 setup on the host
- How/when to update the image version
docker/demo/compose.simulation.yaml (1)
1-27: Consider consolidating common configurations.The configuration blocks are duplicated across compose files. Consider:
- Moving common configurations to a shared compose file
- Using Docker Compose's
extendsfeature or including shared YAML filesExample approach:
# docker/demo/compose.common.yaml x-common-config: &common-config network_mode: host ipc: host environment: - RMW_IMPLEMENTATION=rmw_cyclonedds_cpp # ... other common env varsThen in service-specific files:
include: - compose.common.yaml.github/workflows/protect-default-branch.yaml (1)
Line range hint
2-27: Branch protection logic looks good, consider enhancing error reporting.The branch naming validation is well-implemented with clear patterns. Consider:
- Adding more detailed error messages that include examples of valid branch names
- Logging the actual branch name in the error message
Example enhancement:
- echo "PRs must come from branches containing 'hotfix' phrase or matching X.X.X-YYYYMMDD pattern." + echo "Invalid branch name: ${{ github.head_ref }}" + echo "Branch names must either:" + echo "- Contain 'hotfix' (e.g., feature-hotfix-123)" + echo "- Match pattern X.X.X-YYYYMMDD (e.g., 1.2.3-20241225)"docker/Dockerfile.simulation (3)
19-22: Consider using vcs for specific controller versions.Instead of manually copying specific controllers, consider using a more maintainable approach by specifying exact versions in the
.reposfile. This would make updates easier to track and manage.
30-31: Make version extraction more robust.The current version extraction using grep and sed could fail with malformed XML. Consider using xmllint or similar XML-aware tools.
-echo $(cat /ros2_ws/src/husarion_ugv/husarion_ugv/package.xml | grep '<version>' | sed -r 's/.*<version>([0-9]+.[0-9]+.[0-9]+)<\/version>/\1/g') >> /version.txt +echo $(xmllint --xpath "string(//version)" /ros2_ws/src/husarion_ugv/husarion_ugv/package.xml) >> /version.txt
1-38: Add HEALTHCHECK instruction.Consider adding a Docker HEALTHCHECK to ensure the ROS2 environment is functioning correctly.
+HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \ + CMD source /opt/ros/$ROS_DISTRO/setup.bash && ros2 node list || exit 1Also, consider creating a non-root user for better security:
+RUN groupadd -g 1001 ros && \ + useradd -u 1000 -g ros -s /bin/bash -m ros && \ + echo "ros ALL=(ALL) NOPASSWD: ALL" > /etc/sudoers.d/ros +USER rosdocker/utils/update_config_directory.sh (2)
38-39: Avoid hardcoded user/group IDs.The script uses hardcoded user (1000) and group (1001) IDs. Consider making these configurable through environment variables.
-chown -R 1000:1001 /config +: "${CONFIG_USER_ID:=1000}" +: "${CONFIG_GROUP_ID:=1001}" +chown -R "$CONFIG_USER_ID:$CONFIG_GROUP_ID" /config
1-39: Add verbose mode and proper error handling.Consider adding a verbose mode for debugging and proper error handling throughout the script.
#!/bin/bash set -e +set -o pipefail + +# Enable debug mode with DEBUG=1 +[[ ${DEBUG:-0} == 1 ]] && set -x + +# Log error messages +error() { + echo "ERROR: $*" >&2 +} + +# Log info messages when verbose +info() { + [[ ${VERBOSE:-0} == 1 ]] && echo "INFO: $*" +}🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 16-16: Declare and assign separately to avoid masking return values.
(SC2155)
docker/Dockerfile.hardware (2)
1-7: Consider adding platform-specific build argsThe Dockerfile looks well-structured, but consider adding
TARGETPLATFORMandBUILDPLATFORMbuild args for multi-architecture support, especially since this is for hardware deployment.ARG ROS_DISTRO=humble +ARG TARGETPLATFORM +ARG BUILDPLATFORM FROM husarnet/ros:${ROS_DISTRO}-ros-core
14-37: Optimize the RUN layer to reduce image sizeThe build process is correct but could be optimized to reduce the final image size:
- Consider combining
apt-get updatewith installation- Use
--no-install-recommendsfor apt-get- Move version extraction before cleanup to avoid potential issues
-RUN apt-get update && \ +RUN apt-get update && \ apt-get install -y \ + --no-install-recommends \ ros-dev-tools && \ # Setup workspace vcs import src < src/husarion_ugv/husarion_ugv/${HUSARION_ROS_BUILD_TYPE}_deps.repos && \ cp -r src/ros2_controllers/diff_drive_controller src && \ cp -r src/ros2_controllers/imu_sensor_broadcaster src && \ rm -rf src/ros2_controllers && \ # Install dependencies rosdep init && \ rosdep update --rosdistro $ROS_DISTRO && \ rosdep install --from-paths src -y -i && \ # Build source /opt/ros/$ROS_DISTRO/setup.bash && \ colcon build --symlink-install --packages-up-to husarion_ugv --cmake-args -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=$BUILD_TEST && \ + # Get version + echo $(cat /ros2_ws/src/husarion_ugv/husarion_ugv/package.xml | grep '<version>' | sed -r 's/.*<version>([0-9]+.[0-9]+.[0-9]+)<\/version>/\1/g') >> /version.txt && \ # Size optimization export SUDO_FORCE_REMOVE=yes && \ apt-get remove -y \ ros-dev-tools && \ apt-get autoremove -y && \ apt-get clean && \ - rm -rf /var/lib/apt/lists/* - # Get version - echo $(cat /ros2_ws/src/husarion_ugv/husarion_ugv/package.xml | grep '<version>' | sed -r 's/.*<version>([0-9]+.[0-9]+.[0-9]+)<\/version>/\1/g') >> /version.txt + rm -rf /var/lib/apt/lists/*docker/demo/compose.hardware.yaml (1)
1-7: Consider adding health checks to common configurationThe common configuration looks good but could benefit from health checks to ensure service stability.
x-common-config: &common-config network_mode: host ipc: host restart: always + healthcheck: + test: ["CMD-SHELL", "ros2 node list || exit 1"] + interval: 30s + timeout: 10s + retries: 3 env_file: - /home/husarion/config/common/.envdocker/README.md (2)
36-41: Improve configuration backup instructionsThe note about configuration backup could be more detailed and include actual commands.
> [!NOTE] - > This may overwrite your changes made in the `config` directory. If you want to keep your configuration you should skip this step or create backup of the `config` directory. + > This will overwrite your changes in the `config` directory. To preserve your configurations: + > 1. Create a backup: `cp -r /home/husarion/config /home/husarion/config.backup` + > 2. After updating, review and merge your changes from the backup ```bash docker run --rm -v /home/husarion/config:/config husarion/panther:humble-<newest_tag> update_config_directory<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [uncategorized] ~37-~37: Possible missing comma found. Context: ...ig` directory. If you want to keep your configuration you should skip this step or create bac... (AI_HYDRA_LEO_MISSING_COMMA) --- [uncategorized] ~37-~37: Possible missing article found. Context: ...ion you should skip this step or create backup of the `config` directory. ```bash ... (AI_HYDRA_LEO_MISSING_A) </details> </details> --- `68-71`: **Enhance NVIDIA GPU configuration instructions** The note about NVIDIA GPU configuration could be more detailed and include verification steps. Add verification steps: ```bash # Verify NVIDIA Container Toolkit installation nvidia-ctk --version nvidia-container-cli info # Test GPU access docker run --rm --gpus all nvidia/cuda:11.0-base nvidia-smi🧰 Tools
🪛 LanguageTool
[style] ~68-~68: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 2364 characters long)
Context: ...e -f compose.simulation.yaml up ``` > [!NOTE] > > 1. You can change robot model ...(EN_EXCESSIVE_EXCLAMATION)
[uncategorized] ~71-~71: Possible missing article found.
Context: ...DIA Container Toolkit installed, modify following Docker compose file by replacing `*cpu-...(AI_HYDRA_LEO_MISSING_THE)
.github/workflows/build-docker.yaml (1)
71-85: Consider adding Docker image security scanningThe workflow builds Docker images but doesn't include security scanning. Consider adding a step to scan for vulnerabilities.
You could add Trivy scanning after the build step:
- name: Scan Docker Image uses: aquasecurity/trivy-action@master with: image-ref: 'your-image-name:${{ matrix.tag }}' format: 'table' exit-code: '1' ignore-unfixed: true vuln-type: 'os,library' severity: 'CRITICAL,HIGH'.github/workflows/unit-tests.yaml (1)
Line range hint
13-14: Review security implications of self-hosted runnerUsing a self-hosted runner can expose your infrastructure to security risks, especially if the workflow can be triggered by external pull requests.
Consider:
- Implementing proper job isolation
- Using GitHub-hosted runners for public repositories
- Adding security controls for workflow dispatch triggers
.github/workflows/release-candidate.yaml (1)
75-79: Validate access token usageThe workflow uses RAFAL_ACCESS_TOKEN for cross-repository operations. Consider using a dedicated service account instead of a personal access token.
Best practices for cross-repository access:
- Use GitHub Apps instead of personal access tokens
- Limit token permissions to the minimum required
- Rotate tokens regularly
- Use environment secrets for different environments
.github/workflows/release-repository.yaml (1)
Line range hint
63-67: Consider simplifying complex conditional expressions.While the
fromJSONusage for boolean parsing is correct, consider extracting complex conditions into shell variables or composite conditions for better readability.Example refactor:
- if: ${{ github.event.inputs.release_candidate != env.MAIN_BRANCH && fromJSON(inputs.automatic_mode) == true }} + if: ${{ env.IS_NOT_MAIN_BRANCH == 'true' && fromJSON(inputs.automatic_mode) == true }}Add at the job level:
env: IS_NOT_MAIN_BRANCH: ${{ github.event.inputs.release_candidate != env.MAIN_BRANCH }}Also applies to: 73-77, 84-88, 95-104
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.dockerignore(1 hunks).github/workflows/build-docker.yaml(1 hunks).github/workflows/pre-commit.yaml(1 hunks).github/workflows/protect-default-branch.yaml(1 hunks).github/workflows/release-candidate.yaml(2 hunks).github/workflows/release-project.yaml(3 hunks).github/workflows/release-repository.yaml(1 hunks).github/workflows/unit-tests.yaml(2 hunks)docker/Dockerfile.hardware(1 hunks)docker/Dockerfile.simulation(1 hunks)docker/README.md(1 hunks)docker/demo/.env.cpu(1 hunks)docker/demo/.env.gpu(1 hunks)docker/demo/compose.hardware.yaml(1 hunks)docker/demo/compose.rviz.yaml(1 hunks)docker/demo/compose.simulation.yaml(1 hunks)docker/utils/setup_environment.sh(1 hunks)docker/utils/update_config_directory.sh(1 hunks)husarion_ugv/package.xml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .dockerignore
- .github/workflows/pre-commit.yaml
🧰 Additional context used
🪛 LanguageTool
docker/README.md
[style] ~26-~26: Consider a more expressive alternative.
Context: ... physical robot or run a simulation. To do this, clone this repository to your rob...
(DO_ACHIEVE)
[uncategorized] ~37-~37: Possible missing comma found.
Context: ...ig` directory. If you want to keep your configuration you should skip this step or create bac...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~37-~37: Possible missing article found.
Context: ...ion you should skip this step or create backup of the config directory. ```bash ...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~61-~61: Possible missing comma found.
Context: ...mulation To give Docker access to your screen run: ```bash xhost local:docker docker...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~68-~68: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 2364 characters long)
Context: ...e -f compose.simulation.yaml up ``` > [!NOTE] > > 1. You can change robot model ...
(EN_EXCESSIVE_EXCLAMATION)
[uncategorized] ~71-~71: Possible missing article found.
Context: ...DIA Container Toolkit installed, modify following Docker compose file by replacing `*cpu-...
(AI_HYDRA_LEO_MISSING_THE)
🪛 Shellcheck (0.10.0)
docker/utils/update_config_directory.sh
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 16-16: Declare and assign separately to avoid masking return values.
(SC2155)
docker/utils/setup_environment.sh
[warning] 8-8: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
🪛 actionlint (1.7.4)
.github/workflows/build-docker.yaml
11-11: input "build_type" of workflow_call event has the default value "development", but it is also required. if an input is marked as required, its default value will never be used
(events)
🔇 Additional comments (12)
docker/demo/.env.cpu (1)
1-1: LGTM! Good error handling for DISPLAY variable.
The parameter expansion with error message is a good practice to catch configuration issues early.
docker/demo/.env.gpu (1)
1-1: LGTM! Consistent error handling with CPU version.
The DISPLAY variable configuration maintains consistency across environments.
docker/demo/compose.rviz.yaml (2)
10-13: Verify the existence of .env.cpu file.
The configuration references an external environment file.
Let's check if the file exists and its contents:
✅ Verification successful
Environment file exists with required display configurations
The .env.cpu file exists in the expected location with appropriate display-related environment variables:
DISPLAY: Ensures X11 display is properly configuredLIBGL_ALWAYS_SOFTWARE: Forces software renderingQT_X11_NO_MITSHM: Disables MIT-SHM X11 extension
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if .env.cpu exists and its contents
if [ -f "docker/demo/.env.cpu" ]; then
echo "File exists"
cat "docker/demo/.env.cpu"
else
echo "File not found"
fi
Length of output: 187
14-18: Verify the existence of .env.gpu file and NVIDIA runtime requirements.
The configuration properly sets up NVIDIA runtime support.
Let's check the GPU environment file and documentation:
✅ Verification successful
GPU configuration is properly documented and implemented
The verification confirms:
.env.gpufile exists with correct NVIDIA environment variables- GPU configuration is documented in
docker/README.mdwith instructions for NVIDIA Container Toolkit installation - GPU config is consistently used across multiple compose files (
compose.rviz.yamlandcompose.simulation.yaml)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check GPU-related files and documentation
if [ -f "docker/demo/.env.gpu" ]; then
echo "GPU env file exists"
cat "docker/demo/.env.gpu"
fi
# Check if README mentions NVIDIA runtime requirements
rg -i "nvidia" README.md
Length of output: 243
Script:
#!/bin/bash
# Let's check for any documentation about GPU requirements or setup
fd -t f "README|INSTALL|SETUP|CONTRIBUTING" -e md -X rg -i "gpu|nvidia"
# Check if there are any other GPU-related environment files
fd -t f ".env" -X rg -i "gpu|nvidia"
# Check if there are any GPU-related configuration files
fd -t f "compose|config|settings" -e yml -e yaml -X rg -i "gpu|nvidia"
Length of output: 1040
husarion_ugv/package.xml (1)
18-19: Verify ROS2 environment variable condition support.
The conditional dependencies based on $HUSARION_ROS_BUILD_TYPE are a good practice for separating hardware and simulation dependencies. However, let's verify if this approach is supported in ROS2.
docker/Dockerfile.hardware (1)
42-43: Verify the entrypoint script modification
The sed command modifies all files ending with _entrypoint.sh. This could be risky if there are multiple such files.
.github/workflows/build-docker.yaml (1)
58-65: Verify platform compatibility for Docker builds
The matrix configuration specifies different platforms for hardware (arm64) and simulation (amd64). Ensure these platform choices align with your deployment requirements.
✅ Verification successful
Platform choices are appropriate for the intended deployment
The platform choices in the matrix configuration are correctly aligned with deployment requirements:
- Hardware Dockerfile (arm64): Uses a generic ROS base image
husarnet/ros:${ROS_DISTRO}-ros-coreand includes hardware-specific dependencies throughhardware_deps.repos. This is appropriate for ARM-based hardware deployments. - Simulation Dockerfile (amd64): Uses the same base image but includes simulation-specific dependencies through
simulation_deps.repos. This is suitable for desktop/server simulation environments.
Both Dockerfiles are platform-agnostic in their instructions, relying on the build platform specified in the workflow matrix, and the dependencies are managed through ROS's platform-aware package management system (rosdep).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Dockerfile contents for platform-specific instructions
echo "Checking hardware Dockerfile for ARM-specific instructions:"
rg -n 'FROM.*arm|RUN.*arm' docker/Dockerfile.hardware
echo "Checking simulation Dockerfile for AMD64-specific instructions:"
rg -n 'FROM.*amd|RUN.*amd' docker/Dockerfile.simulation
Length of output: 379
Script:
#!/bin/bash
# Let's check the base images and any platform-specific configurations in both Dockerfiles
echo "=== Hardware Dockerfile contents ==="
cat docker/Dockerfile.hardware
echo -e "\n=== Simulation Dockerfile contents ==="
cat docker/Dockerfile.simulation
# Also check if there are any platform-specific build arguments or configurations in the workflow file
echo -e "\n=== GitHub Workflow file contents ==="
cat .github/workflows/build-docker.yaml
Length of output: 6010
.github/workflows/release-candidate.yaml (1)
48-52: Add validation for Docker image tags
The sed commands modify Docker compose files without validating the resulting content.
.github/workflows/release-project.yaml (3)
Line range hint 2-29: LGTM! Well-structured workflow inputs with clear validation requirements.
The input parameters are well-documented with explicit format requirements for version and date stamps.
37-44: Security improvement: Using local workflow file.
Good practice using a local workflow file instead of referencing an external repository, which reduces supply chain security risks.
46-49: Verify the 'development' build type configuration.
Please ensure that the build_type: development parameter is properly handled in the build-docker.yaml workflow.
✅ Verification successful
Let me check the actual content of the workflow file to see how the build_type parameter is used.
The 'development' build type parameter is properly configured and used
The build_type parameter is correctly:
- Defined as a workflow input with proper description and validation
- Has "development" as the default value
- Properly passed to the downstream action (husarion-ci/ros-docker-img-action)
- Used consistently across both workflow_call and workflow_dispatch triggers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how build_type parameter is used in the workflow
rg -l "build_type|development" .github/workflows/build-docker.yaml
Length of output: 104
Script:
#!/bin/bash
# Display the content of build-docker.yaml to analyze build_type parameter usage
cat .github/workflows/build-docker.yaml
Length of output: 2829
.github/workflows/release-repository.yaml (1)
Line range hint 2-29: LGTM! Well-structured reusable workflow with typed inputs.
Good improvements:
- Added explicit type definitions for all inputs
- Clear parameter documentation
- Appropriate default values for boolean flags
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/release-candidate.yaml (1)
32-38:⚠️ Potential issueCritical: Remove TODO and implement unit tests
Skipping unit tests in the release candidate workflow is a critical issue that could lead to quality problems in releases.
🧹 Nitpick comments (1)
.github/workflows/release-candidate.yaml (1)
58-59: Consider using GitHub Actions bot for commits.Instead of using a custom action-bot, consider using the default GitHub Actions bot credentials:
- author_name: action-bot - author_email: action-bot@action-bot.com + author_name: github-actions[bot] + author_email: github-actions[bot]@users.noreply.github.com
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release-candidate.yaml(2 hunks)
🔇 Additional comments (1)
.github/workflows/release-candidate.yaml (1)
Line range hint 2-17: LGTM! Well-structured workflow configuration.
The workflow trigger configuration is well-defined with clear input validations and proper environment variable setup.
| with: | ||
| owner: husarion | ||
| repo: docs_new | ||
| github_token: ${{ secrets.GH_PAT }} | ||
| workflow_file_name: test-release.yml | ||
| github_token: ${{ secrets.RAFAL_ACCESS_TOKEN }} |
There was a problem hiding this comment.
Consider using GITHUB_TOKEN instead of personal access token.
Using a personal access token (RAFAL_ACCESS_TOKEN) could be a security risk as these tokens typically have broader permissions. Consider using GITHUB_TOKEN if possible, or ensure the personal token has minimal required permissions.
| - name: Create new branch | ||
| uses: GuillaumeFalourd/create-other-repo-branch-action@v1.5 | ||
| with: | ||
| owner: husarion | ||
| repo: panther-docker | ||
| github_token: ${{ secrets.GH_PAT }} | ||
| workflow_file_name: ros-docker-image.yaml | ||
| ref: ${{ env.RC_BRANCH_NAME }} | ||
| client_payload: | | ||
| { | ||
| "panther_codebase_version": "${{ env.RC_BRANCH_NAME }}", | ||
| "build_type": "development", | ||
| "target_distro": "humble" | ||
| } | ||
| repository_owner: husarion | ||
| repository_name: panther-rpi-os-img | ||
| new_branch_name: ${{ env.RC_BRANCH_NAME }} | ||
| new_branch_ref: ros2-devel | ||
| access_token: ${{ secrets.RAFAL_ACCESS_TOKEN}} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeouts and error handling for external operations.
The workflow makes multiple external calls without proper timeout controls or error handling:
- Branch creation could fail silently
- External workflow triggers have no timeout specified
- Personal access token is used repeatedly
Consider these improvements:
- Add timeout to workflow triggers:
uses: convictional/trigger-workflow-and-wait@v1.6.5
with:
+ trigger_workflow_timeout: '3600' # 1 hour timeout- Use GITHUB_TOKEN where possible or centralize the token usage:
- github_token: ${{ secrets.RAFAL_ACCESS_TOKEN }}
+ github_token: ${{ secrets.GITHUB_TOKEN }}- Add error handling for branch creation:
- name: Create new branch
uses: GuillaumeFalourd/create-other-repo-branch-action@v1.5
+ id: create_branch
with:
repository_owner: husarion
repository_name: panther-rpi-os-img
new_branch_name: ${{ env.RC_BRANCH_NAME }}
new_branch_ref: ros2-devel
access_token: ${{ secrets.RAFAL_ACCESS_TOKEN}}
+ continue-on-error: true
+
+- name: Check branch creation
+ if: steps.create_branch.outcome == 'failure'
+ run: |
+ echo "Failed to create branch"
+ exit 1Also applies to: 86-86, 101-101
| - name: Update docker image tag | ||
| run: | | ||
| sed -i 's/\(husarion\/panther:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.hardware.yaml | ||
| sed -i 's/\(husarion\/panther:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.rviz.yaml | ||
| sed -i 's/\(husarion\/panther-gazebo:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.simulation.yaml |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for sed operations.
The sed commands could fail silently if the files don't exist or if the patterns aren't found. Consider adding error checking:
- sed -i 's/\(husarion\/panther:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.hardware.yaml
- sed -i 's/\(husarion\/panther:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.rviz.yaml
- sed -i 's/\(husarion\/panther-gazebo:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.simulation.yaml
+ for file in docker/demo/compose.{hardware,rviz}.yaml; do
+ if [ ! -f "$file" ]; then
+ echo "Error: $file not found"
+ exit 1
+ fi
+ if ! sed -i 's/\(husarion\/panther:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' "$file"; then
+ echo "Error: Failed to update $file"
+ exit 1
+ fi
+ done
+
+ if [ ! -f "docker/demo/compose.simulation.yaml" ]; then
+ echo "Error: compose.simulation.yaml not found"
+ exit 1
+ fi
+ if ! sed -i 's/\(husarion\/panther-gazebo:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.simulation.yaml; then
+ echo "Error: Failed to update compose.simulation.yaml"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Update docker image tag | |
| run: | | |
| sed -i 's/\(husarion\/panther:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.hardware.yaml | |
| sed -i 's/\(husarion\/panther:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.rviz.yaml | |
| sed -i 's/\(husarion\/panther-gazebo:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.simulation.yaml | |
| - name: Update docker image tag | |
| run: | | |
| for file in docker/demo/compose.{hardware,rviz}.yaml; do | |
| if [ ! -f "$file" ]; then | |
| echo "Error: $file not found" | |
| exit 1 | |
| fi | |
| if ! sed -i 's/\(husarion\/panther:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' "$file"; then | |
| echo "Error: Failed to update $file" | |
| exit 1 | |
| fi | |
| done | |
| if [ ! -f "docker/demo/compose.simulation.yaml" ]; then | |
| echo "Error: compose.simulation.yaml not found" | |
| exit 1 | |
| fi | |
| if ! sed -i 's/\(husarion\/panther-gazebo:humble-\)[^[:space:]]*/\1${{ env.RC_BRANCH_NAME }}/g' docker/demo/compose.simulation.yaml; then | |
| echo "Error: Failed to update compose.simulation.yaml" | |
| exit 1 | |
| fi |
Waiting on: #467
Description
Requirements
Tests 🧪
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
README.mdfor Docker images with comprehensive usage and setup instructions.Chores