Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: (reverts #108) full ros2-ws Dockerfile with fixed SHA256 for ros:iron #109

Merged
merged 8 commits into from
May 13, 2024

Conversation

eeberhard
Copy link
Member

Description

The last PR #108 reverted three previous commits and replaced the ros2-ws Dockerfile with a simple wrapper on a previously tagged version of the (now legacy) ros2-control image. It was merged as a hotfix because the underlying docker.io/library/ros:iron image has been rebuilt and retagged many times since our last build of ros2-control, so the most recent build of ros2-ws with the plain ros:iron tag actually included breaking changes to the rclcpp::Node API and therefore breaking downstream users like the modulo library. Using the old ros2-control image that was last built in September 11, 2023 was a temporary workaround so that we could continue refactoring downstream users to expect the new holistic ros2-ws image instead of the previous variants like ros2-control-libraries, ros2-modulo or ros2-control.

Now that the initial hotfix has had time to settle in, I would prefer to revert the revert, i.e. to go back to the intended future structure of this Dockerfile and repository. However, we still want to avoid the breaking change in the base ros:iron image (for now). This can be achieved using a specific SHA256 digest instead of the plain tag.

The repository info for official docker images (https://github.com/docker-library/repo-info) is updated with the SHA256 digest of all builds. By going back in history to the ROS tag-details file from September 02, 2023, we can identify the SHA256 digest of our desired base ros:iron image as:

ros@sha256:6df9b084cb7e918455df628126b2b647d2ad2e4b0e979fb8fbc525a610573bbb

I updated the Dockerfile and build workflow in commit 7d98f0a to use more build args (ROS_DISTRO, BASE_IMAGE, BASE_TAG) and some conditional logic to set the specific digest for iron while using the normal tag for other distros.

As part of this followup work, in commit 4bf3b7b I also fixed an issue with the image metadata. The tech.aica.image.metadata base.version currently uses BASE_TAG (which is equivalent to the ROS distro, i.e. iron)). I realized that package-builder:v1 will fail to parse the metadata since it expects the base version as a semver string. We will get an error along the lines of:

ERROR: failed to solve: invalid AICA metadata "{\"type\":\"base/ws\",\"base\":{\"name\":\"docker.io/library/ros\",\"version\":\"iron\"}}" in image "@aica/foss/ros2-ws": Malformed version: iron

In my opinion the simplest and safest approach is to use the VERSION arg instead (which is set from the git tag, i.e. vX.Y.Z-foo). This way the version is parsed as valid, and we can still introspect the ROS distro from it. As far as I am aware, the exact version or formulation of LabelAICABase.Version is not critical as long as we are consistent with it, but please correct me if I am mistaken @LouisBrunner.

Finally, while adjusting the image metadata, I also added a label for devcontainer metadata (see more info at https://containers.dev/implementors/reference/#labels). This basically sets some defaults which can be interpreted by devcontainer users like VSCode (unless those properties are overwritten by .devcontainer.json file). I thought the container user ros2 and the mount path for /home/ros2/ws/src are very useful defaults to have in there. I could also leave that for a separate PR, but I didn't want to rebuild patches 1.0.1 and 1.0.2 back to back for no reason.

Review guidelines

Estimated Time of Review: 15 minutes

The first four revert commits are just plain reverts of #108 in reverse order.

Then review the next three commits in order, which fix AICA metadata, set a specific base image and tag, and add devcontainer metadata accordingly.

Checklist before merging:

  • Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

* Change tech.aica.image.metadata base.version to use VERSION
(expected as semver vX.Y.Z-foo) instead of BASE_TAG (which was
equiavlent to ROS distro (i.e iron). Otherwise, package-builder:v1
will fail to parse the metadata since it expects the base version
as a semver string
* Include more build args in Dockerfile to disambiguate ROS_DISTRO
from BASE_TAG and to support different BASE_IMAGE

* Update build push workflow to set new build args with condition
based on ROS distro

* Use specific SHA256 digest image and tag for ros:iron base image
based on a build from September 02, 2023
* Set default container user and mounts for devcontainers
(https://containers.dev/implementors/reference/#labels)
Copy link
Member

@domire8 domire8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

ros2_ws/Dockerfile Outdated Show resolved Hide resolved
@domire8
Copy link
Member

domire8 commented May 13, 2024

Thanks for tracking down the SHA!

@eeberhard eeberhard merged commit 6f5c5ad into main May 13, 2024
@eeberhard eeberhard deleted the revert/ros2-ws branch May 13, 2024 10:58
LABEL org.opencontainers.image.base.name="${BASE_IMAGE}:${BASE_TAG}"
LABEL tech.aica.image.metadata='{"type":"base/ws","base":{"name":"${BASE_IMAGE}:${BASE_TAG}","version":"${VERSION}"}}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the value is correct, the formatting isn't. The previous version used "'${VAR}'" for a specific reason (' strings do not extrapolate variables). This breaks the metadata, see:

                "tech.aica.image.metadata": "{\"type\":\"base/ws\",\"base\":{\"name\":\"${BASE_IMAGE}:${BASE_TAG}\",\"version\":\"${VERSION}\"}}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants