-
Notifications
You must be signed in to change notification settings - Fork 146
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
Added case for instances of ExecuteLocal in resolveProcess function #587
Conversation
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>
@adityapande-1995 friendly ping |
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.
Could you please add a test case for this feature ?
Added case for handling ExecuteLocal actions in _proc_to_name_and_args in proc_lookup.py. Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>
Test cases added. I also had to modify a function in proc_lookup to handle both ExecuteProcess and ExecuteLocal objects. |
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.
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.
LGTM, unless @jacobperron disagrees.
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.
LGTM
The resolveProcess function needs to allow cases where the process is launched via instances of ExecuteLocal. This will likely be revisited when ExecuteRemote is added since we'll probably want to generalize the base class, but for now this should let the tests for ros2/launch_ros#215 pass so it can finally get merged.