-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue fix (456) for resolution of service account mapping (Translation of principals) #459
Conversation
WIP: Fix for Translation of principal failed error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your contribution, this is very much appreciated and honestly the only way to make this tool improve 😄 . I have left some small comments on the PR, let me know what do you think?
Looking forward to align and merge this in!
Thanks again!
src/main/java/com/purbon/kafka/topology/roles/CCloudAclsProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/com/purbon/kafka/topology/roles/CCloudAclsProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/com/purbon/kafka/topology/roles/PrincipalType.java
Outdated
Show resolved
Hide resolved
Thank you for your valuable comments @purbon , really appreciate your time |
LGTM, thanks for the contribution! |
…n of principals) (#459) * Fix for Translation of principal failed error * Round trip unit test * Using the PrincipalType enum * Tests for Group principal type * Renamed the variable * minor refactoring * refactoring, fixed test input string * minor refactoring * using @value for Principal * test for error on invalid principal type * test for error on malformed string for principal * minor refactoring to clarify formatting * minor refactoring to clarify formatting * Updated the test case * Moved the mapped-principal method to Principal class * Made the fields private * Refactoring to rename and move under model.users package * Renamed method and removed unnecessary variable Co-authored-by: Krishnan Mani <k.mani.1@elsevier.com> Co-authored-by: Krishnan Mani <krishnan-mani@users.noreply.github.com>
We couldn't find the existing testcase for CCloudAclsProvider and found it challenging to create one with CCloudApi not injected. So we have proposed a localised change and added test for the new code we introduced
We consider that the bug 456 is a show stopper for our use case where we expect to use the service account names (rather than numeric ids) and therefore we might require a new release/hot fix