-
Notifications
You must be signed in to change notification settings - Fork 485
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
Use django-jsonfield-backport (and move build env to github actions) #474
Conversation
This will also make #473 obsolete. Sorry. |
.github/workflows/workflow.yaml
Outdated
# mysql service | ||
mysql: | ||
# docker hub image | ||
image: mysql:5.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.
why not upgrading to mysql 8.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.
These are the same requirements as in the tox build. Will happily upgrade to newer releases
.github/workflows/workflow.yaml
Outdated
# postgres service | ||
postgres: | ||
# docker hub image | ||
image: postgres:9.5-alpine |
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.
postgres:10+ubuntu would be great
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.
These are the same requirements as in the tox build. Will happily upgrade to newer releases
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.
Any specific reasons for ubuntu as alpine images are smaller, lightweight and faster.
@@ -1,35 +0,0 @@ | |||
[tox] |
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.
should we delete tox right now?
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 don't see a reason to continue using tox as GitHub actions can cover everything required.
Less tools, less trouble.
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.
You can even run GitHub actions locally in a proper way with https://github.com/nektos/act
This will also use the proper Python versions whereas tox locally uses the installed 3.x release for all 3.x tests as it's based on virtualenv. So no proper 3.6, 3.7, 3.8, etc tests locally with tox.
Updated postgres to 13 and mysql to 8 |
Well... this PR started with the discussion in #449
Because I was annoyed by the extremely slow build times on TravisCI, I decided to move the build env to GitHub actions.
Not a big deal I tought...
(Sidenote: Travis will more or less remove it's free tier, so the migration would have been necessary at some point in the future anyway)
During implementation I found out that the current build is broken for Django 3.0/3.1, see
django-activity-stream/tox.ini
Lines 13 to 14 in 04db1ed
This is now implicitly fixed by the move to GitHub actions.
The build env also defined django-jsonfield>=1.0.1, see
django-activity-stream/tox.ini
Line 17 in 04db1ed
which will break tests on postgres because it doesn't deserialize JSON anymore after it's 1.3.0 release on newer Django >= 3.1 which is also mentioned in #457
Long story short: There is no easy and proper way to make all of this work smoothly. Therefore I decided to include the next step and replace all currently used libs with django-jsonfield-backport.
This change addresses #472, #449 and #437
The library gets installed by the
jsonfield
extra of actstream and will be dynamically loaded when necessary.Everything should be fully compatible with existing setups.