-
Notifications
You must be signed in to change notification settings - Fork 79
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
Append ros namespace to target container if available. #429
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Mukunda Bharatheesha <[email protected]>
ae1920c
to
f5671d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulls: #429 |
Thanks @fujitatomoya . All builds succeeded it seems |
Thank you for the PR! I'm not too familiar with the launch internals. Would you be willing to add a test to https://github.com/ros2/launch_ros/tree/rolling/launch_ros/test that shows how this fixes the issue? 🧇 |
@mbharatheesha I'm wondering if containers should automatically get the ros namespace pushed onto them. What if you have a higher level abstract container in which you want to put a bunch of namespaced nodes and topics. This would make a setup like this much harder to achieve, because you would have to namespace everything going into the container separately. Example: container use-case 2: This change will make it such that you can no longer push ros namespace in use-case 1, but it makes use-case 2 easier to achieve. So the only question for me is, how often do people use use-case 1 vs use-case 2, if use-case 1 never happens then this is an improvement, otherwise it makes use-case 1 cumbersome. |
This PR introduces a change to fix #428.
With this change, when a composable node is loaded into a target container that is running under a
ros_namespace
set using<push_ros_namespace>
, the target container name gets prefixed withros_namespace
using theLaunchContext
. This enables the composable nodes to load into a namespaced target container.How did I test this:
apt install ros-rolling-image-tools
ros2 node list
Without the changes in this PR, the output from the launch would be:
And the composable nodes wouldn't show up with
ros2 node list