You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/01/11 02:03:04 UTC
[kudu-CR] [TLS cert management] security service units tests
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5675
Change subject: [TLS cert management] security service units tests
......................................................................
[TLS cert management] security service units tests
Added unit tests for the security service (Kudu TLS certificate
generation service).
Change-Id: I3ccabdd91db1e1fd114d6e6868dd9ef4137af9e9
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/test/security_service-test.cc
2 files changed, 283 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/5675/1
--
To view, visit http://gerrit.cloudera.org:8080/5675
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3ccabdd91db1e1fd114d6e6868dd9ef4137af9e9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [TLS cert management] security service units tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5675
to look at the new patch set (#2).
Change subject: [TLS cert management] security service units tests
......................................................................
[TLS cert management] security service units tests
Added unit tests for the security service (Kudu TLS certificate
generation service).
Change-Id: I3ccabdd91db1e1fd114d6e6868dd9ef4137af9e9
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/test/cert_signing_service-test.cc
2 files changed, 193 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/5675/2
--
To view, visit http://gerrit.cloudera.org:8080/5675
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3ccabdd91db1e1fd114d6e6868dd9ef4137af9e9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [TLS cert management] security service units tests
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [TLS cert management] security service units tests
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5675/1/src/kudu/security/test/security_service-test.cc
File src/kudu/security/test/security_service-test.cc:
Line 1: // Licensed to the Apache Software Foundation (ASF) under one
meta-note: generally not a fan of splitting patches into one for the code and one for the tests. I usually want to look at the tests as I review the code to understand how it's used, etc.
Line 81: mini_master_.reset(new MiniMaster(Env::Default(),
similar to comment in parent patch -- we don't want this upward dependency from security/ to master/, because now we have an indirect dependency of stuff like the rpc/ system all the way down to the master, which will be a big pain for folks like Impala who are importing our RPC.
Line 120: const char SecurityServiceTest::kCaCert_[] = R"***(
can we centralize these in security/test/test_certs.{h,cc}?
Line 218: for (size_t i = 0; i < arraysize(kFormats); ++i) {
can use a normal foreach?
--
To view, visit http://gerrit.cloudera.org:8080/5675
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3ccabdd91db1e1fd114d6e6868dd9ef4137af9e9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [TLS cert management] security service units tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [TLS cert management] security service units tests
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5675/1/src/kudu/security/test/security_service-test.cc
File src/kudu/security/test/security_service-test.cc:
Line 1: // Licensed to the Apache Software Foundation (ASF) under one
> meta-note: generally not a fan of splitting patches into one for the code a
Thank you for letting me know. I'll merge the test soon.
Line 81: mini_master_.reset(new MiniMaster(Env::Default(),
> similar to comment in parent patch -- we don't want this upward dependency
It's should be all right now: the cert-generation code is put into a separate security_ca lib (rpc does not depend on that), and this is just a test which depends both on security_ca and MiniMaster (which depends on master). Test are leaves in the sense of dependencies and this should be fine.
Let me know if I'm missing something in this context.
Line 120: const char SecurityServiceTest::kCaCert_[] = R"***(
> can we centralize these in security/test/test_certs.{h,cc}?
Good idea -- will do.
Line 218: for (size_t i = 0; i < arraysize(kFormats); ++i) {
> can use a normal foreach?
Good idea, but parameterized test seems to be even better.
--
To view, visit http://gerrit.cloudera.org:8080/5675
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3ccabdd91db1e1fd114d6e6868dd9ef4137af9e9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [TLS cert management] security service units tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change.
Change subject: [TLS cert management] security service units tests
......................................................................
Abandoned
Abandoned: the unit tests are merged into item 5674.
--
To view, visit http://gerrit.cloudera.org:8080/5675
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I3ccabdd91db1e1fd114d6e6868dd9ef4137af9e9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>