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

[CCI][BUG] Test support required for Python v3.9+ #336

Closed
wants to merge 3 commits into from

Conversation

bl1nkker
Copy link
Contributor

Description

Python 3.9+ support has been added

Issues Resolved

#323

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.

@dblock
Copy link
Member

dblock commented Mar 20, 2023

Thanks, needs -s for DCO please.

@bl1nkker
Copy link
Contributor Author

done 👍

@dblock
Copy link
Member

dblock commented Mar 21, 2023

Before I merge this, we have a list of Python versions in https://github.com/opensearch-project/opensearch-py/blob/main/.github/workflows/ci.yml#L42. Why do we have the same list here? What does this one do, and are we going to forget to add a version next time?

I'd imagine we want to have a list of Python versions in GHA only, and every time we add a new version we also say "Added support for Python X.Y" in CHANGELOG.

@bl1nkker
Copy link
Contributor Author

When I was working on another problem, I noticed this too. It was rather strange to see that the same lines of code for tests were repeated twice

In any case, those versions specified in ci.yml are required for testing before deployment. While the versions that are in noxfile are needed for testing locally. In fact, this tests are only needed for development purposes.

@dblock
Copy link
Member

dblock commented Mar 21, 2023

@bl1nkker Should we add a comment here to also update .yml for CI and vice-versa?

@bl1nkker
Copy link
Contributor Author

yeah, sure

…ents

Signed-off-by: bl1nkker <nurovich14@gmail.com>
@bl1nkker
Copy link
Contributor Author

done👍

also changed changelog.md

@bl1nkker
Copy link
Contributor Author

@dblock Should i resolve this conflict? Or is it better to wait for a mentor?

@dblock
Copy link
Member

dblock commented Mar 28, 2023

You should always rebase/resolve conflicts in your PRs, and iterate to green until they are merged.

@bl1nkker
Copy link
Contributor Author

bl1nkker commented Apr 7, 2023

yes, sorry for the delayed reply

@bl1nkker
Copy link
Contributor Author

bl1nkker commented Apr 8, 2023

any idea why python 3.4-3.6 tests don't work?
image

python-version: ["2.7", "3.7", "3.8", "3.9", "3.10", "3.11"]
# Python versions used for testing. Any changes made here should also be reflected in the noxfile.py file.
python-version:
["2.7", "3.4", "3.5", "3.6", "3.7", "3.8", "3.9", "3.10", "3.11"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Version matrix can be kept the same. Python versions 3.5 and 3.6 need a specific os and hence are already included in line 47 and line 50 of this file.

@VachaShah
Copy link
Collaborator

any idea why python 3.4-3.6 tests don't work? image

Please check my latest comment on why the tests are failing.

@saimedhi
Copy link
Collaborator

saimedhi commented Jun 2, 2023

Hello, @bl1nkker. Please do not modify existing Python versions. It is sufficient to add versions 3.10 and 3.11 to the Nox file and the Github workflow. If you are unable to resolve merge conflicts, please close this PR and create a new PR with only the changes indicated. Thanks :)

@saimedhi
Copy link
Collaborator

Hello @bl1nkker, please complete the pull request.

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

Successfully merging this pull request may close these issues.

5 participants