Skip to content

Conversation

SuperJappie08
Copy link
Contributor

Original committer: @esteve
Original author: @kenji-miyake
Original reviewer: @ivanpauno

Description

Added the requested tests to #327, also exposed autostart.

Fixes #236

Is this user-facing behavior change?

Did you use Generative AI?

No

Additional Information

Rebased original pull request on rolling.

Kenji Miyake and others added 2 commits August 23, 2025 17:12
@SuperJappie08
Copy link
Contributor Author

@fujitatomoya, this is the updated version of #327, which you closed recently.
Would you have time to review it?

Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

i think this is lgtm.

@christophebedard @ahcorde i would like to have 2nd review on this. can you take a look when you have time?

@fujitatomoya
Copy link
Contributor

fujitatomoya commented Oct 20, 2025

Pulls: #482
Gist: https://gist.githubusercontent.com/fujitatomoya/dc3ac29ce2fd7482d8091acdfd376795/raw/fc06bb5c62c0819740751c82b217c78322a2ce74/ros2.repos
BUILD args: --packages-above-and-dependencies launch_ros test_launch_ros
TEST args: --packages-above launch_ros test_launch_ros
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17317

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@SuperJappie08
Copy link
Contributor Author

@fujitatomoya, I think the CI failed on cloning the repos.
Would it benefit from retrying?

@christophebedard christophebedard self-requested a review October 22, 2025 16:36
@christophebedard
Copy link
Member

I'll give this a quick review and take care of CI later today.

@christophebedard
Copy link
Member

Just to note this here:

#236 mentions supporting event handlers (i.e., the RegisterEventHandler action) in frontends. As explained in #327 (review), without this, there isn't much advantage to exposing LifecycleNode to frontends. However, as mentioned in that issue, it's still a good first step. Also, it looks like we have ros2/launch#658 for exposing RegisterEventHandler to frontends, so I think it's fine to close #236.

Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Looks good to me! We just need to wait for the CI jobs I retriggered to finish; CI queues are a bit full right now.

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.

Support LifecycleNode and EventHandler in frontend

3 participants