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

Remove QA support for Python 3.7 #410

Closed
wants to merge 1 commit into from
Closed

Conversation

amotl
Copy link
Member

@amotl amotl commented Nov 22, 2023

About

As suggested by @pilosus at GH-408, this patch drops official support for Python 3.7.

@amotl amotl requested review from seut and matriv November 22, 2023 23:19
@amotl amotl marked this pull request as ready for review November 22, 2023 23:20
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

I saw just a few downloads with 3.7, if you think it's fine to drop, I also don't have any objection.

Copy link
Member

@seut seut left a comment

Choose a reason for hiding this comment

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

If we remove QA for 3.7, we should remove general 3.7 support. Otherwise we don't know if 3.7 is still working. Or what is the rational about this otherwise?
We should also update the relevant documentation which seems to be quite outdated anyhow as 3.4 is still mentioned as supported. https://cratedb.com/docs/crate/crash/en/latest/getting-started.html

@@ -5,6 +5,7 @@ Changes for crash
Unreleased
==========

- Remove QA support for Python 3.7. Thanks, @pilosus.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Remove QA support for Python 3.7. Thanks, @pilosus.
- Remove support for Python 3.7. Thanks, @pilosus.

@pilosus
Copy link
Contributor

pilosus commented Nov 23, 2023

Basically, Python 3.8 or higher is needed for testing/developing only, as cratedb-toolkit itself we want to use and at least one of its transitive deps (click-aliases) require 3.8+. Given that we want to merge some changes to crash that require cratedb-toolkit for test extra, there are few options:

  1. Drop 3.7 support in crash in favour of 3.8, see my motivation/stats here. Pros: easy, Cons: under 1% of users may still be affected
  2. (this PR) Provide 3.7 general support in crash, but 3.8+ for testing/development. Pros: easy, Cons: inconsistent, error proned, given CI's testing matrix must be adjusted
  3. Fix cratedb-toolkit to support 3.7: get rid/fork/write our own alternative to click-aliases (it's just a single class under 100 LoC). Pros: Python version support is consitent across the projects, Cons: there might be more than 1 Python 3.8+ third-party deps in cratedb-toolkit, need to research first

@mfussenegger
Copy link
Member

cratedb-toolkit seems to have an extremely broad scope, and mentions it's alpha/beta/incubation-quality code. Are we sure we want to include that here?

@pilosus
Copy link
Contributor

pilosus commented Nov 23, 2023

cratedb-toolkit seems to have an extremely broad scope, and mentions it's alpha/beta/incubation-quality code. Are we sure we want to include that here?

my two cents:
cratedb-toolkit is meant, among other things, to contain testcontainers and all the wrappers/fixtures around it.
We are going to use them in crash as well, see #402

@pilosus
Copy link
Contributor

pilosus commented Nov 23, 2023

Btw, reg. this one:

Fix cratedb-toolkit to support 3.7: get rid/fork/write our own alternative to click-aliases

I've compiled both 3.7 and 3.8 and it seems that click-aliases is the only culprit that prevents using Python 3.7.
The code of click-aliases will be working just fine on 3.7. It's worth submitting the PR there, I guess

@pilosus
Copy link
Contributor

pilosus commented Nov 23, 2023

Ah, @amotl was quick!
click-contrib/click-aliases#14

@amotl
Copy link
Member Author

amotl commented Nov 23, 2023

Hi there,

thanks for your excellent feedback on this topic. I've also talked to @seut about it, and we decided not to bring in this patch for now, together with those additional thoughts, also responding to your requests and questions:

  1. While Python 3.7 is end-of-life, there is currently no strict need to drop support for it from the perspective of crash.
  2. While the Testcontainer stuff has been incubating in cratedb-toolkit, it does not need to stay like this. The plan is actually to upstream it to https://github.com/testcontainers/testcontainers-python. I think it might be the right time now to already start working on this, after Adapt testcontainers to be testing framework agnostic cratedb-toolkit#82 is properly integrated and absorbed.
  3. @pilosus: When following the (in my opinion) right strategy outlined at 2., I am humbly asking for your patience regarding Use testcontainers to improve platform coverage #408. It would need to wait a bit for the testcontainers code to be upstreamed beforehand.

With kind regards,
Andreas.

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.

5 participants