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.

Sign up for GitHub

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

Jump to bottom

Add enclaves introspection method in Node #538

Merged
merged 8 commits into from
Apr 13, 2020
Merged

Add enclaves introspection method in Node #538

merged 8 commits into from
Apr 13, 2020

Conversation

ivanpauno
Copy link
Member

Fixes #529.

With this, policy generation from a running example can be reactivated in sros2.

@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label Apr 9, 2020
@ivanpauno ivanpauno self-assigned this Apr 9, 2020
mikaelarguedas
mikaelarguedas reviewed Apr 9, 2020
View reviewed changes
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

this looks like a lot of copied code, would there be a way to reuse rather than duplicate ?

Code itself lgtm

rclpy/rclpy/node.py Outdated Show resolved Hide resolved
@ivanpauno ivanpauno mentioned this pull request Apr 10, 2020
Open
20 tasks
@ivanpauno
Copy link
Member Author

this looks like a lot of copied code, would there be a way to reuse rather than duplicate ?

See 7b4e246.
It would be easier if rcl API would only provide one function, and accept a nullptr in the other outputs. But, I don't want to create PRs in many layers.

@ivanpauno
Copy link
Member Author

ivanpauno commented Apr 10, 2020 •
edited
Loading

  • Linux Build Status (unrelated failures)
  • Linux-aarch64 Build Status (unrelated failures)
  • macOS Build Status (unrelated failures, was run after the block doc addition)
  • Windows Build Status (unrelated failures)

mikaelarguedas
mikaelarguedas approved these changes Apr 10, 2020
View reviewed changes
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

much nicer thanks 👍

One documentation nit otherwise LGTM with green CI

rclpy/src/rclpy/_rclpy.c Show resolved Hide resolved
mikaelarguedas
mikaelarguedas approved these changes Apr 10, 2020
View reviewed changes
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

LGTM

I would be great to update this to use "enclaves" and match ros2/rcl#612 so that there isn't a need for an immediate follow-up PR changing this new API

@ivanpauno
Copy link
Member Author

I would be great to update this to use "enclaves" and match ros2/rcl#612 so that there isn't a need for an immediate follow-up PR changing this new API

Sounds good, I will wait until the other group of PRs get merged, and then update.

hidmic
hidmic approved these changes Apr 10, 2020
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM

rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
ivanpauno and others added 7 commits April 13, 2020 11:18
@ivanpauno
Add security context introspection
198d5b5 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Correct documentation
372c187 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Improve code reuse
4bb11e0 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Add doc block
31ec2d9 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno @hidmic
nit
145324b 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-Authored-By: Michel Hidalgo <michel@ekumenlabs.com>
@ivanpauno
Address peer review comments
8ed2e2c 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
security_context ->enclave
94038e7 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/add-security-context-introspection branch from dd8b49e to 94038e7 Compare April 13, 2020 14:33
@ivanpauno ivanpauno changed the title Add security context introspection Add enclaves introspection method in Node Apr 13, 2020
hidmic
hidmic approved these changes Apr 13, 2020
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM

rclpy/rclpy/node.py Outdated Show resolved Hide resolved
@ivanpauno @hidmic
Fix nit in doc string
c496d51 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-Authored-By: Michel Hidalgo <michel@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

ivanpauno commented Apr 13, 2020 •
edited
Loading

  • Linux Build Status (unrelated build warning)
  • Linux-aarch64 Build Status (unrelated build warning)
  • macOS Build Status (unrelated build warning)
  • Windows Build Status (unrelated build warning + unrelated test failures)

@ivanpauno ivanpauno merged commit 75119f3 into master Apr 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/add-security-context-introspection branch April 13, 2020 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic approved these changes

@ruffsl ruffsl Awaiting requested review from ruffsl

@mikaelarguedas mikaelarguedas Awaiting requested review from mikaelarguedas

Assignees

@ivanpauno ivanpauno

Labels
in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a way to introspect security context names
3 participants
@ivanpauno @mikaelarguedas @hidmic