Extra parameter to start a container #616

caguero · 2024-09-27T15:47:45Z

🦟 Bug fix

Leaving it as a draft to discuss what people think. The idea behind this extra parameter is to provide more flexibility. We'd have three options:

use_composition:=False . Run as an external ROS node.
use_composition:=True and create_own_container. Load as a composable node within its own ROS container.
use_composition:=True and not create_own_container. Load as a composable node but delegate to the user to specify the ROS container with the container_name parameter.

Summary

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎉 New feature

Closes #

Summary

Test it

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

➡️ Forward port

Port <FROM_BRANCH> to <TO_BRANCH>

Branch comparison: https://github.com/gazebosim//compare/<TO_BRANCH>...<FROM_BRANCH>

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎈 Release

Preparation for <X.Y.Z> release.

Comparison to <x.y.z>: https://github.com/gazebosim//compare/<LATEST_TAG_BRANCH>...<RELEASE_BRANCH>

Needed by <PR(s)>

Checklist

Asked team if this is a good time for a release
There are no changes to be ported from the previous major version
No PRs targeted at this major version are close to getting in
Bumped minor for new features, patch for bug fixes
Updated changelog
Updated migration guide (as needed)
Link to PR updating dependency versions in appropriate repository in gazebo-release (as needed):

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Carlos Agüero <[email protected]>

azeey

I think we should also make use_composition=False the default. The scenario I'd like to prevent is that the Gazebo server is started using gz sim -s (I.e, not using the GzServer launch action) and the bridge is started using the RosGzBridge action. Since the default now is use_composition=True, it will try to add itself into an existing container, but due to the way Gazebo was started, that container will not be available.

azeey · 2024-09-30T22:08:30Z

ros_gz_bridge/launch/ros_gz_bridge.launch.py

@@ -46,6 +47,12 @@ def generate_launch_description():
        description='Name of container that nodes will load in if use composition',
    )

+    declare_start_container_cmd = DeclareLaunchArgument(
+        'start_container',


I would suggest changing this to create_own_container to make it a little more clear that this is not necessary for the bridge to work as a composable node. I would also add more to the description so users don't think they have to set this to True in order to get the benefits of composition.

I renamed the new parameter as suggested and added to the launch action. I also changed the default value of use_composition to False. f0f63b3

Signed-off-by: Carlos Agüero <[email protected]>

ros_gz_bridge/launch/ros_gz_bridge.launch.py

Signed-off-by: Carlos Agüero <[email protected]>

Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Carlos Agüero <[email protected]>

Signed-off-by: Carlos Agüero <[email protected]>

azeey · 2024-10-03T18:49:22Z

Should we do the same for the gz_server launch file? Right now, it's not possible to put the GzServer node in a container created by the consumer.

caguero · 2024-10-03T18:57:15Z

Should we do the same for the gz_server launch file? Right now, it's not possible to put the GzServer node in a container created by the consumer.

I think so.

What do you think about the default value for use_composition in the gz_server launch? In some way it's more of an end-application, which makes me think a value of True isn't as problematic here. But at the same time having a consistency set of launch files (defaulting use_composition to False like in the bridge) makes it easier to use.

Signed-off-by: Carlos Agüero <[email protected]>

azeey · 2024-10-03T20:49:42Z

I don't have a strong opinion, but I lean toward making use_composition=False for both launch files by default. It feels safer at this point.

Signed-off-by: Carlos Agüero <[email protected]>

caguero · 2024-10-10T16:49:56Z

I don't have a strong opinion, but I lean toward making use_composition=False for both launch files by default. It feels safer at this point.

Thanks, changed to use_composition:=False and create_own_container:=False by default.

ahcorde · 2024-10-14T12:09:30Z

@ros-pull-request-builder retest this please

caguero · 2024-10-15T15:04:22Z

https://github.com/Mergifyio backport jazzy

mergify · 2024-10-15T15:04:27Z

backport jazzy

✅ Backports have been created

#618 Extra parameter to start a container (backport #616) has been created for branch jazzy

Signed-off-by: Carlos Agüero <[email protected]> (cherry picked from commit 8115cca)

Signed-off-by: Carlos Agüero <[email protected]> (cherry picked from commit 8115cca) Co-authored-by: Carlos Agüero <[email protected]>

Extra parameter to start a container

a2adf04

Signed-off-by: Carlos Agüero <[email protected]>

caguero requested a review from ahcorde as a code owner September 27, 2024 15:47

caguero marked this pull request as draft September 27, 2024 15:48

azeey reviewed Sep 30, 2024

View reviewed changes

Use create_own_container

f0f63b3

Signed-off-by: Carlos Agüero <[email protected]>

azeey reviewed Oct 2, 2024

View reviewed changes

ros_gz_bridge/launch/ros_gz_bridge.launch.py Outdated Show resolved Hide resolved

caguero and others added 4 commits October 3, 2024 17:53

Style

144a6cf

Signed-off-by: Carlos Agüero <[email protected]>

Fix style

e0c64ea

Signed-off-by: Carlos Agüero <[email protected]>

Update ros_gz_bridge/launch/ros_gz_bridge.launch.py

cac63cf

Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Carlos Agüero <[email protected]>

Tweak documentation

42bdda3

Signed-off-by: Carlos Agüero <[email protected]>

caguero marked this pull request as ready for review October 3, 2024 16:59

Add create_own_container parameter to ros_gz_sim launch files.

306b3bd

Signed-off-by: Carlos Agüero <[email protected]>

caguero added 3 commits October 10, 2024 18:27

Disable use_composition by default.

d6bc3ac

Signed-off-by: Carlos Agüero <[email protected]>

Merge branch 'ros2' into caguero/start_container

4b66447

Set default value for create_own_container to False

26a0a3f

Signed-off-by: Carlos Agüero <[email protected]>

ahcorde approved these changes Oct 14, 2024

View reviewed changes

ahcorde merged commit 8115cca into ros2 Oct 14, 2024
5 checks passed

ahcorde deleted the caguero/start_container branch October 14, 2024 13:59

caguero mentioned this pull request Oct 15, 2024

Updating ros_gz documentation related to composition gazebosim/docs#526

Merged

8 tasks

mergify bot pushed a commit that referenced this pull request Oct 15, 2024

Extra parameter to start a container (#616)

183ae06

Signed-off-by: Carlos Agüero <[email protected]> (cherry picked from commit 8115cca)

mergify bot mentioned this pull request Oct 15, 2024

Extra parameter to start a container (backport #616) #618

Merged

24 tasks

ahcorde pushed a commit that referenced this pull request Oct 15, 2024

Extra parameter to start a container (#616) (#618)

b404dbd

Signed-off-by: Carlos Agüero <[email protected]> (cherry picked from commit 8115cca) Co-authored-by: Carlos Agüero <[email protected]>

Amronos mentioned this pull request Oct 18, 2024

Add create_own_container argument to ros_gz_spawn_model.launch.py #622

Merged

8 tasks

mergify bot mentioned this pull request Oct 25, 2024

Add create_own_container argument to ros_gz_spawn_model.launch.py (backport #622) #625

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extra parameter to start a container #616

Extra parameter to start a container #616

caguero commented Sep 27, 2024 •

edited

Loading

azeey left a comment

azeey Sep 30, 2024

caguero Oct 1, 2024

azeey commented Oct 3, 2024

caguero commented Oct 3, 2024

azeey commented Oct 3, 2024

caguero commented Oct 10, 2024 •

edited

Loading

ahcorde commented Oct 14, 2024

caguero commented Oct 15, 2024

mergify bot commented Oct 15, 2024 •

edited

Loading

Extra parameter to start a container #616

Extra parameter to start a container #616

Conversation

caguero commented Sep 27, 2024 • edited Loading

🦟 Bug fix

Summary

Checklist

🎉 New feature

Summary

Test it

Checklist

➡️ Forward port

🎈 Release

Checklist

azeey left a comment

Choose a reason for hiding this comment

azeey Sep 30, 2024

Choose a reason for hiding this comment

caguero Oct 1, 2024

Choose a reason for hiding this comment

azeey commented Oct 3, 2024

caguero commented Oct 3, 2024

azeey commented Oct 3, 2024

caguero commented Oct 10, 2024 • edited Loading

ahcorde commented Oct 14, 2024

caguero commented Oct 15, 2024

mergify bot commented Oct 15, 2024 • edited Loading

✅ Backports have been created

caguero commented Sep 27, 2024 •

edited

Loading

caguero commented Oct 10, 2024 •

edited

Loading

mergify bot commented Oct 15, 2024 •

edited

Loading