Skip to content
This repository was archived by the owner on May 28, 2020. It is now read-only.

Add Django 1.11 compatibility. - #4

Merged
doctoryes merged 5 commits into
masterfrom
edx-fork
Jul 19, 2017
Merged

Add Django 1.11 compatibility.#4
doctoryes merged 5 commits into
masterfrom
edx-fork

Conversation

@doctoryes

Copy link
Copy Markdown

Just for testing...

Renzo Lucioni and others added 5 commits July 18, 2017 18:03
In most cases, only one OAuth package is necessary.
Use edX fork of django-oauth2-provider. Update dependencies.
@clintonb

Copy link
Copy Markdown

Where are we using this package? API endpoints that need Bearer authentication can use our custom backend. See http://edx-drf-extensions.readthedocs.io/en/latest/settings.html#bearerauthentication.

@doctoryes

Copy link
Copy Markdown
Author

find edx-platform -name \*.py -exec grep rest_framework_oauth {} \; -print shows 10 different places. Are you implying that it would be as simple as swapping out the OAuth2Authentication classes for BearerAuthentication classes?

@clintonb

Copy link
Copy Markdown

It should be that easy. You'll also need to configure the appropriate Django settings for that class.

@doctoryes

Copy link
Copy Markdown
Author

@clintonb I've tried creating a PR that removes this module as a dependency. The work-in-progress is here:
https://github.com/edx/edx-platform/pull/15603
However, it's incomplete and missing the Django settings configuration you mention.
Is the work to remove this dependency represented as a JIRA ticket? If not, I'd be happy to create a ticket. I'd like to move ahead with merging this PR and using this update in edx-platform for the Django 1.11 upgrade work. It'd be great to remove this dependency altogether, but it's out of scope of this work.

@doctoryes doctoryes changed the title Do Not Merge: Edx fork Add Django 1.11 compatibility. Jul 19, 2017
@doctoryes
doctoryes requested review from bmedx, clintonb and rlucioni July 19, 2017 17:38
@clintonb

Copy link
Copy Markdown

There is no ticket. Please create one.

Comment thread setup.py
packages=get_packages(package),
package_data=get_package_data(package),
cmdclass={'test': PyTest},
install_requires=install_requires,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks like there are valid requirements, is there a reason they're not shown here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The install_requires were removed in @rlucioni 's PR to make the dependencies optional based on which auth method was being used:
jpadilla#12

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@doctoryes if you're removing this you should also remove the line above that assigns install_requires. Out of curiosity, why aren't those changes already on this branch?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That was poor merging on my part between the changes made on the edx fork versus upstream. I'll make a PR to remove that install_requires - though it won't actually affect anything in this version.

@clintonb

Copy link
Copy Markdown

Can we expand the scope of the Django 1.11 project to ditch this package, please? Given that it is pretty much dead (jpadilla#11), I don't think we should invest in this package any longer.

@clintonb clintonb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I disagree with continuing to use this package, but this PR is fine as-is.

@doctoryes

Copy link
Copy Markdown
Author

I've created this ticket to perform the work to remove this dependency from edx-platform:
https://openedx.atlassian.net/browse/PLAT-1660

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants