Add the ability to record any key/value pair in the 'custom' field in metadata.yaml

[t0ny-peng]

Usage: ros2 bag record --all --custom-data key1=value1 key2=value2. The custom data will show up in the metadata.yaml file:

rosbag2_bagfile_information:
  ...
  custom:
    key1: value1
    key2: value2

Closes #547

[MichaelOrlov]

@t0ny-peng Thank you for contribution. Overall looks good.
I have some minor findings. Please see them in inline comments.

I also have suggestion to rename node["custom"] to the node["custom_data"].
name custom is to generic and could confuse to what it relates for.

[MichaelOrlov]

@t0ny-peng Thank you for the iteration in code review.
I have a few more requests for changes.
Also could you please next time push new changes as a separate commit?
One commit per one review iteration should be fine.
It's very difficult to review code outside of the Github e.g. in CLion, when all changes squashed in one commit and pushed with force command. i.e. there are no history for the changes outside of the PR.

Multiple commits will not clutter the final git history since we have mandatory squash for commits when we do merge on master. Although it's really helpful for code review.

[t0ny-peng]

@MichaelOrlov I've addressed your comments. I didn't split the first commit because it seems pretty atomic. The changes are all related and cannot be broken down to subcomponents. However moving the convert structs has its own commit. Please review. Thx

[MichaelOrlov]

@t0ny-peng Thanks, approving now.

As regards to the

I didn't split the first commit because it seems pretty atomic. The changes are all related and cannot be broken down to subcomponents. However moving the convert structs has its own commit.

It's ok and this is exactly how I expected it to be.

[MichaelOrlov]

Running CI
Gist: https://gist.githubusercontent.com/MichaelOrlov/f295a4dda3d8ce90076def0a0646c519/raw/620153ccb8176c963f45162d6b39e1903ee19bf2/ros2.repos
BUILD args: --packages-up-to ros2bag rosbag2_cpp rosbag2_storage rosbag2_tests
TEST args: --packages-select ros2bag rosbag2_cpp rosbag2_storage rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10087/

• Linux
• Linux-aarch64
• Windows

[t0ny-peng]

@MichaelOrlov I'm not sure if increasing the version is the correct move here. rosbag2_tests has some database files that store version 5 at them, and the test failed because of version 6(from the code) and version 5(from the database file).

In this case, what's the correct way of updating the database? Using sqlite to modify it manually?

[MichaelOrlov]

@t0ny-peng I think we need to fix tests by updating databases.
We need to change the metadata version number each time as we are changing something in the BagMetadata staruct.
https://github.com/ros2/rosbag2/blob/master/rosbag2_storage/include/rosbag2_storage/bag_metadata.hpp#L44

Need to look to the failing tests, may be there are some routines for generating test database.

BTW I see that tests fails not only in rosbag2_tests also in rosbag2_cpp

[MichaelOrlov]

@t0ny-peng Update: Don't need to change database files.
I was able to fix failing tests from rosbag2_tests altering version number in https://github.com/t0ny-peng/rosbag2/blob/master/rosbag2_tests/resources/reindex_test_bags/target_metadata/multiple_files/metadata.yaml#L2 and adding new section for the custom_data at the end.

However I was not able to reproduce failures in rosbag2_cpp
with message:

C++ exception with description "Exception on parsing info file: invalid node; first invalid key: "custom_data"" thrown in the test body.

[t0ny-peng]

@MichaelOrlov I think the error in rosbag2_cpp has already been fixed. Could you please try the latest commit(aae2fc7)?

WRT changing the version in the test, I don't think comparing the current version of metadata CPP struct with the version in the test yaml file is the best way. It means the YAML file has to be updated every time the CPP metadata version increases. However for now I'll just do that as a one time patch.

[t0ny-peng]

@MichaelOrlov I think the error in rosbag2_cpp has already been fixed. Could you please try the latest commit(aae2fc7)?

WRT changing the version in the test, I don't think comparing the current version of metadata CPP struct with the version in the test yaml file is the best way. It means the YAML file has to be updated every time the CPP metadata version increases. However for now I'll just do that as a one time patch.

[MichaelOrlov]

Running CI again after fix in rosbag2_tests
Gist: https://gist.githubusercontent.com/MichaelOrlov/f295a4dda3d8ce90076def0a0646c519/raw/620153ccb8176c963f45162d6b39e1903ee19bf2/ros2.repos
BUILD args: --packages-up-to ros2bag rosbag2_cpp rosbag2_storage rosbag2_tests rosbag2_transport
TEST args: --packages-select ros2bag rosbag2_cpp rosbag2_storage rosbag2_tests rosbag2_transport
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10089/

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

Windows build unstable on baseline i.e. has some CMake warnings.
Merging.