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

Fix cluster connecting option bug #889

Merged

Conversation

@supercaracal
Copy link
Contributor

supercaracal commented Feb 1, 2020

This is related to #888.

I modified it as keeping node connecting option as Hash instead of URI.

@byroot
byroot approved these changes Feb 1, 2020
Copy link
Member

byroot left a comment

Two very minor comments, LGTM otherwise.

[Node.new(option.per_node_key, node_flags, option.use_replica?),
Slot.new(available_slots, node_flags, option.use_replica?)]
ensure
node.map(&:disconnect)
node&.map(&:disconnect)

This comment has been minimized.

Copy link
@byroot

byroot Feb 1, 2020

Member

Nitpick, but since the return values doesn't matter, .each would make sense here.

This comment has been minimized.

Copy link
@supercaracal

supercaracal Feb 1, 2020

Author Contributor

fixed 584a8c6

def add_common_node_option_if_needed(options, node_opts, key)
return options if options[key].nil? && node_opts.first[key].nil?

options[key] = options[key] || node_opts.first[key]

This comment has been minimized.

Copy link
@byroot

byroot Feb 1, 2020

Member

You can use ||= here.

This comment has been minimized.

Copy link
@supercaracal

supercaracal Feb 1, 2020

Author Contributor

fixed 584a8c6

@supercaracal
Copy link
Contributor Author

supercaracal commented Feb 1, 2020

NoMethodError: undefined method `compact'

oh... Ruby 2.3 is EOL but I will accommodate Hash#compact.

@byroot byroot merged commit ac988fa into redis:master Feb 1, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@byroot
Copy link
Member

byroot commented Feb 1, 2020

Thanks!

@supercaracal supercaracal deleted the supercaracal:fix-password-option-bug-for-cluster branch Feb 1, 2020
@supercaracal
Copy link
Contributor Author

supercaracal commented Feb 1, 2020

Thank you for your quick and polite reviewing.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.