[User management] Add CreateUserResponse and GetUserResponse gRPC response wrappers [DPP-854] #12682
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pbatko-da
requested review from
meiersi-da,
stefanobaghino-da and
a team
as code owners
kamil-da
approved these changes
Jan 31, 2022
pbatko-da
changed the title
Comment on lines
-219
to
+223
| assertEquals(res1, user1) | ||
| assertEquals(res1, CreateUserResponse(Some(user1))) | ||
| assertUserAlreadyExists(res2) | ||
| assertEquals(res3, user2) | ||
| assertEquals(res3, CreateUserResponse(Some(user2))) |
There was a problem hiding this comment.
Could these changes require to add some exclusions to compatibility tests?
There was a problem hiding this comment.
Added Breaks-protobuf: true to the commit message as per ci/check-protobuf-stability.sh
Comment on lines
36
to
+39
| StubHelper.authenticating(this.serviceFutureStub, maybeToken) | ||
| .createUser(request.toProto()), | ||
| sequencerFactory) | ||
| .map(User::fromProto); | ||
| .map(res -> User.fromProto(res.getUser())); |
There was a problem hiding this comment.
A wrapper type for the Protobuf should be added and that should be returned, rather than keeping the User type. Otherwise we lose the advantage of being able to evolve the API over time and this is especially important for the Java bindings, which are officially supported for external customers. I'm ok merging this as is and doing the work myself, just let me know what's your preference.
There was a problem hiding this comment.
Would be great if you could handle it, thank you!
There was a problem hiding this comment.
Ok, I'll keep an eye on this and work on it once it's done.
pbatko-da
force-pushed
the
pbatko/persistent-user-management-grpc-wrapper
branch
from
4af3fab to
83573a4
Compare
mziolekda
reviewed
Feb 3, 2022
ledger-api/grpc-definitions/com/daml/ledger/api/v1/admin/user_management_service.proto
Show resolved
Hide resolved
mziolekda
approved these changes
Feb 3, 2022
|
/azp rerun |
|
Command 'rerun' is not supported by Azure Pipelines. Supported commands
See additional documentation. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
meiersi-da
approved these changes
Feb 7, 2022
…ponse wrappers [DPP-854] changelog_begin Ledger API Specification: CreateUser and GetUser endpoints of UserManagementService now return CreateUserResponse and GetUserResponse rather than User message. changelog_end Breaks-protobuf: true
stefanobaghino-da
force-pushed
the
pbatko/persistent-user-management-grpc-wrapper
branch
from
3f374fd to
3157413
Compare
Merged
ghost
mentioned this pull request
7 tasks
7 tasks
garyverhaegen-da
added a commit
that referenced
this pull request
Feb 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.