Skip to content

Fix AttributeError for Elasticsearch>2.0.0,<3.0.0#197

Open
artemknd wants to merge 1 commit into
aio-libs:masterfrom
artemknd:patch-1
Open

Fix AttributeError for Elasticsearch>2.0.0,<3.0.0#197
artemknd wants to merge 1 commit into
aio-libs:masterfrom
artemknd:patch-1

Conversation

@artemknd

Copy link
Copy Markdown

AIOHttpConnection takes use_ssl argument and passes one to the parent's init.

Then self.use_ssl gets used instead of use_ssl here
https://github.com/aio-libs/aioelasticsearch/blob/master/aioelasticsearch/connection.py#L52

It works for the latest elasticsearch-py versions, however, it doesn't work with >2.0.0, b/c there's no self.use_ssl = use_ssl there.

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

AIOHttpConnection takes use_ssl argument and passes one to the parent's __init__.

Then self.use_ssl gets used instead of use_ssl here
https://github.com/aio-libs/aioelasticsearch/blob/master/aioelasticsearch/connection.py#L52

It works for the latest elasticsearch-py versions, however, it doesn't work with >2.0.0, b/c there's no self.use_ssl = use_ssl there.
@codecov

codecov Bot commented Nov 11, 2019

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 0.23%. Comparing base (26fcd7c) to head (baa68ef).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
aioelasticsearch/connection.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master    #197   +/-   ##
======================================
  Coverage    0.23%   0.23%           
======================================
  Files           6       6           
  Lines         433     433           
  Branches       77      77           
======================================
  Hits            1       1           
  Misses        432     432           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@asvetlov

Copy link
Copy Markdown
Member

Is elasticsearch 2.x supported still?

@artemknd

artemknd commented Nov 12, 2019

Copy link
Copy Markdown
Author

@asvetlov I tested out search and scan aioelasticsearch examples.
They both work for ES 2.x with my tiny fix.

As for elasticsearch-py, 2.X is supported as well
https://github.com/elastic/elasticsearch-py#compatibility

@asvetlov

Copy link
Copy Markdown
Member

Manual testing is not enough; the library should be tested by travis-ci against all supported ES versions.
ES_TAG env var already exists in .travis.yml config file, I'm ok with adding the travis matrix for testing several versions.
Sorry, I have no time/motivation for making the fix myself but happy to review a PR from a champion.
Please note: extending the matrix requires a separate PR.

@artemknd

artemknd commented Nov 14, 2019

Copy link
Copy Markdown
Author

@asvetlov did you have a chance to take a look at the code itself?

The update is very straightforward. I just substituted referred parent class's Connection property self.use_ssl with constructor's argument use_ssl directly.

I'd even say it's quite reasonable and makes more sense, b/c there's no dependency on the underlying parent's implementation.

Current tests are all good. So, I believe it's safe to merge one.
Could you take a look at the code diff and merge it, please?

@asvetlov

Copy link
Copy Markdown
Member

Please don't get me wrong.
You are asking for changing the code that works fine already according to the current test suite.
There is no reason to do it, even if the change is barely minimal.
My motivation for hesitation is a chance to break something by accident. Don't fix if not broken, test the fix of broken things.

You said that the PR fixes ES 2.x, that's awesome!
What I'm asking is adding a test to check this (and to make sure that the lastest aioelasticsearch works with ES 2.x at all). It can be done easily by extending travis config matrix.
Could you update it?

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.

2 participants