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
Sweep cleanup in rosbag2 recorder CLI args verification code #1633
base: rolling
Are you sure you want to change the base?
Sweep cleanup in rosbag2 recorder CLI args verification code #1633
Conversation
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
- On Jazzy and Rolling we have relaxed limitations in topic filter, and it is ok to apply --exclude-topics above --topics or --exclude-services above --services. Also --all, --all-topics or --all-services overrides --regex. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
98f1701
to
cab378c
Compare
- Also adjust integration tests to check that --all, --all-topics and --all-services are allowed to be used together with --topics and --services CLI arguments. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
LGTM |
@clalancette @emersonknapp This PR was reviewed and approved by @Barry-Xu-2018. |
@clalancette @emersonknapp Friendly ping here for romal review/approval. |
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.
@MichaelOrlov lgtm with green CI!
Pulls: #1633 |
@clalancette @emersonknapp Since the Jazzy release is done, perhaps you will find time to formally approve this PR. |
--help
mentions that--regex
is applied on top of topic list, but doing so gives "Must specify only one option out of topics, --regex or --all" #1620Motivation:
We have reworked
rosbag2_transport ::topic_filter
in the scope of the Services recording/Replay feature and removed many constraints and limitations which was applied to the topic filter. However, the recorder CLI arguments verification code and help section hasn't been updated so far.List of changes:
--regex
now can be used together with--topics
and--services
lists. Also now --all, --all-topics or --all-services will override--regex
. Before, it was the opposite.--exclude-topics
can be used together with--topics
. Sort of nonsense but why not..--exclude--services
can be used together with--services
.Note: We already have test coverage for most (almost all) of the newly defined behavior in the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp
Note: There are no API/ABI breaking changes in this PR. However, it is not backportable to the Iron or Humble since we have different logic in the
rosbag2_transport ::topic_filter
.