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

Updated rootpath validation method in zookeeper coordinator #438

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cluyihunter
Copy link

(Previous pull request closed due to messy commits)
Validate rootpath with Exists instead of Create to avoid 'zk: not authenticated' error.
Solved the problem on Issue #347 if rootpath already exists on a pre-configured zookeeper server.

yumHunter added 3 commits August 3, 2018 09:25
Validate rootpath with Exists instead of Create to avoid 'zk: not authenticated' error

Fixed bug: mock Exists

Fixed bug: zookeeper exists err

Add zookeeper coordinator test on Exists method

Fixed bug: add zookeeper coordinator test on Exists method

Fixed bug: Exists return err message

Update zookeeper coordinator test - Exists return type

Update zookeeper coordinator test - Exists & Create

Fixed zookeeper coordinator test - Exists

Fixed protocol.go formatting issue: LF will be relaced by CRLF

gofmt fix on changed files

Modify coordinator to increase test coverage

Modify coordinator to fix build error

Modify errExists

Modify order of method calls in zk coordinator test

Bug fix: zk coordinator test last Exists call return &zk.Stat{}

Remove return errExist to improve test coverage

Add test cases to helpers - validation_test

Allow blank hostname in TestValidateHostList

Allow blank hostname in ValidateHostPort

Fix broken zk coordinator and allow space in consumer name
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.009%) to 73.976% when pulling 309b3ee on cluyihunter:master into 12e681a on linkedin:master.

@toddpalino
Copy link
Contributor

This change would be fine. However, your PR contains a lot of unrelated changes. Please fix the PR to contain only the changes/tests that are appropriate.

@tobiade
Copy link

tobiade commented Aug 19, 2019

Hey @cluyihunter any chance you're going to clean this up? :)
Running into a similar problem this PR should fix.

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.

4 participants