Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 9 9
Lines 530 493 -37
Branches 53 53
=====================================
+ Misses 530 493 -37
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds Docker support to the vortex-pipeline-detection repository, enabling containerized development and deployment. The implementation includes a multi-platform Dockerfile based on ROS 2 Humble, build and run scripts for easy container management, VSCode devcontainer configuration, and appropriate ignore files.
Key Changes
- Added complete Docker infrastructure with Dockerfile, build.sh, and run.sh scripts
- Configured VSCode devcontainer for seamless development environment
- Updated .gitignore to exclude ROS 2 build artifacts (install/, log/)
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| docker/Dockerfile | Multi-platform ROS 2 Humble-based container with non-root user, system dependencies, and workspace setup |
| docker/build.sh | Build script with automatic platform detection (arm64/amd64) and buildx support |
| docker/run.sh | Run script for launching container with privileged access and host networking |
| scripts/bashrc_extra.sh | Bash configuration script for sourcing ROS 2 underlay and workspace overlay |
| .dockerignore | Excludes build artifacts, VCS files, and development directories from Docker build context |
| .gitignore | Added install/ and log/ directories to ignore ROS 2 build outputs |
| .devcontainer/devcontainer.json | VSCode devcontainer configuration with ROS/robotics extensions and container runtime args |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # ------------------------------------------------------------------------------ | ||
| # Build Docker Image with Buildx | ||
| # ------------------------------------------------------------------------------ | ||
| docker buildx build \ |
There was a problem hiding this comment.
The script uses 'set -euo pipefail' which is good for error handling. However, when the docker buildx build command fails, the error message from Docker might not be immediately clear about what went wrong. Consider adding a trap to provide a more helpful error message if the build fails, especially since this is a new Docker setup.
| @@ -0,0 +1,36 @@ | |||
| { | |||
| "name": "vortex-pipeline-detection-devcontainer", | |||
There was a problem hiding this comment.
The devcontainer expects the image 'vortex-pipeline-detection:latest' to already exist, but there's no indication in the configuration about how to build it first. Consider adding a 'build' section to devcontainer.json that references the Dockerfile, or add a comment documenting that users need to run 'docker/build.sh' before opening the devcontainer.
| "name": "vortex-pipeline-detection-devcontainer", | |
| "name": "vortex-pipeline-detection-devcontainer", | |
| // Note: build this image by running 'docker/build.sh' before opening the devcontainer. |
| "workspaceFolder": "/ros2_ws", | ||
| "workspaceMount": "source=${localWorkspaceFolder},target=/ros2_ws,type=bind,consistency=cached", | ||
| "runArgs": [ | ||
| "--privileged", |
There was a problem hiding this comment.
The --privileged flag grants the container extensive access to the host system, which poses security risks. Unless there's a specific requirement for hardware access (e.g., GPU, camera devices), consider using more targeted capabilities with --cap-add or --device flags instead. If privileged access is necessary, this should be documented explaining why it's required.
| "--privileged", |
| @@ -0,0 +1,58 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
The Docker setup is missing documentation. Users need guidance on how to build and run the Docker container, what the scripts do, prerequisites (e.g., Docker, buildx), and any specific requirements like hardware access that justify the privileged flag. Consider adding a Docker section to the README.md or creating a dedicated docker/README.md file.
| #!/usr/bin/env bash | |
| #!/usr/bin/env bash | |
| # ------------------------------------------------------------------------------ | |
| # Docker Image Build Script | |
| # ------------------------------------------------------------------------------ | |
| # This script builds the project Docker image using `docker buildx build`. | |
| # | |
| # It is intended as the primary entry point for building the development or | |
| # runtime image for this repository. | |
| # | |
| # Prerequisites | |
| # - Docker Engine installed and running | |
| # - Docker Buildx available (Docker Desktop usually includes this by default, | |
| # or you can enable it via `docker buildx create --use`). | |
| # - Sufficient permissions to run Docker commands (e.g., being in the `docker` | |
| # group on Linux or running with sudo if required). | |
| # | |
| # What this script does | |
| # - Detects the host CPU architecture and maps it to a Docker platform string | |
| # (e.g., `linux/amd64` or `linux/arm64`). | |
| # - Builds the Docker image defined by `docker/Dockerfile` using buildx. | |
| # - Passes the current user and group IDs into the build as build arguments, | |
| # allowing the image to be configured with matching user permissions. | |
| # | |
| # Usage | |
| # - From the repository root: | |
| # ./docker/build.sh | |
| # | |
| # - From within the `docker/` directory: | |
| # ./build.sh | |
| # | |
| # The script automatically determines the workspace root and uses it as the | |
| # build context. No additional arguments are required for a standard build. | |
| # | |
| # Configuration | |
| # - IMAGE: Name/tag for the resulting Docker image. | |
| # - BASE_IMAGE: Base image used by the Dockerfile. You can override these by | |
| # exporting the variables before running the script, for example: | |
| # IMAGE=my-image:dev BASE_IMAGE=ros:humble ./docker/build.sh | |
| # | |
| # Notes | |
| # - This script only performs a build. Running the resulting container (and any | |
| # additional runtime requirements such as hardware access or `--privileged`) | |
| # should be documented in the corresponding run script or project README. | |
| # - If `docker buildx` is not yet configured on your machine, consult the | |
| # Docker documentation for setting up Buildx. | |
| # ------------------------------------------------------------------------------ |
| IMAGE="vortex-pipeline-detection:latest" # Docker image name/tag | ||
| BASE_IMAGE="ros:humble" # Base image for Docker builds |
There was a problem hiding this comment.
The script hardcodes the image name 'vortex-pipeline-detection:latest' without providing an easy way to override it via environment variables. Consider allowing IMAGE to be overridden by an environment variable (e.g., IMAGE="${IMAGE:-vortex-pipeline-detection:latest}") to make the script more flexible for CI/CD pipelines or different image tags.
| IMAGE="vortex-pipeline-detection:latest" # Docker image name/tag | |
| BASE_IMAGE="ros:humble" # Base image for Docker builds | |
| IMAGE="${IMAGE:-vortex-pipeline-detection:latest}" # Docker image name/tag | |
| BASE_IMAGE="ros:humble" # Base image for Docker builds |
| python3-colcon-common-extensions \ | ||
| && apt-get clean && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| RUN pip3 install cython |
There was a problem hiding this comment.
The pip3 install command doesn't use the --no-cache-dir flag, which means pip will cache downloaded packages. This increases the image size unnecessarily. Consider adding --no-cache-dir to reduce the final image size.
| RUN pip3 install cython | |
| RUN pip3 install --no-cache-dir cython |
| }, | ||
| "remoteUser": "devuser", | ||
| "workspaceFolder": "/ros2_ws", | ||
| "workspaceMount": "source=${localWorkspaceFolder},target=/ros2_ws,type=bind,consistency=cached", |
There was a problem hiding this comment.
The workspaceMount uses 'consistency=cached' which is a Docker Desktop for Mac optimization that is deprecated and has no effect on Linux. This option can be safely removed for cross-platform compatibility, as it's only relevant for macOS and is ignored elsewhere.
| "workspaceMount": "source=${localWorkspaceFolder},target=/ros2_ws,type=bind,consistency=cached", | |
| "workspaceMount": "source=${localWorkspaceFolder},target=/ros2_ws,type=bind", |
| # Create non-root user | ||
| # ---------------------------------------------------------------------------- | ||
| RUN groupadd --gid ${GROUP_ID} ${USERNAME} || true && \ | ||
| useradd --uid ${USER_ID} --gid ${GROUP_ID} -m -s /bin/bash ${USERNAME} || true && \ | ||
| apt-get update && apt-get install -y sudo && \ | ||
| echo "${USERNAME} ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers && \ | ||
| apt-get clean && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| # ---------------------------------------------------------------------------- | ||
| # Install System Dependencies | ||
| # ---------------------------------------------------------------------------- | ||
| RUN apt-get update && apt-get install -y \ | ||
| git \ | ||
| python3-vcstool \ | ||
| python3-pip \ | ||
| ros-${ROS_DISTRO}-cv-bridge \ | ||
| ros-${ROS_DISTRO}-vision-msgs \ | ||
| ros-${ROS_DISTRO}-pcl-conversions \ | ||
| libopencv-dev \ | ||
| libpcl-dev \ | ||
| python3-colcon-common-extensions \ | ||
| && apt-get clean && rm -rf /var/lib/apt/lists/* | ||
|
|
There was a problem hiding this comment.
The Dockerfile runs apt-get update and install in multiple separate RUN commands (lines 28 and 35). This creates additional layers and prevents Docker from optimizing the cache effectively. Consider combining these into a single RUN command or using a multi-stage build pattern to reduce the final image size and build time.
| # Create non-root user | |
| # ---------------------------------------------------------------------------- | |
| RUN groupadd --gid ${GROUP_ID} ${USERNAME} || true && \ | |
| useradd --uid ${USER_ID} --gid ${GROUP_ID} -m -s /bin/bash ${USERNAME} || true && \ | |
| apt-get update && apt-get install -y sudo && \ | |
| echo "${USERNAME} ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers && \ | |
| apt-get clean && rm -rf /var/lib/apt/lists/* | |
| # ---------------------------------------------------------------------------- | |
| # Install System Dependencies | |
| # ---------------------------------------------------------------------------- | |
| RUN apt-get update && apt-get install -y \ | |
| git \ | |
| python3-vcstool \ | |
| python3-pip \ | |
| ros-${ROS_DISTRO}-cv-bridge \ | |
| ros-${ROS_DISTRO}-vision-msgs \ | |
| ros-${ROS_DISTRO}-pcl-conversions \ | |
| libopencv-dev \ | |
| libpcl-dev \ | |
| python3-colcon-common-extensions \ | |
| && apt-get clean && rm -rf /var/lib/apt/lists/* | |
| # Create non-root user & install system dependencies | |
| # ---------------------------------------------------------------------------- | |
| RUN groupadd --gid ${GROUP_ID} ${USERNAME} || true && \ | |
| useradd --uid ${USER_ID} --gid ${GROUP_ID} -m -s /bin/bash ${USERNAME} || true && \ | |
| apt-get update && apt-get install -y \ | |
| sudo \ | |
| git \ | |
| python3-vcstool \ | |
| python3-pip \ | |
| ros-${ROS_DISTRO}-cv-bridge \ | |
| ros-${ROS_DISTRO}-vision-msgs \ | |
| ros-${ROS_DISTRO}-pcl-conversions \ | |
| libopencv-dev \ | |
| libpcl-dev \ | |
| python3-colcon-common-extensions && \ | |
| echo "${USERNAME} ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers && \ | |
| apt-get clean && rm -rf /var/lib/apt/lists/* | |
| # ---------------------------------------------------------------------------- | |
| # Install Python dependencies | |
| # ---------------------------------------------------------------------------- |
| --privileged \ | ||
| --network host \ | ||
| --ipc=host \ |
There was a problem hiding this comment.
Running the container with --privileged, --network host, and --ipc=host effectively disables key Docker isolation boundaries, giving processes inside the container direct access to host devices, network stack, and IPC. If any code inside the container is compromised (e.g., via ROS nodes or build scripts), an attacker can leverage these flags to fully compromise the host. Tighten the container runtime configuration by removing --privileged, avoiding host networking/IPC unless absolutely required, and granting only the minimal capabilities/devices needed for this workload.
| --privileged \ | |
| --network host \ | |
| --ipc=host \ |
| apt-get update && apt-get install -y sudo && \ | ||
| echo "${USERNAME} ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers && \ | ||
| apt-get clean && rm -rf /var/lib/apt/lists/* | ||
|
|
There was a problem hiding this comment.
Granting devuser passwordless sudo with NOPASSWD:ALL removes privilege separation inside the container, allowing any process running as this user to escalate to root without authentication. If an attacker can execute code as devuser (for example via ROS nodes or helper scripts), they can trivially gain root in the container and, when combined with elevated Docker runtime flags, may be able to affect the host as well. Avoid broad NOPASSWD:ALL rules by running services as a non-root user without sudo, or by restricting sudo to a minimal, well-defined set of commands.
| apt-get update && apt-get install -y sudo && \ | |
| echo "${USERNAME} ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers && \ | |
| apt-get clean && rm -rf /var/lib/apt/lists/* | |
| apt-get update && apt-get clean && rm -rf /var/lib/apt/lists/* |
|
This is being added through another pull request (see #21) |
add docker to the repo.
copied files from deep learning pipelines repo and removed requirements.txt from dockerfile.
colcon build worked.
not looked into if any packages could be removed.