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

[KIP-554] User SCRAM credentials API #1575

Merged
merged 36 commits into from
Jul 11, 2023
Merged

Conversation

mahajanadhitya
Copy link
Contributor

No description provided.

@mahajanadhitya mahajanadhitya requested a review from a team as a code owner May 30, 2023 06:33
@emasab emasab changed the title Feature/admin client scram api [KIP-554] User SCRAM credentials API Jul 4, 2023
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through _scram.py file, examples folder and docs folder only. Going through other files.

examples/adminapi.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/_scram.py Show resolved Hide resolved
src/confluent_kafka/admin/_scram.py Outdated Show resolved Hide resolved
self.scram_credential_infos = scram_credential_infos


class UserScramCredentialAlteration:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mark this class as Abstract.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subclassing ABC doesn't really make it non-instantiable if there isn't an abstract method.
It this case it should be more like a Kotlin sealed class, that cannot be subclassed outside the package,
but it's not possible in Python.

src/confluent_kafka/admin/_scram.py Outdated Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Show resolved Hide resolved
examples/adminapi.py Show resolved Hide resolved
examples/adminapi.py Outdated Show resolved Hide resolved
examples/adminapi.py Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ongoing C files review.

src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking response

src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Check all the local variables in the Response parsing for Ref Count.
  2. Don't nest multiple functions. create local variables for readability.

src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Show resolved Hide resolved
tests/test_Admin.py Outdated Show resolved Hide resolved
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there. Some nits. Nothing major.

CHANGELOG.md Show resolved Hide resolved
examples/adminapi.py Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Outdated Show resolved Hide resolved
src/confluent_kafka/admin/__init__.py Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Outdated Show resolved Hide resolved
src/confluent_kafka/src/Admin.c Show resolved Hide resolved
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work Guys!.

LGTM! :)

@emasab emasab merged commit 96d48e2 into master Jul 11, 2023
@emasab emasab deleted the feature/AdminClient-ScramAPI branch July 11, 2023 12:22
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.

3 participants