420 feature execute catkin make on docker compose up #497
Conversation
catkin_make inside the container. One of them is more convenient as a list of containers is automatically provided. It however depends on a vscode extension not yet in recomendations. We might need to select one of the two options and remove the other.
As the addition of a vscode extension is no problem, the cumbersome way of selecting the task via an input field was removed. This now requires the tasksshellinput extension.
WalkthroughThe changes involve updates to two Visual Studio Code configuration files. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VSCode
participant Docker
User->>VSCode: Select task "Run 'catkin_make' in docker container"
VSCode->>User: Show list of running containers
User->>VSCode: Choose a container
VSCode->>Docker: Execute command to enter container
Docker-->>VSCode: Container access granted
VSCode->>Docker: Navigate to `catkin_ws` and source ROS setup
VSCode->>Docker: Run `catkin_make`
Docker-->>VSCode: Build completed
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.vscode/tasks.json (1)
1-22: This implementation doesn't meet the PR objectiveThe PR's objective is to "execute catkin make on docker compose up", but these changes only add a manual VS Code task. To achieve the actual objective, you should:
- Modify the
docker-compose.ymlto runcatkin_makeduring container startup- Consider using a Docker entrypoint script that:
- Checks if build is needed
- Runs catkin_make
- Starts the actual application
Would you like assistance in implementing the correct solution in
docker-compose.yml?🧰 Tools
🪛 Biome
[error] 10-10: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 19-19: Expected a property but instead found '}'.
Expected a property here.
(parse)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.vscode/extensions.json(1 hunks).vscode/tasks.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/extensions.json
🧰 Additional context used
🪛 Biome
.vscode/tasks.json
[error] 10-10: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 19-19: Expected a property but instead found '}'.
Expected a property here.
(parse)
🔇 Additional comments (2)
.vscode/tasks.json (2)
12-21: 🛠️ Refactor suggestion
Input configuration needs improvement for better error handling
The current implementation has several issues:
- No filtering of containers (might list unrelated containers)
- No handling of empty container list
- JSON formatting issues
Consider this improved implementation:
"inputs": [
{
"id": "container_name",
"type": "command",
"command": "shellCommand.execute",
"args": {
- "command": "docker ps --format '{{.Names}}'"
+ "command": "docker ps --filter 'label=com.docker.compose.project=paf' --format '{{.Names}}' | grep . || echo 'No running PAF containers found' >&2"
- }
+ },
+ "description": "Select a running PAF container"
}
]Let's verify the container labeling:
#!/bin/bash
# Check if docker-compose configuration properly labels containers
rg -g 'docker-compose*.yml' 'labels:'🧰 Tools
🪛 Biome
[error] 19-19: Expected a property but instead found '}'.
Expected a property here.
(parse)
4-10:
Task configuration needs improvements for reliability and security
Several issues need to be addressed:
- The
-itflags are unnecessary for automated task execution and could cause issues - Relative path navigation (
../../) is fragile and could break if the workspace structure changes - The command assumes bash is available in the container
- No error handling for failed commands
- This task configuration doesn't achieve the PR objective of running
catkin_makeondocker-compose up
Consider this improved implementation:
{
"label": "Run 'catkin_make' in docker container. Choose container from list of running containers.",
"type": "shell",
- "command": "docker exec -it ${input:container_name} bash -c 'cd ../../catkin_ws && source /opt/ros/noetic/setup.bash && bash devel/setup.bash && catkin_make'",
+ "command": "docker exec ${input:container_name} sh -c '[ -d /catkin_ws ] || exit 1; cd /catkin_ws && source /opt/ros/noetic/setup.sh && if [ -f devel/setup.sh ]; then . devel/setup.sh; fi && catkin_make'",
"problemMatcher": [],
- "detail": "Executes 'catkin_make' selected container. Requires Tasks Shell Input Extension, in order to generate the list of containers.",
+ "detail": "Executes 'catkin_make' in selected container. Note: This is a manual task. For automatic builds, modify docker-compose.yml instead."
}Let's verify the container configuration:
🧰 Tools
🪛 Biome
[error] 10-10: Expected a property but instead found '}'.
Expected a property here.
(parse)
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Does this PR introduce a breaking change?
e.g. is old functionality not usable anymore
Most important changes
Which files functionalities are most important in this PR. On which part should the reviewer be focussed on?
Checklist:
Summary by CodeRabbit
"augustocdias.tasks-shell-input".catkin_makewithin a Docker container, allowing users to easily build their ROS workspace.These updates enhance the development experience by streamlining task execution and providing useful extension recommendations.