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

Sweep cleanup in rosbag2 recorder CLI args verification code #1633

Open
wants to merge 6 commits into
base: rolling
Choose a base branch
from

Conversation

MichaelOrlov
Copy link
Contributor

Motivation:

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:

  • Remove outdated checks that impose limitations that only one option out of others can be used. Such as:
    • "Only one option out of --all, --all-services --services or --regex can be used"
    • "Only one option out of --all, --all-topics, --topics, --topic-types or --regex can be used"
    • "--exclude-regex argument requires either --all, --all-topics, --all-services or --regex"
    • and so on..
  • Move arguments checks to a separate method 'validate_parsed_arguments(self, args, uri)'
  • Fix the wrong limitation on arguments in the recorder CLI help section.
    • The --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.

@MichaelOrlov MichaelOrlov changed the title Cleanup in rosbag2 recorder CLI args verification code Sweep cleanup in rosbag2 recorder CLI args verification code May 3, 2024
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>
@MichaelOrlov MichaelOrlov force-pushed the morlov/cleanup_in_recorder_cli_args_verification_code branch from 98f1701 to cab378c Compare May 7, 2024 00:41
@MichaelOrlov MichaelOrlov marked this pull request as ready for review May 7, 2024 00:43
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner May 7, 2024 00:43
@MichaelOrlov MichaelOrlov requested review from emersonknapp, james-rms, fujitatomoya and Barry-Xu-2018 and removed request for a team and james-rms May 7, 2024 00:43
- 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>
ros2bag/ros2bag/verb/record.py Outdated Show resolved Hide resolved
ros2bag/ros2bag/verb/record.py Show resolved Hide resolved
ros2bag/ros2bag/verb/record.py Show resolved Hide resolved
ros2bag/ros2bag/verb/record.py Show resolved Hide resolved
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@Barry-Xu-2018
Copy link
Contributor

LGTM

@MichaelOrlov
Copy link
Contributor Author

@clalancette @emersonknapp This PR was reviewed and approved by @Barry-Xu-2018.
However, I need your formal review/approval to be able to merge this PR.

@MichaelOrlov
Copy link
Contributor Author

@clalancette @emersonknapp Friendly ping here for romal review/approval.
This PR blocks another PR #1646

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.

@MichaelOrlov lgtm with green CI!

@MichaelOrlov
Copy link
Contributor Author

Pulls: #1633
Gist: https://gist.githubusercontent.com/MichaelOrlov/5f0723928c4a6536b269e7fc767de58a/raw/83fc88eb59f337cf880618eeb9a8a56a73dc4b95/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13938

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

@MichaelOrlov
Copy link
Contributor Author

@clalancette @emersonknapp Since the Jazzy release is done, perhaps you will find time to formally approve this PR.
It was already reviewed by @fujitatomoya and @Barry-Xu-2018.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

3 participants