-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Django 1.11 upgrade #4750
Django 1.11 upgrade #4750
Conversation
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.
A lot of changes in 1.11 but nothing breaking for our code! Changes look good for me, I'll test locally some projects next week.
I'm really excited for the update ⬆️ 🆙
I should note that I have not yet extensively tested this. I ran the test suite, clicked around, and did a full local build but not much else. |
Running this locally and will see if I hit any issues. Builds are working though, so that's good :) |
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.
Amazing!
All the changes look good to me. Although, I'd think that we need to block this until we have other repos also updated:
- readthedocs-ops (probably the most important one --the setting name was changed)
- readthedocs-corporate-ops
- readthedocs-corporate
- readthedocs-ext (already merged)
Changes in corporate are not required for this PR to get merged, but at least I think we can test how much it breaks and start upgrading it there also. Otherwise, we will be blocked in an old version of .org when deploying.
I will start with corporate and share how it goes.
def test_make_project(self): | ||
"""Test that a superuser can use the API.""" | ||
def test_cant_make_project(self): | ||
"""Test that a user can't use the API to create projects.""" |
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 inverted the logic of the test here.
Don't we still need the test for testing that it's possible to use the API as super user to create a project?
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 decided to remove the creation of a project using the api, it was never documented anyway.
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.
On this change specifically, there's more backstory in #4325.
@@ -126,7 +126,7 @@ def INSTALLED_APPS(self): # noqa | |||
def USE_PROMOS(self): # noqa | |||
return 'readthedocsext.donate' in self.INSTALLED_APPS | |||
|
|||
MIDDLEWARE_CLASSES = ( | |||
MIDDLEWARE = ( |
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.
This will require a change in -ops
and -corporate-ops
.
six==1.11.0 | ||
future==0.16.0 | ||
|
||
# django-tastypie 0.13.x and 0.14.0 are not compatible with our code | ||
django-tastypie==0.13.0 | ||
# django-tastypie 0.14.0 drops support for django 1.9 |
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.
If we are migrating to django 1.11 we may be possible to use 0.14.x here, I think.
It doesn't break too much. I've already opened a PR on the corporate site and all tests are passing. First step done. |
Just deployed 2.7.2. Will now merge this, so we can get more testing by using it locally & in CI. |
Model.__init__
checking keyword values against known good values.