Skip to content
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

Merged
merged 20 commits into from
Mar 21, 2022

Conversation

grampurohitp
Copy link
Contributor

  • Please check if the PR fulfills these requirements
  • The commit messages are descriptive
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • An issue has been created for the pull requests. Some issues might require previous discussion.

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

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  • Bug fix
  • What is the current behavior? (You can also link to an open issue here)
  • What is the new behavior (if this is a feature change)?
  • Julie is now able to resolve the service accounts specified by name in the descriptor to the corresponding numeric id, so that the ACLs are created as expected
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
  • Unlikely (this restores the ability of julie to function when the principal translation is enabled)
  • Other information:
    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

Sorry, something went wrong.

Copy link
Collaborator

@purbon purbon left a 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!

@grampurohitp
Copy link
Contributor Author

Thank you for your valuable comments @purbon , really appreciate your time

@purbon
Copy link
Collaborator

purbon commented Mar 21, 2022

LGTM, thanks for the contribution!

@purbon purbon merged commit 891cf49 into kafka-ops:master Mar 21, 2022
purbon pushed a commit that referenced this pull request Apr 6, 2022
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants