KIKIMR-20079: create/alter/drop group #501
Conversation
|
Note This is an automated comment that will be appended during run. 🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 21f0971.
🔴 linux-x86_64-release-asan: some tests FAILED for commit 21f0971.
|
| return MakeFuture(ResultFromError<TGenericResult>("Couldn't get domain name")); | ||
| } | ||
|
|
||
| NKikimrSchemeOp::TModifyScheme schemeTx; |
There was a problem hiding this comment.
With these changes we get a new code that does the same things as in kqp_ic_gateway.cpp. Can we make some library with helpers that fill TModifyScheme proto and is used by both gateways?
There was a problem hiding this comment.
It is not a strict requirement. I think we need to see if it will bring us a profit. Maybe in this particular cases protos are very simple. If so, maybe it would be better to leave this code in both gateways.
| if (!response.GetIssues().empty()) { | ||
| NYql::TIssues issues; | ||
| NYql::IssuesFromMessage(response.GetIssues(), issues); | ||
| Promise.SetValue(NYql::NCommon::ResultFromIssues<TResult>(NYql::TIssuesIds::SUCCESS, "", issues)); |
There was a problem hiding this comment.
Should you put SUCCESS status here or maybe infer it from response.GetIssues()?
There was a problem hiding this comment.
Condition in case is already a status of response
This changes in ExecComplete status, so it means that result is success
| renameGroup.SetNewName(settings.NewName); | ||
|
|
||
| SendSchemeRequest(ev.Release()).Apply( | ||
| [renameGroupPromise](const TFuture<TGenericResult>& future) mutable { |
There was a problem hiding this comment.
maybe [&renameGroupPromise]
There was a problem hiding this comment.
renameGroupPromise is local variable. So it will be error to capture it by reference. Promises have reference counting, it is safe to copy renameGroupPromise into lambda.
| @@ -111,13 +114,39 @@ class TAlterLogin: public TSubOperationBase { | |||
| } else { | |||
| db.Table<Schema::LoginSidMembers>().Key(removeGroupMembership.GetGroup(), removeGroupMembership.GetMember()).Delete(); | |||
| result->SetStatus(NKikimrScheme::StatusSuccess); | |||
| if (response.Warning) { | |||
There was a problem hiding this comment.
AddGroup has if Notice, RemoveGroup has if Warning. Maybe there should be the same condition?
There was a problem hiding this comment.
I just made it like in pg, but yeah, maybe it is better to make it with the same levels
| } | ||
|
|
||
| if (!settings.Roles.size()) { | ||
| return MakeFuture(ResultFromError<TGenericResult>("No roles given for AlterGroup request")); |
There was a problem hiding this comment.
I can't see the same check for CreateGroup. I think they should be equivalent in this case
There was a problem hiding this comment.
CreateGroup can be without any users, but before calling AddUsersToGroup I check that settings.Roles.size() is not 0
https://github.com/ydb-platform/ydb/pull/501/files#diff-fb95cf18d73802eec5aabc3807b82c4e98f0b3cfae62523917dab93336b0c5ebR933
* KIKIMR-20079: create/alter/drop group * fix test * fix mistake * resolve issues
No description provided.