Pylint integration updates

opensearch-project / opensearch-py

Python Client for OpenSearch

https://opensearch.org/docs/latest/clients/python/

Apache License 2.0

338 stars 170 forks source link

Pylint integration updates #643

Closed macohen closed 8 months ago

macohen commented 8 months ago

Description

extracted a method for per-folder linting
created a feature flag for newer dynamic pylint so now lints can be fixed first locally and then enabled without forcing build failures
ignore opensearchpy for pylint
updated noxfile.lint_per_folder with type hints
enabled unspecified-encoding in pylint
added disable missing-function-docstring pragma to test_clients.py in test_async and test_server

Issues Resolved

Progress on #610

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (3eba72c) 72.10% compared to head (107469c) 72.04%. Report is 2 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #643 +/- ## ========================================== - Coverage 72.10% 72.04% -0.07% ========================================== Files 89 89 Lines 7949 7945 -4 ========================================== - Hits 5732 5724 -8 - Misses 2217 2221 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

saimedhi commented 8 months ago

Thank you, @macohen , for your contribution! I'll thoroughly review the PR today and aim to merge it soon. I've observed a failing integration test, so I'm rerunning it to determine if it's a flaky test.

macohen commented 8 months ago

Thank you, @macohen , for your contribution! I'll thoroughly review the PR today and aim to merge it soon. I've observed a failing integration test, so I'm rerunning it to determine if it's a flaky test.

Thanks @saimedhi. That's the test against main?

FAILED test_opensearchpy/test_server/test_rest_api_spec.py::test_rest_api_spec[OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/indices/stats/50_noop_update[0]] - opensearchpy.exceptions.ConnectionTimeout: ConnectionTimeout caused by - ReadTimeoutError(HTTPConnectionPool(host='localhost', port=9200): Read timed out. (read timeout=3))

I'm not sure I've ever seen that test succeed in this PR.

saimedhi commented 8 months ago

Thank you, @macohen , for your contribution! I'll thoroughly review the PR today and aim to merge it soon. I've observed a failing integration test, so I'm rerunning it to determine if it's a flaky test.

Thanks @saimedhi. That's the test against main?

FAILED test_opensearchpy/test_server/test_rest_api_spec.py::test_rest_api_spec[OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/indices/stats/50_noop_update[0]] - opensearchpy.exceptions.ConnectionTimeout: ConnectionTimeout caused by - ReadTimeoutError(HTTPConnectionPool(host='localhost', port=9200): Read timed out. (read timeout=3))

I'm not sure I've ever seen that test succeed in this PR.

Yes, it is test against main or unreleased OpenSearch. I will check, if the failing test is related to this PR or not.

saimedhi commented 8 months ago

Hi @macohen, in your latest commit, could you please remove the changes causing test failures? If not, consider excluding that commit and let's proceed with merging this code. We can address the other changes in a follow-up PR. Apologies for the delay.

macohen commented 8 months ago

Hi @macohen, in your latest commit, could you please remove the changes causing test failures? If not, consider excluding that commit and let's proceed with merging this code. We can address the other changes in a follow-up PR. Apologies for the delay.

Thanks. I was working on figuring out how to run the two tasks that are failing locally so I can reproduce the failures. I thought I needed to check out the appropriate branch of opensearch (2.0, main) and run the commands in the workflow, but I haven't been able to reproduce. If you have any guidance there, that would be helpful. Also, I will update this comment in about an hour with the command I'm running...

macohen commented 8 months ago

I opened #651 with a trivial change and it failed in these integration tests the same way. @macohen open an issue please, fix the TODOs ask and we'll merge this.

Opened https://github.com/opensearch-project/opensearch-py/issues/653, pushed an update with the failing test skipped and all checks pass now! Thanks to @saimedhi and @dblock for all the help and patience. :)