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 issue; node roles are defaulting to true when undefined (false) breaking usage of nodeFilter option #967

Merged
merged 2 commits into from
Sep 18, 2019

Conversation

deuscapturus
Copy link
Contributor

nodeFilter option was unable to filter nodes where master, data or ingest roles were false because they were defaulting to true when not set.

The example below demonstrates the problem:

// roles are returned from the elastic API as a list
roleslist = ['master', 'ingest']
console.log(roleslist)

// The client converts the list to a dictionary
const roles = roleslist.reduce((acc,role) => { acc[role] = true; return acc}, {})
console.log(roles)

// The client then assigns default values to undefined roles
console.log(
        Object.assign({'data': true, 'master': true, 'injest': true, 'ml': false}, roles)
)

Output:

[ 'master', 'ingest' ]
{ master: true, ingest: true }
{ data: true, master: true, injest: true, ml: false, ingest: true }

data is true when it should be false.

…ingest role were true if even they were false on the node.
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for the PR!
You are right, this is a bug.
Can you also add a unit test?

@deuscapturus
Copy link
Contributor Author

tests added

@deuscapturus
Copy link
Contributor Author

oops. I hijacked another test. Let me try something different...

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

@delvedor
Copy link
Member

jenkins run the tests please

@delvedor delvedor merged commit 7fef37c into elastic:master Sep 18, 2019
delvedor pushed a commit that referenced this pull request Sep 18, 2019
* Fix issue; nodeFilter was unable to filter because master, data, and ingest role were true if even they were false on the node.

* Test nodesToHost of BaseConnectionPool correctly maps node roles
delvedor pushed a commit that referenced this pull request Sep 23, 2019
* Fix issue; nodeFilter was unable to filter because master, data, and ingest role were true if even they were false on the node.

* Test nodesToHost of BaseConnectionPool correctly maps node roles
delvedor added a commit that referenced this pull request Oct 2, 2019
* Update code generation (#969)

* Updated code generation scripts to use the new spec

* API generation

* Fix bad link

* Updated API reference doc (#945)

* Updated API reference doc

* Updated docs script

* Fix issue; node roles are defaulting to true when undefined (fal… (#967)

* Fix issue; nodeFilter was unable to filter because master, data, and ingest role were true if even they were false on the node.

* Test nodesToHost of BaseConnectionPool correctly maps node roles

* API generation

* Docker: use 7.4-SNAPSHOT

* API generation

* Use 7.4 stable
@niksrc
Copy link

niksrc commented Oct 16, 2019

@delvedor any reason this change is not released for @elastic/elasticsearch@5 yet?

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.

3 participants