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

Moved OAuth support out of DRF and into a separate package. #1840

Closed
wants to merge 2 commits into from

Conversation

jlafon
Copy link
Contributor

@jlafon jlafon commented Sep 5, 2014

Closes #1767.

This work came from a sprint at DjangoCon 2014.

This PR removes all functionality and tests for OAuth 1.0a and 2.0 from DRF (see #1767). The same functionality has been ported to the Django REST Framework OAuth package. I plan on updating the DRF OAuth package to use oauthlib, which is actively maintained and supports Python 3.

  • Only necessary changes were made in moving code from DRF to DRF OAuth
  • Dependencies in travis, tox, and requirements.txt for django-oauth-plus, oauth2, and django-oauth2-provider have also been removed
  • The documentation has been modified to reference the 3d party package. More documentation changes are necessary, but not until after I update DRF OAuth as described above

@tomchristie tomchristie added this to the 3.0 Release milestone Sep 6, 2014
@tomchristie
Copy link
Member

Nice! Milestoning for 3.0 - we shouldn't merge this just yet until we've had a bit of a chance to test it all out. I'd suggest pinging the mailing list about the current status of the work you've done here... https://groups.google.com/forum/#!topic/django-rest-framework/HAq4qVfYec4

Ideally if we could get one or two folks to help test if out that'd be great.

@tomchristie tomchristie changed the title Moved OAuth support out of DRF and into a separate package, per #1767 Moved OAuth support out of DRF and into a separate package. Sep 6, 2014
@tomchristie
Copy link
Member

@thedrow For one thing, we need to continue to support folks already using this OAuth package.

why not just recommand django-oauth-toolkit as the default

That may happen, yup.

@jlafon
Copy link
Contributor Author

jlafon commented Sep 12, 2014

I've pinged the mailing list about testing this but haven't gotten any replies.

Also, I'd like to start working to migrate django-rest-framework-oauth to oauthlib (or maybe oauthlib via django-oauth-toolkit) soon. If anyone wants to contribute toward that, PRs are always welcome.

@tomchristie
Copy link
Member

I've pinged the mailing list about testing this but haven't gotten any replies.

I was still meaning to get back to you on that. :)
I wouldn't worry about not getting responses immediately btw.

@carltongibson
Copy link
Collaborator

Just one question — can we not merge this against the 3.0 branch now, rather than against master sometime later?

@tomchristie
Copy link
Member

@carltongibson - Possibly, meaning to raise this on the discussion group.

I've been considering that a better strategy wrt easing migration pains of users might be to leave this (and any other move-out-of-core stuff that might crop up) to 3.1.

That would seem to present an easier upgrade path to me. 3.0 will be all the necessarily backwards-compat breaking stuff, but we'd be keeping that the changes as minimal as possible. 3.1 should then be a trivial upgrade step as with previous median point releases.

Bundling this into 3.0 runs the risk of folks attempting to upgrade, getting stuck on something, and then not being sure if the problem is due to 3.0 API changes or due to misconfiguration. (Hope I've explained adequately there)

Not fully decided, but pushing this in 3.1 would seem to present an incremental series of smaller steps for upgrades.

@carltongibson
Copy link
Collaborator

Hope I've explained adequately there

Yeah. All makes sense. Not sure either way myself — it's just a pip install ... and an import change... — but I see your point.

For me it would be nice to have the PR merged (on some branch). Folks will check that out and run into issues as the time approaches, where they might not check out a PR itself. (But again, who knows...)

@tomchristie
Copy link
Member

For me it would be nice to have the PR merged (on some branch).

Sure, wouldn't have any problem merging this into a 3.1 branch, that'd make sense.
Needs bring up to date with master first.

@carltongibson carltongibson modified the milestones: 3.0 pre-release reassessment, 3.0 Release, 3.1 Release Sep 15, 2014
@carltongibson
Copy link
Collaborator

@jlafon I've created a version-3.1 branch from 3.0.

Can I ask you to update and create a new pull request with these changes against that? (Pull the latest in from the branch; push and pick that branch when comparing on GitHub.)

We'll get this merged in there — Thanks for the effort. Top work!

@Geekfish Geekfish mentioned this pull request Nov 15, 2014
@carltongibson
Copy link
Collaborator

Closing in Favour of #2076.

(Aside: I closed #1767, taking it for the PR — if people feel strongly about it I can reopen no problem)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move built-in OAuth support into a third-party package.
3 participants