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

Adding PulsarAdmin configs to PulsarHelper #158

Merged
merged 9 commits into from
Aug 31, 2023

Conversation

ericm-db
Copy link
Contributor

Motivation

In #151 we added admin.url without adding Admin Confs.

Modifications

Describe the modifications you've done.

Basically reverting #120

Verifying this change

  • [ x] Make sure that the change passes the CI checks.

(Please pick either of the following options)

  • [ x] This change is a trivial rework / code cleanup without any test coverage.

  • This change is already covered by existing tests, such as:

  • This change added tests and can be verified as follows:

Documentation

Check the box below.

Need to update docs?

  • doc-required
  • no-need-doc
  • doc

@ericm-db ericm-db requested review from nlu90 and a team as code owners August 29, 2023 00:56
@github-actions github-actions bot added the no-need-doc This pr does not need any document label Aug 29, 2023
@ericm-db
Copy link
Contributor Author

@chaoqin-li1123 For some reason I can't add you as a reviewer

@chaoqin-li1123
Copy link
Contributor

Code change LGTM, Thanks for the work!
Could you fix the scala style error?
Also please update the doc that admin config need to be provided for admission control to work.

@chaoqin-li1123
Copy link
Contributor

Can we add a small test for this change? Set one of the pulsar.admin.* option in the query, if this option is invalid, an exception should be thrown.

Copy link
Contributor

@chaoqin-li1123 chaoqin-li1123 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!

@nlu90 nlu90 merged commit 3f2e904 into streamnative:master Aug 31, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-need-doc This pr does not need any document
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants