Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Bug 1134890 upgrade to django 15 #90

Merged
merged 13 commits into from Mar 10, 2015
Merged

Bug 1134890 upgrade to django 15 #90

merged 13 commits into from Mar 10, 2015

Conversation

peterbe
Copy link
Contributor

@peterbe peterbe commented Feb 23, 2015

No description provided.

@peterbe
Copy link
Contributor Author

peterbe commented Feb 23, 2015

@camd any idea why travis doesn't run pull requests?

@peterbe peterbe changed the title [DO NOT MERGE] Bug 1134890 upgrade to django 15 Bug 1134890 upgrade to django 15 Mar 6, 2015
@peterbe
Copy link
Contributor Author

peterbe commented Mar 6, 2015

Hey @shinglyu and @camd
This monster of a PR is going to be hard to review but I want you to be aware of it. We can't not go ahead with this. We need to upgrade Django to a supported version because old versions don't get security fixes.

This whole PR is about 1 original thing: Upgrade django from 1.4 to 1.5.

Doing so required me to also upgrade django-webtest and django-tastypie.
And django-tastypie has changed a lot from 0.9.11 to 0.12.1. To begin with, the whole permission model has changed. Another big change is that IDs, that are integers, used to be returned as strings (e.g "123") but now they're returned as ints (e.g. 123).

I guess it's going to be hard to review this. Perhaps we just land it, stick it on dev and bang at it till it works.

My plan is to upgrade a bunch of other dependencies too. Like django-browserid and django-compressor. However, the next goal is to continue to upgrade Django to 1.6. We can't go all the way to Django 1.7.

@camd
Copy link
Contributor

camd commented Mar 6, 2015

Peter: Not sure if it’s worth it, but we could split the commits to make it more review-able. Like with git add -p. Though I’ve been using Atlassian SourceTree to do it and that is MUCH easier. You can split chunks of changes up and makes it much easier to follow.

It may not be worth it in this case. Sounds like the biggest issue was the permissions thing, though. And I think I understand that problem as well as I’m going to… :) So no need to review for that.

So maybe, like you say, we could just go with it.

Fwiw- I skipped 1.6 and went from 1.5 to 1.7 and that wasn’t that bad. A few issues wrt migrations, which was the big difference. But the python 2.6/2.7 issue may be what stops us on that.

Maybe I should take a quick look through it to see if anything stands out. Did you want to merge that today?
-Cam

On Mar 6, 2015, at 8:58 AM, Peter Bengtsson notifications@github.com wrote:

Hey @shinglyu https://github.com/shinglyu and @camd https://github.com/camd
This monster of a PR is going to be hard to review but I want you to be aware of it. We can't not go ahead with this. We need to upgrade Django to a supported version because old versions don't get security fixes.

This whole PR is about 1 original thing: Upgrade django from 1.4 to 1.5.

Doing so required me to also upgrade django-webtest and django-tastypie.
And django-tastypie has changed a lot from 0.9.11 to 0.12.1. To begin with, the whole permission model has changed. Another big change is that IDs, that are integers, used to be returned as strings (e.g "123") but now they're returned as ints (e.g. 123).

I guess it's going to be hard to review this. Perhaps we just land it, stick it on dev and bang at it till it works.

My plan is to upgrade a bunch of other dependencies too. Like django-browserid and django-compressor. However, the next goal is to continue to upgrade Django to 1.6. We can't go all the way to Django 1.7.


Reply to this email directly or view it on GitHub #90 (comment).

@peterbe
Copy link
Contributor Author

peterbe commented Mar 6, 2015

Maybe I should take a quick look through it to see if anything stands out. Did you want to merge that today?

Not today. But some time early next week would be nice.
Don't scrutinize. That would take too long for you. But skim through for big obvious problems.

One thing that makes me a tiny percent worried and something I just opted to not worry about for now is that in Django 1.5 there's a validator regex on the username thus now disallowing spaces or : characters and we have some of those in the mysql database. However, this will go away over time since we'll be using Persona which generates a safe-looking username.

It might matter if you do some editing of old user accounts maybe.

@@ -61,10 +63,50 @@ def is_authenticated(self, request, **kwargs):
return self.get_key(user, api_key)


# # # Debuggin'
Copy link
Contributor

Choose a reason for hiding this comment

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

should we remove this now? or still useful for debuggin'... :)

camd pushed a commit that referenced this pull request Mar 10, 2015
@camd camd merged commit d311bf7 into mozilla:master Mar 10, 2015
@peterbe
Copy link
Contributor Author

peterbe commented Mar 10, 2015

You merged it. I didn't get a chance to delete all the left-over debugging that was commented out or left defunct. Shall I clean those up after?

@peterbe peterbe deleted the bug-1134890-upgrade-to-django-15 branch March 10, 2015 14:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants