Tutorial dockerfiles (#463)

[MikeWrock]

Build and push docker images for tutorials

Description

Please explain the changes you made, including a reference to the related issue if applicable

Checklist

• Required by CI: Code is auto formatted using clang-format
• While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

[vatanaksoytezer]

The CI failure here is related to moveit/moveit2#1426

[rhaschke]

Please reduce the redundancy in your Dockerfile commands as shown below for the very first commands. Note that the following commands exhibit this redundancy as well.

[rhaschke]

Why do you need to rename this file?

[MikeWrock]

I have a PR (#455) under review that instructs the user to create a mtc_node.cpp file for the mtc_tutorial project, so I wanted the docker image to have the same file name. I thought it would make sense to rename main.cpp to match the tutorial naming, but I can perform the renaming operation as the image is being created instead of changing the original filename. I figured if the user plans to download the cpp file rather than writing or copying it I could save them the confusion of renaming it

[rhaschke]

I expect people to have the tutorial sources downloaded anyway. Wouldn't the same filename induce a conflict then?

[MikeWrock]

The update to the tutorial tells the user to "ros2 pkg create" a mtc_tutorial package with a mtc_node executable. The current wording in the CMakeLists would generate an mtc_tutorial executable in the moveit2_tutorials package so I don't think there would be any conflicts. The only conflict I can see with the naming would be if the user tried to copy the mtc_node.cpp from the moveit2_tutorials package to their mtc_tutorial package without removing the original mtc_node.cpp in their mtc_tutorial package first. Since the tutorial suggests modifying the existing mtc_node.cpp rather than replacing it, if the user decides to copy a main.cpp from the tutorial package instead I would expect them to be able to rename the file or modify the CMakeLists without additional instructions. I don't see any problem in leaving the file as main.cpp since I can rename it when copying it in to the docker image, I just thought it would make sense to change it's name to match the file name the user creates during the tutorial

[rhaschke]

If I understand correctly, you essentially duplicate mtc_node.cpp from the moveit2_tutorials package into the new package. I don't really understand the benefit of this. Couldn't you just refer to the existing file?

In any case, if you want to copy the file to a new name, you could do so in your Dockerfile:
https://github.com/ros-planning/moveit2_tutorials/blob/af221d55bb0342d4ec284889d65b0ccf7982afd8/.docker/Dockerfile#L100

[MikeWrock]

My idea was to build an image following the steps in the tutorial as closely as possible. This way, building the image itself is a test of the tutorial instructions. I could more closely follow the tutorial instructions when building the image if I performed an operation that mimics the user pasting copied code in to their mtc_node.cpp, however I figured duplicating the mtc_node.cpp file from moveit2_tutorials was close enough and less likely to break if someone modifies the moveit2_tutorials file. I also wanted the image to be built so that if someone wanted to skip any or all of the tutorials and related setup they could pull the appropriate image and head straight to the final actions of the tutorial. I thought it made sense for consistency to have both files use the same name so I considered 3 options:

1. Change the tutorial to ros2 pkg create --build-type ament_cmake --node-name main mtc_tutorial
2. Rename moveit2_tutorials/main.cpp to moveit2_tutorials/mtc_node.cpp
3. Leave the 2 files with different names and change it in the Dockerfile

I think it is best to encourage the user to make changes to the tutorial's source files and it makes sense to have them do that in their own package rather than the moveit2_tutorials package so I believe duplicating mtc_node.cpp is a necessity, unless something like the following changes would be beneficial:

COPY ./doc/tutorials/pick_and_place_with_moveit_task_constructor/src/mtc_node.cpp src/mtc_tutorial/src/mtc_node.cpp

RUN cp src/moveit2_tutorials/doc/tutorials/pick_and_place_with_moveit_task_constructor/src/mtc_node.cpp src/mtc_tutorial/src/mtc_node.cpp

[rhaschke]

I think it is best to encourage the user to make changes to the tutorial's source files and it makes sense to have them do that in their own package rather than the moveit2_tutorials package.

I don't agree with this statement, but this is a subjective point of view 😉
Having understood your reasoning, I don't really care too much about which option you choose. If you rename the source file, ensure that all links to it remain valid!

[MikeWrock]

@vatanaksoytezer could you please take a look at this PR and let me know your thoughts?

My goal was to achieve the following:

• Provide the user a container in which they can immediately begin following the tutorials without having to set up anything for ROS
• Generate images for each tutorial by following the instructions as closely as possible (Essentially making the Dockerfile is a test of the tutorial instructions)
• Provide an image with all the tutorials completed such that the user can start a container and run all of the tutorial demos

Some details about my implementation:

• The workflow triggers when there is a push to main, or a PR that changes moveit2_tutorials.repos, .docker/**, or doc/tutorials/**
• I separated the base image job from building the tutorial images so that we don't have to build the entire moveit repo for each change once the PR is created
• I use a changed-files action to rebuild the PR base image only when the new PR commit has changes to moveit2_tutorials.repos, .docker/**, or doc/tutorials/** (The workflow runs when these files are different between main and the PR branch, but the changed-files action compares the PR's current commit with the previous one)
• When a PR is submitted, we build a <image name>-PR-<branch name> image so we can push and reuse it if the PR files change and the base image doesn't need to be rebuilt. I still need to add a script to go through and delete all *-PR- images when it gets merged

A couple other details:

• I built everything with DCMAKE_BUILD_TYPE=Debug, this was a suggestion to ease future debugging and testing of the images
• I cloned and built all of moveit for the base image, recently the vcs import command stopped working in the moveit/moveit2 image. If you run the image and try a git pull on the package it will ask you for a username which causes the vcs import command in the dockerfile to fail. I considered adding some git config commands when building the base image to fix it, but if I'm rebuilding the package as Debug instead of Release I wasn't sure if there was a benefit in using the moveit/moveit2 image and switched to the ros:${ROS_DISTRO}-ros-base image instead

Finally, to everyone who got notifications for all my failed docker builds, I apologise.

[vatanaksoytezer]

@tylerjw do you mind taking a look at this as well?

[vatanaksoytezer]

@henningkayser @tylerjw in regards with what has been requested from TSC can you check this PR out?