You got rid of setting this key when there is no proxy variable set. Seems we shouldn't need it when there is a proxy either. Any reason you left it here?

I added back a build arg in 5b460ff

What does this buy us?

I couldn't get the system test to pass without this, suggesting that somehow different systems were interfering with each other. WIthout this variable set, all ROS 2 systems on the network can see each other. This variable makes it so only the ROS 2 systems with ID 22 can see each other.

I'm assuming that somewhere at Travis, at any given moment there must be some ROS 2 node running causing interference.

Anyhow, it's an ugly hack. Clearly the system test needs work. Maybe I'll just disable it again for the time being.

Is there a reason you need to enable the host network driver? I.e: docker run ... --net=host. I feel this is unnecessary when only using one container.

Well ... I tried it with and without setting that flag, and it only worked with it set. My assumption was that the network restrictions in docker were interfering with the ROS 2 infrastructure, but I never investigated why exactly.

As I recall, AMCL couldn't receive a map from the map server without that set. This is the first communication between elements of the navigation stack, so it seemed even the most basic message passing was inoperable without --net=host

However, this could stand more investigation.

Could we test the latest commit of this PR? I'd like to see exact errors.

Now that the system test is running again, I retried without --net=host and without setting the ROS_DOMAIN_ID and the system test seems to work fine.

I'm not sure why they seemed necessary before. All I can think of is that, since the test was flaky, I just got unlucky and got random passes with those set and got random failures when they weren't.

Could possibly expand this into a build matrix for release, debug, etc...
https://docs.travis-ci.com/user/environment-variables/#global-variables

Thanks!

I made this multiline to better breakout future arguments

'bash -l' wasn't working when non-interactively detached, so a swapped to sleep infinity instead.

Using /ros_entrypoint.sh with a given command is the same as sourcing, given how the dockerfile was prepared.

Is there a more official org for ros-planning docker hub repos, or is stevemacenski the indefinite de facto. Might be nice to play upon the CMAKE_BUILD_TYPE env/matrix to push debug and release tags if that would be helpful.

We don't currently have a more official docker hub. I'm not sure how to create one. @SteveMacenski have you looked at that?

If we do end up creating a org on DockerHub, I'd recommend something along the lines described in theAction Items section of #534

I saw in #527 that the retry ci was removed travis. If you add anymore helper CI scripts, I might suggest you mount them as volumes at the docker run command rather than baking then into the build image.

We didn't merge #527 because we found a real issue that is being addressed in #538, so we DO want the system tests to stay enabled. If you have a better way to do this from the Travis file, please add it to the PR.

What exact command do you want to run?
We can just add it to the current order of scripts in the .travis.yaml

The Travis CI is calling the ctest_retry, which runs the system tests and retries up to 3 times, if they pass once, it returns true. This is because we have some system issues that cause about a 20% flaky test rate. We're working on fixing those by changing to use lifecycle nodes in the future, but this allows us to get the flaky fail rate down to ~1% and enable the test in CI to validate PRs. We don't want to remove this without another equivalent method.

For changing up the environment at test or runtime, I'd suggest passing a env.list file to docker run/exec in the travis file, rather than baking them into the build image.
https://docs.docker.com/engine/reference/commandline/run/#set-environment-variables--e---env---env-file

In that case we have to set this for every docker run command right? Is there no way to set it once?

In that case we have to set this for every docker run command right?

Each time you you invoke docker run you start a separate container, so correct, yes.

Is there no way to set it once?

Not without baking it into the image or reusing the same container. Yet adding a single --env-file ./env.list arg to be able to dynamically set all the runtime environment variables your CI may need seems far simpler, given Docker still has no way of Reset properties inherited from parent image: moby/moby#3465

However I think setting the ROS_DOMAIN_ID is completely unnecessary given we probably shouldn't have been using --net=host network driver to begin with. That'd just be a timebomb until someone else's on travis happens to use the same ID, or we trigger two CI jobs scheduled in parallel and they start cross talking during tests due to bypassing the network isolation.

I sorta feel like we should be COPYing in the source code from the current build context, rather than going through the trouble of cloning from a remote URL, given that much of Travis and CIs sort of deal with merging and staging the repo (build context) before running our scripts. But I guess that depends on what the expected use case of this Dockerfile is. Does one clone this repo to acquire said dockerfile, or expect to build it with the one Dockerfile standalone.

This might also make the build more traceable, as there would be so much of a chance for race conditions when building while other commits are merged into the tracked BRANCH.

There was a discussion about this in #389. @SteveMacenski @mkhansen-intel Are you still opposed to this?

Will a copy in docker build actually copy from the host machine? I'm not sure it will, the container wouldnt have permissions

Will a copy in docker build actually copy from the host machine?

Yes, building the dockerfile locally on the host works just the same as if its was build using CIs like Travis, CircleCI, or an automated repo on Docker Hub. One would simply checkout the remote PR they want to test locally, and then invoke docker build in desired branch.

I'm not sure it will, the container wouldnt have permissions

I'm not sure what you mean here. When you build a Dockerfile, the entire directory that the Dockerfile resides in used as the build context for the Docker engine. By default, COPY does not preserve the permissions of the content copied from the build context unless you explicitly configure it.

https://docs.docker.com/engine/reference/builder/#copy

Here the default user is left unchanged as root, given this Dockerfile is just used for building, not hosting a web service/database or anything.

In that case we have to set this for every docker run command right? Is there no way to set it once?

We don't currently have a more official docker hub. I'm not sure how to create one. @SteveMacenski have you looked at that?

Why move the nav2 workspace under /opt ? Is there a reason for that?

Not much other than to keep image file heigheracy tidy. This might be helpful to organize the workspace that the entrypoint is sourcing, rather than polluting the root directory. This mainly just me trying to establish a template other ROS2 projects could replicate, or build upon. Say, perhaps if this navigation2 source build image is somehow reused or built FROM by another CI in ros-planning.

This will make it so the dockerfile can't be run as a stand-alone outside of the navigation2 folder. I believe @SteveMacenski had a use case which needed that capability

Most Docker enabled pipelines (DockerHub/Travis/CircleCI/GitLab) work on the premise of downloading the target Dockerfile and its build context before building, so deviating this pattern would make things harder to robustly integrate. For one, it would make it hard to ensure a one-to-one commit-to-ci relationship when the Dockerfile build is compiling the head of PR branch, and not the exact commit that triggered the CI 30-min ago while waiting in the Travis job/slot que. Building an image to test a branch using a Dockerfile not from that branch would be dangerous; like risking reproducibility by building from both the wrong blueprint and with outdated measurements.

Citing "Best practices for getting code into a container (git clone vs. copy vs. data container)":

We never use git clone, beacuse using it you are not able to pull specific commit with easy way in CI and you are not interested in latest commit, cause during build on CI new commit can appear.

https://forums.docker.com/t/best-practices-for-getting-code-into-a-container-git-clone-vs-copy-vs-data-container/4077/3

Also, the previous Dockerfile from before was copying artifacts from the local build context, so checkout-and-copy was already necessary to get the context (if not the Dockerfile itself). Plus, it's not much of an issue if we want to clone a PR branch on github if we want to build the Dockerfile locally, and still be confident about the exact commit we want to test:
https://help.github.com/articles/checking-out-pull-requests-locally

What sets the $ROS_DISTRO env var?

This is an environment variable set upstream in all ros related images from osrf, and enables downstream images to be more postable across distro releases, e.g. preventing folks from having to hardcode a dockerfile to the distro installed from the current base image.

https://github.com/osrf/docker_images/blob/ea0fdabf250f816203d058dca1df12802a9c44cc/ros2/nightly/nightly/Dockerfile#L45

We didn't merge #527 because we found a real issue that is being addressed in #538, so we DO want the system tests to stay enabled. If you have a better way to do this from the Travis file, please add it to the PR.