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

security-context -> enclave #198

Merged
merged 4 commits into from
Apr 13, 2020
Merged

security-context -> enclave #198

merged 4 commits into from
Apr 13, 2020

Conversation

mikaelarguedas
Copy link
Member

Signed-off-by: Mikael Arguedas mikael.arguedas@gmail.com

@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #198 into master will not change coverage by %.
The diff coverage is 80.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #198   +/-   ##
=======================================
  Coverage   60.00%   60.00%           
=======================================
  Files          16       16           
  Lines         565      565           
  Branches       52       52           
=======================================
  Hits          339      339           
  Misses        212      212           
  Partials       14       14           
Flag Coverage Δ
#unittests 60.00% <80.00%> (ø)
Impacted Files Coverage Δ
sros2/sros2/api/_artifact_generation.py 0.00% <0.00%> (ø)
sros2/sros2/verb/generate_artifacts.py 0.00% <ø> (ø)
sros2/sros2/api/_policy.py 85.71% <66.66%> (ø)
sros2/sros2/api/_key.py 90.14% <100.00%> (ø)
sros2/sros2/api/_keystore.py 84.72% <100.00%> (ø)
sros2/sros2/api/_permission.py 92.30% <100.00%> (ø)
sros2/sros2/verb/create_key.py 80.00% <100.00%> (ø)
sros2/sros2/verb/create_permission.py 68.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ea3858...b4d7709. Read the comment docs.

@ivanpauno ivanpauno mentioned this pull request Apr 10, 2020
20 tasks
@mikaelarguedas
Copy link
Member Author

CI for this and connected PRs:

ros2/rcl#612
ros2/rmw#211
ros2/rmw_connext#407
ros2/rmw_cyclonedds#146
ros2/rmw_dds_common#13
ros2/rmw_implementation#91
ros2/rmw_fastrtps#365
#198
ros2/system_tests#414

Repos file at https://gist.githubusercontent.com/mikaelarguedas/cd1009797d836e6d46a0f6d12b1cc05d/raw/2770591673f28d73d21ea5d432d939fee171e75d/enclaves.repos

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

@ivanpauno AFAICT all these test failures are also present in the nightly jobs. Please let me know if you know of any that may have been borken by this set of PRs

ivanpauno
ivanpauno previously approved these changes Apr 10, 2020
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

@ivanpauno
Copy link
Member

ivanpauno commented Apr 10, 2020

@ivanpauno AFAICT all these test failures are also present in the nightly jobs. Please let me know if you know of any that may have been borken by this set of PRs

Please, run windows job again only with one rmw implementation.
We have recently got rid of Poco and something seems to be broken now.

The other platforms looks ok.

@mikaelarguedas
Copy link
Member Author

Please, run windows job again only with one rmw implementation.
We have recently got rid of Poco and something seems to be broken now.

gotcha

Connext only, using updated rcl after resolving merge conflicts

  • Windows Build Status

ruffsl
ruffsl previously requested changes Apr 10, 2020
Copy link
Member

@ruffsl ruffsl left a comment

Choose a reason for hiding this comment

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

Check for all case and plur varenents like Contexts -> Enclaves

sros2/sros2/policy/schemas/policy.xsd Outdated Show resolved Hide resolved
sros2/sros2/policy/schemas/policy.xsd Outdated Show resolved Hide resolved
Copy link
Member

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

Mostly good, although found one issue with the sedding.

kyrofa
kyrofa previously approved these changes Apr 10, 2020
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas
Copy link
Member Author

mikaelarguedas commented Apr 10, 2020

Another round of CI after rebasing all PRs
CI only with connext to avoid Windows build failure
Build and testing only rcl and sros2

  • Linux Build Status (cmake warning unrelated)
  • macOS Build Status (cmake warning unrelated)
  • Windows Build Status
  • Windows existing failures on master (without these PRs): Build Status

@mikaelarguedas
Copy link
Member Author

Looks like windows rcl is broken... seems unrelated to this set of PRs though.
rcl master build to compare: Build Status
@ivanpauno can we go ahead and merge all these renaming PRs? or do you want to wait for the windows issue on master to be fixed first?

@mikaelarguedas mikaelarguedas dismissed ruffsl’s stale review April 10, 2020 23:24

comments addressed

@ruffsl
Copy link
Member

ruffsl commented Apr 11, 2020

Just skimming the test results from Build Status , it seems that test_rcl_get_node_names_with_security_contexts failed, but wasn't that renamed using enclave here:
https://github.com/ros2/rcl/pull/612/files#diff-6dd792d0229de5c8296858ba7649a6d3R168

Is the job using all the latest commits in the PR?

@mikaelarguedas
Copy link
Member Author

mikaelarguedas commented Apr 11, 2020

That job is running on master to show that all rcl tests on master are broken. It doesn't include any change from these PRs.

Updated #198 (comment) to make it clearer

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

CI looks good.
If I'm not missing something, everything is being renamed well.

@ivanpauno ivanpauno merged commit ebc1885 into ros2:master Apr 13, 2020
@mikaelarguedas mikaelarguedas deleted the enclaves branch April 13, 2020 14:20
@mikaelarguedas mikaelarguedas mentioned this pull request Jun 5, 2020
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.

4 participants