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

Create new rmw_security_common #330

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

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jan 24, 2025

the new rmw_zenoh_cpp has an open PR to include certificates using the --enclave argument. This requires to use rmw_dds_common::get_security_files which is not a dds unique utility.

The idea with new package is to include here all the security common things.

Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.94%. Comparing base (5968e6b) to head (5300b83).

Additional details and impacted files
@@           Coverage Diff            @@
##           rolling     #330   +/-   ##
========================================
  Coverage    88.94%   88.94%           
========================================
  Files           24       24           
  Lines          615      615           
  Branches        64       64           
========================================
  Hits           547      547           
  Misses          50       50           
  Partials        18       18           
Flag Coverage Δ
unittests 88.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@clalancette
Copy link
Contributor

I know I suggested this, but I am slightly concerned here about the fact that this package is very low-level (e.g. used by RMW implementations), while the rest of the packages here are very high-level (e.g. Python and CMake packages). I discussed this a bit with @mjcarroll , and we thought it might make more sense to actually move this into https://github.com/ros2/rmw . However, as part of that it might actually also make sense to reimplement it as a pure-C header, rather than C++ (so it can be wrapped).

I'm going to put this on the agenda for the ROS 2 weekly meeting next week so we can discuss it a bit more.

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.

we thought it might make more sense to actually move this into https://github.com/ros2/rmw

i agree with this 👍

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.

3 participants