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

[testclient] Replace '--auth_plugin' with '--auth-plugin' #12263

Merged
merged 1 commit into from
Oct 12, 2021
Merged

[testclient] Replace '--auth_plugin' with '--auth-plugin' #12263

merged 1 commit into from
Oct 12, 2021

Conversation

yuruguo
Copy link
Contributor

@yuruguo yuruguo commented Oct 2, 2021

Motivation

The name of the option should be linked with a horizontal line(-) instead of an underline(_) in CLI, so that it conforms to the unified standard.
Therefore, option --auth_plugin should be deprecated and replaced with --auth-plugin.

Modifications

  • Hidden --auth_plugin
  • --auth_plug takes effect when --auth-plugin is blank and --auth_plugin is not blank

Documentation

  • need to update docs

@yuruguo yuruguo changed the title Replace '--auth_plugin' with '--auth-plugin' [testclient] Replace '--auth_plugin' with '--auth-plugin' Oct 2, 2021
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Very good

But we should change that variable name

@@ -95,7 +95,10 @@
@Parameter(names = { "-t", "--num-topic" }, description = "Number of topics", validateWith = PositiveNumberParameterValidator.class)
public int numTopics = 1;

@Parameter(names = { "--auth_plugin" }, description = "Authentication plugin class name")
@Parameter(names = { "--auth_plugin" }, description = "Authentication plugin class name", hidden = true)
public String DEPRECATED_authPluginClassName;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is bad name for a Java field.

Please use Camel case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay :)

@yuruguo yuruguo requested a review from eolivelli October 5, 2021 09:48
Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

LGTM

@hangc0276 hangc0276 merged commit 794f054 into apache:master Oct 12, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
### Motivation
The name of the option should be linked with a horizontal line(-) instead of an underline(_) in CLI, so that it conforms to the unified standard.
Therefore, option --auth_plugin should be deprecated and replaced with --auth-plugin.

### Modifications
Hidden --auth_plugin
--auth_plug takes effect when --auth-plugin is blank and --auth_plugin is not blank
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