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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
