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.

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

Add warning and annotation to ros2 node list when there are non-unique nodes #462

Merged

Conversation

emersonknapp
Copy link
Contributor

@emersonknapp emersonknapp commented Feb 27, 2020

First half of #453
First half of https://github.com/ros-tooling/aws-roadmap/issues/196

Add a simple warning, and annotate which nodes it applies to, when ros2 node list lists multiple nodes with the same exact name. Within the "request for design" discussion ros2/design#187, there was confusion expressed about where multiple names come from, whether they are artifacts of the printing process. This at least clears that up a little bit -it does not change the functionality at all.

@emersonknapp emersonknapp force-pushed the emersonknapp/nodelist_annotate_duplicates branch from f11d96f to cd57c9c Compare February 27, 2020 20:56
@emersonknapp
Copy link
Contributor Author

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/emersonknapp/aff09b99d9b062699155ddb86a970b43/raw/8c63209a6cb44e48c7cc71f42e588a10ece4692d/ros2.repos
BUILD args: --packages-up-to ros2node
TEST args: --packages-select ros2node
Job: ci_launcher

@piraka9011
Copy link

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows-container Build Status

@emersonknapp
Copy link
Contributor Author

@mjcarroll perhaps, review?

ros2node/test/test_node.py Outdated Show resolved Hide resolved
@emersonknapp emersonknapp force-pushed the emersonknapp/nodelist_annotate_duplicates branch from cd57c9c to 75d398b Compare March 2, 2020 19:09
@emersonknapp
Copy link
Contributor Author

@chapulina who should we ask for OSRF-side review on this PR?

Copy link

@dabonnie dabonnie left a comment

Choose a reason for hiding this comment

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

LGTM

ros2node/test/test_node.py Outdated Show resolved Hide resolved
@thomas-moulard thomas-moulard mentioned this pull request Mar 4, 2020
3 tasks
@emersonknapp
Copy link
Contributor Author

@hidmic can you review this?

ros2node/ros2node/api/__init__.py Outdated Show resolved Hide resolved
ros2node/ros2node/api/__init__.py Outdated Show resolved Hide resolved
ros2node/ros2node/api/__init__.py Outdated Show resolved Hide resolved
ros2node/ros2node/verb/list.py Outdated Show resolved Hide resolved
@emersonknapp emersonknapp force-pushed the emersonknapp/nodelist_annotate_duplicates branch from 418ea4c to d26c103 Compare March 13, 2020 01:21
@emersonknapp
Copy link
Contributor Author

@wjwwood uploaded a simplified version of this PR

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/nodelist_annotate_duplicates branch from d26c103 to b66cf91 Compare March 13, 2020 17:16
@wjwwood
Copy link
Member

wjwwood commented Mar 13, 2020

I approved the changes, can one of you all do CI again (since there were new changes) and then I (or anyone really) can merge it.

@emersonknapp
Copy link
Contributor Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp
Copy link
Contributor Author

Hypothesizing it's flaky preexisting tests, retrying osx build

Build Status

@emersonknapp
Copy link
Contributor Author

Hypothesis confirmed, @wjwwood i think we're good to go?

@wjwwood wjwwood merged commit beadc01 into ros2:master Mar 17, 2020
@thomas-moulard thomas-moulard deleted the emersonknapp/nodelist_annotate_duplicates branch March 17, 2020 05:30
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.

None yet

5 participants