-
Notifications
You must be signed in to change notification settings - Fork 22
Update Django and Python versions in test matrix #91
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
base: master
Are you sure you want to change the base?
Conversation
I will look into refactoring the test stack to work with more modern packages and defaults, but wanted to have this in as a first step, and base any refactors off of that — so I plan to follow up separately after landing this. Feel free to amend the dependencies as you see fit, I'd rather not touch these (only wanted to have more current tests without rewriting any actual code as a first step). |
@@ -27,20 +27,19 @@ def find_version(*file_paths): | |||
packages=find_packages(), | |||
include_package_data=True, | |||
zip_safe=False, | |||
install_requires=["Django>=2.2", "requests>=2.21.0"], | |||
install_requires=["Django>=3.2", "requests>=2.26.0"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR the rest is mostly metadata, so this is really the only impact here — basically moving the requires in line with what is pinned for testing, to have the visibility into confirming it truly working. Feel free to revert if you feel like still keeping the older (and fully working as of now) versions around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine. If it breaks for somone, they can still use an earlier release while they open a bug report
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah requests needs to be kept at non-vulnerable version anyways, and I'm on the fence how to properly version stable packages when just silently dropping EOLed envs (and not really breaking them per se) … that are basically half a decade out of support. (Semver purists would insist on major, I don't think it's even noteworthy;D)
61e1406
to
b10c29d
Compare
- python-version: "3.9" | ||
django-tox-env: "django51" | ||
- python-version: "3.9" | ||
django-tox-env: "django52" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can avoid having to duplicate these exclusions by using tox-gh-actions
or what looks like a newer package maintained by the tox devs, tox-gh
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My issue with tox-gh* while cleaner in the yml (but chattier in the ini) is that it's extremely easy to have tests passing but run on non-matching env — if a mere typo in conf is passed to tox, it'd run all the M×N on defaults (=the same unconstrained deps just many times over) instead of being loud about it, and hide the fact silently inside endless collapsed plaintext logs.
(And, also, runs the tests sequentially instead of all in parallel, which is why I stopped upgrading to it honestly; it actually was part of this PR at some point, but I've probably rebased it away to see the matrix done faster…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However: I recently came across this smart hack mozilla-services/python-dockerflow#102 that kinda solves all of the shortcomings of the different approaches (combined!) so once I get a bit more familiar with it, I may start converting some workflows to it, to get rid of duplicated configs on both sides. Since I will be touching the matrix again soon, I'll try to apply something similar to that instead at that point.
"Programming Language :: Python :: 3.9", | ||
"Programming Language :: Python :: 3.10", | ||
"Programming Language :: Python :: 3.11", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could add "Programming Language :: Python :: 3 :: Only"
also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I'll be updating the identifiers again after redoing the test stack to something more current anyways so I'll keep that in mind.
(I must have followed a pattern from another moz(meao) package that also never adopted it at some point in the history…)
BTW I'm no expert in the metadata, what's the significance of the value per se? Is it to make it explicit that it used to support v2.x but now is only v3.x compatible? Or there's another sentiment included with it over the years? Thanks!
django40: Django>=4.0,<4.1 | ||
django42: Django>=4.2,<5.0 | ||
django51: Django>=5.1,<5.2 | ||
django52: Django>=5.2,<6.0 | ||
|
||
nose==1.3.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blast from the past 👃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good improvements. Totally fine if we want to work in any of my comments in for later phases.
Adding newer versions and removing non-LTS branches, now running tests on Python 3.9–3.11 and Django 3.2, 4.2 and 5.1–5.2 ✅
(To support Python 3.12 a refactor is in the cards unfortunately, migrating away from imp, therefore replacing nose with nose2 or pynose or pytest…)