[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix no specified namespace #153

Merged
merged 5 commits into from
Jun 22, 2020
Merged

Fix no specified namespace #153

merged 5 commits into from
Jun 22, 2020

Conversation

ivanpauno
Copy link
Member

If the user doesn't explicitly pass a namespace to Node action neither uses PushRosNamespace, the node namespace can't be known at launch time neither the fully qualified node name:

launch_ros/launch_ros/launch_ros/actions/node.py

Lines 355 to 358 in 7974bd4

if self.__expanded_node_namespace != '':
cmd_extension = ['-r', LocalSubstitution("ros_specific_arguments['ns']")]
self.cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_extension])
validate_namespace(self.__expanded_node_namespace)

This PR fixes that incorrect assumption.

ivanpauno added 2 commits June 19, 2020 13:26
@ivanpauno
Mark namespace as not specified when it was not
c40c8ef 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Correct test expectations
3f94b0c 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the bug Something isn't working label Jun 19, 2020
@ivanpauno ivanpauno requested a review from hidmic June 19, 2020 16:30
@ivanpauno ivanpauno self-assigned this Jun 19, 2020
@ivanpauno ivanpauno mentioned this pull request Jun 19, 2020
Merged
@ivanpauno
Address peer review comments
3933e35 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
hidmic
hidmic reviewed Jun 22, 2020
View reviewed changes
launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
ivanpauno added 2 commits June 22, 2020 11:30
@ivanpauno
Address peer review comment
6538d10 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Extend node name tests
2e1f1ef 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno requested a review from hidmic June 22, 2020 16:47
@ivanpauno
Copy link
Member Author
  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit e8894ce into master Jun 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/fix-no-specified-namespace branch June 22, 2020 19:08
@ivanpauno ivanpauno mentioned this pull request Jun 23, 2020
Merged
sloretz added a commit to ros2/demos that referenced this pull request Jun 23, 2020
@sloretz
Specify LifecycleNode namespace
35f8259 
Fixes regression introduced by ros2/launch_ros#153 

Alternative to ros2/launch_ros#157
@sloretz sloretz mentioned this pull request Jun 23, 2020
Closed
sloretz added a commit to ros2/demos that referenced this pull request Jun 23, 2020
@sloretz
Specify LifecycleNode namespace
9d41526 
Fixes regression introduced by ros2/launch_ros#153

Alternative to ros2/launch_ros#157

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@ivanpauno
Copy link
Member Author

I'm not sure of how this can be backported.
It should be somehow combined with #157.

I will try to figure out if there's a no API breaking way of backporting the fix.

@jacobperron
Copy link
Member

@ivanpauno Friendly ping. Let me know if you still plan to look at backporting this change and #157. I think backporting those changes will make other backports easier to do (currently I get conflicts since they touch similar lines of code).

@ivanpauno
Copy link
Member Author

@ivanpauno Friendly ping. Let me know if you still plan to look at backporting this change and #157. I think backporting those changes will make other backports easier to do (currently I get conflicts since they touch similar lines of code).

It's hard to come up with a backport of this.
This is fixing a bug, but it's also slightly changing behavior in ways that can break existing use cases.

I would prefer not backporting this, except we get an explicit request on doing so.
In that case, I will try to come up with something.

jacobperron pushed a commit that referenced this pull request Sep 18, 2020
@ivanpauno @jacobperron
Fix no specified namespace (#153)
1819c25 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@jacobperron jacobperron mentioned this pull request Sep 18, 2020
Merged
jacobperron added a commit that referenced this pull request Oct 5, 2020
@jacobperron @ivanpauno
[foxy backport] Fix no specified namespace (#153, #157) (#179)
93be16c 
* Fix no specified namespace (#153)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Make namespace parameter mandatory in LifecycleNode constructor (#157)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Make namespace argument optional

This maintains backwards compatibility.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Remove test for construction without namespace

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Treat empty namespace as unset

This keeps the current behavior in Foxy.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Address feedback and fix bug in LifecycleNode

If no namespace (or empty string) is provided, assume global namespace.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno mentioned this pull request Oct 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic approved these changes

Assignees

@ivanpauno ivanpauno

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivanpauno @jacobperron @hidmic