Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11657 )
Change subject: [sentry] Fill out more sentry client API ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/11657/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11657/3//COMMIT_MSG@9 PS3, Line 9: This commit adds more sentry client APIs, e.g list/grant sentry : privileges as well as grant roles to groups, for the upcomi > nit: please also add "add group" to this list Done http://gerrit.cloudera.org:8080/#/c/11657/1/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/11657/1/src/kudu/sentry/sentry_client-test.cc@161 PS1, Line 161: req.roleName = "viewer"; > Can you abstract this shared setup into the test class? It may require fid Done http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@85 PS3, Line 85: ASSERT_OK(sentry_client_->Stop()); : ASSERT_OK(sentry_->Stop()); > Does the order here matter? If so, I would've thought that we should stop t Done http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@98 PS3, Line 98: thrift::HaClient<SentryCl > Does this test not work with Kerberos enabled? Done http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@106 PS3, Line 106: lient_opts)); > nit: SentryClient* client Done http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@221 PS3, Line 221: > nit: typo Done http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@216 PS3, Line 216: : ::sentry::TSentryGroup group; : group.groupName = "user"; : set<::sentry::TSentryGroup> groups; : groups.insert(group); : : ::sentry::TAlterSentryRoleAddGroupsRequest group_request; : ::sentry::TAlterSentryRoleAddGroupsResponse group_response; : group_request.requestorUserName = "joe-interloper"; : group_request.roleName = "viewer"; : group_request.groups = groups; : : Status s = sentry_client_->AlterRoleAddGroups(group_request, &group_response); : ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); : : // Attempt to alter a non-exist role. : group_request.requestorUserName = "test-admin"; : s = sentry_client_->AlterRoleAddGroups(group_request, &group_response); : ASSERT_TRUE(s.IsNotFound()) << s.ToString(); : : // Alter role 'viewer' to add group 'user'. : ::sentry::TCreateSentryRoleRequest role_request; : role_request.requestorUserName = "test-admin"; : role_request.roleName = "viewer"; : ASSERT_OK(sentry_client_->CreateRole(role_request)); > nit: Should this be its own test case? It seems pretty disconnected from te Done http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@262 PS3, Line 262: role_request.requestorUserName = "test-admin"; > Not needed? Done -- To view, visit http://gerrit.cloudera.org:8080/11657 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34695cd4cc6723b70617164d58f8681cefd09ddd Gerrit-Change-Number: 11657 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 15 Oct 2018 23:32:40 +0000 Gerrit-HasComments: Yes
