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

set service-url required to false, service-url is not required if ena… #9127

Merged
merged 2 commits into from
Jan 8, 2021

Conversation

dockerzhang
Copy link
Contributor

Fixes #9126

Master Issue: #

Motivation

there is --url required restriction for adding cluster if TLS is enabled, here we only need the --url-secure/--broker-url-secure params

Modifications

set service-url required to false, service-url is not required if enable TLS for pulsar cluster.

Verifying this change

  • Make sure that the change passes the CI checks.

@dockerzhang
Copy link
Contributor Author

/pulsarbot run-failure-checks

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.

please add unit tests, to demonstrate the commands that do not require this option and in order to prevent regressions.
Now that this option is no more required, what happens to the user if he does not provide this option in command that need it ? can you please also add test cases?

@sijie sijie added this to the 2.8.0 milestone Jan 6, 2021
@sijie sijie added the area/cli label Jan 6, 2021
@sijie
Copy link
Member

sijie commented Jan 6, 2021

@dockerzhang Can you rebase your pull request to the latest master to include the changes fixing the broken master?

@sijie
Copy link
Member

sijie commented Jan 8, 2021

/pulsarbot run-failure-checks

@sijie sijie merged commit 3e35af5 into apache:master Jan 8, 2021
@eolivelli
Copy link
Contributor

@dockerzhang

Now that this option is no more required, what happens to the user if he does not provide this option in command that need it ?

@sijie
Copy link
Member

sijie commented Jan 11, 2021

@eolivelli It is the same situation when you use TLS URLs but you don't specify TLS URLs in the cluster metadata. Geo replication will fail at that time.

@eolivelli
Copy link
Contributor

@sijie got it. Thanks for your clarification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

should not have --url required restriction for adding cluster if TLS is enabled
3 participants