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

[pkg/kafka] move internal/kafka to pkg/kafka #30406

Closed
wants to merge 11 commits into from

Conversation

vincentfree
Copy link
Contributor

Description:

Move internal/kafka to pkg/kafka so that the Authorization configuration can be exported for downstream use.

Link to tracking Issue:
#30377

Testing:
Ensured that all modules which have been touched are still working. Since no extra code has been introduced, no extra tests need to be included.

Documentation:

This was mainly a move of files, no new functionality or documentation

[pkg/kafka] move internal/kafka to pkg/kafka
@vincentfree
Copy link
Contributor Author

@dmitryax as promised :)
I have validated that it works for all involved modules but the code coverage failed on my branch probably due to missing configuration for the CI configuration, I expect everything is in order for this PR

@atoulme
Copy link
Contributor

atoulme commented Jan 10, 2024

Please resolve conflicts.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! Please rebase

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Why exposing all the internal package when only config is needed. There are lots of public types in this package which I am not sure we want to expose publicly if only the config is needed per the issue.

@dmitryax
Copy link
Member

Why exposing all the internal package when only config is needed. There are lots of public types in this package which I am not sure we want to expose publicly if only the config is needed per the issue.

Right. That was the original ask actually. I see IAMSASLClient XDGSCRAMClient unnecessary exposed along with ConfigureAuthentication. @vincentfree can you please keep them in the internal module?

@vincentfree
Copy link
Contributor Author

Why exposing all the internal package when only config is needed. There are lots of public types in this package which I am not sure we want to expose publicly if only the config is needed per the issue.

Right. That was the original ask actually. I see IAMSASLClient XDGSCRAMClient unnecessary exposed along with ConfigureAuthentication. @vincentfree can you please keep them in the internal module?

Yea I can move them back

@vincentfree vincentfree requested a review from dmitryax February 5, 2024 11:20
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Feb 22, 2024
Copy link
Contributor

github-actions bot commented Mar 8, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants