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.

Sign up for GitHub

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

Jump to bottom

Updated django-oauth2-provider #11432

Merged
merged 1 commit into from
Feb 10, 2016
Merged

Updated django-oauth2-provider #11432

merged 1 commit into from
Feb 10, 2016

Conversation

clintonb
Copy link
Contributor

@clintonb clintonb commented Feb 4, 2016

New package includes support for client credentials grant.

@clintonb clintonb force-pushed the clintonb/oauth-client-credentials branch 2 times, most recently from f18d6b4 to 9f2bd9c Compare February 8, 2016 22:14
@clintonb
Copy link
Contributor Author

clintonb commented Feb 8, 2016

@nedbat @feanil please review. A diff of the OAuth lib is available at edx/django-oauth2-provider@0.2.7-fork-edx-6a...0.5.0. I'm not quite sure why it includes commits from 2013.

@clintonb
Copy link
Contributor Author

clintonb commented Feb 9, 2016

jenkins run all

@clintonb clintonb force-pushed the clintonb/oauth-client-credentials branch from 9f2bd9c to 83da1e8 Compare February 9, 2016 02:03
@clintonb
Updated django-oauth2-provider
e92802b 
New package includes support for client credentials grant.
@clintonb clintonb force-pushed the clintonb/oauth-client-credentials branch from 83da1e8 to e92802b Compare February 9, 2016 02:48
@feanil
Copy link
Contributor

feanil commented Feb 9, 2016

This part of paver, is not run on production builds, is it a requirement for this to get deployed correctly. If the version numbers of the new requirements have been upped, you shouldn't need that right?

@clintonb
Copy link
Contributor Author

clintonb commented Feb 9, 2016

@feanil I assume you're referring to the uninstall code. I don't know if installing the new package atop the previously-editable version will have any side-effects (e.g. the old version take precedence).

Do we have the option of building this AMI from scratch instead of the previously-deployed AMI?

@feanil
Copy link
Contributor

feanil commented Feb 9, 2016

The option to build AMIs from scratch does exist but it means the release master/devops engineer need to be made aware that they need to do it. It seems like the code precedence is something you can check quickly in a local virtualenv. My expectation is that if the packages have the same name, the last pip install will win.

@clintonb
Copy link
Contributor Author

clintonb commented Feb 9, 2016

I installed locally, and the assumption that the last-installed version is used is incorrect. Both packages are installed, but the editable version takes precedence. We will need to build from scratch.

edxapp@precise64:~/edx-platform$ python
Python 2.7.10 (default, Jun 29 2015, 22:38:23) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import provider
>>> provider.__version__
'0.2.7-fork-edx-6'
edxapp@precise64:~/edx-platform$ pip freeze | grep oauth2-provider
-e git+https://github.com/edx/django-oauth2-provider.git@764601a74b368a6b7296c229673a23a7e940be3f#egg=django_oauth2_provider-0.2.7-fork-edx-6a
edx-django-oauth2-provider==0.5.0
edx-oauth2-provider==0.5.9

@clintonb
Copy link
Contributor Author

@feanil @nedbat thumbs?

@feanil
Copy link
Contributor

feanil commented Feb 10, 2016

@nasthagiri @fredsmith you will be in-charge of the release when this goes out so I want you both to be aware. This change will require building edxapp AMIs from scratch.

👍 @clintonb I want both of them to confirm they have seen this before you merge this.

nedbat
nedbat reviewed Feb 10, 2016
View reviewed changes
pavelib/prereqs.py
if any("django-oauth2-provider==" in line for line in frozen):
sh("pip uninstall --disable-pip-version-check -y django-oauth2-provider")
uninstalled = True

Copy link
Contributor

Choose a reason for hiding this comment

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

This won't run unless you also update expected_version on line 186. Are you sure you need to perform an explicit uninstall?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nedbat it turns out he does, because the egg names are different. django-oauth2-provider vs edx-django-oauth2-provider

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry, I missed the egg-name change.

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to change the expected_version to get this uninstall to run.

@nedbat
Copy link
Contributor

nedbat commented Feb 10, 2016

There are commits from 2013 in the diff because @justinabrahms pulled them in with this commit: edx/django-oauth2-provider@c1cbc3b
which merged them in? Do we want those commits?

@nasthagiri
Copy link
Contributor

How does this change relate to what @jcdyer is doing in his PR https://github.com/edx/edx-platform/pull/11397?

We had decided to wait on releasing Cliff's PR until we validated those changes with his next set of changes for django-oauth-toolkit. So there would be only one operational migration hiccup.

@clintonb
Copy link
Contributor Author

@nasthagiri Cliff's change includes a renaming of the exported package, requiring code changes. I have only changed the name of the package in setup.py (to support PyPI publishing). Merging this will result in one less thing for Cliff to do—uninstall the editable package.

@clintonb
Copy link
Contributor Author

@nedbat looking at edx/django-oauth2-provider#14, we made an explicit decision to retain the history and properly credit the original author of the implicit grant work. Yes, we are fine with the pre-2015 commits.

@nasthagiri
Copy link
Contributor

@clintonb 👍

@fredsmith
Copy link
Contributor

👍 from devops

clintonb added a commit that referenced this pull request Feb 10, 2016
@clintonb
Merge pull request #11432 from edx/clintonb/oauth-client-credentials
219b7f7 
Updated django-oauth2-provider
@clintonb clintonb merged commit 219b7f7 into master Feb 10, 2016
@clintonb clintonb deleted the clintonb/oauth-client-credentials branch February 10, 2016 21:44
@nedbat
Copy link
Contributor

nedbat commented Feb 10, 2016

What is the point of my reviewing this and saying (twice) that you need to change a value in the uninstall code to get it to run, if you are going to merge it anyway? :(

@clintonb
Copy link
Contributor Author

@nedbat I apologize. I thought this had been resolved during the discussion with @feanil. I will create another PR with the updated text file.

clintonb pushed a commit that referenced this pull request Feb 11, 2016
Updated expected version for python_uninstall_version.txt
f78945b 
Follow up to #11432. This value needs to be incremented to ensure the old package is uninstalled.

ECOM-3647
@clintonb clintonb mentioned this pull request Feb 11, 2016
Merged
@jzoldak
Copy link
Contributor

jzoldak commented Feb 11, 2016

@clintonb @feanil @nedbat @benpatterson this has broken ansible provisioning for a new jenkins worker. See https://build.testeng.edx.org/job/build-packer-ami/496/consoleText

    amazon-ebs: Successfully uninstalled edxval-0.0.9
    amazon-ebs: pip uninstall --disable-pip-version-check -y django-oauth2-provider
    amazon-ebs:
    amazon-ebs:
    amazon-ebs: Captured Task Output:
    amazon-ebs: ---------------------
    amazon-ebs:
    amazon-ebs: ---> pavelib.bok_choy.test_bokchoy
    amazon-ebs: ---> pavelib.prereqs.install_prereqs
    amazon-ebs: ---> pavelib.prereqs.install_node_prereqs
    amazon-ebs: ---> pavelib.prereqs.uninstall_python_packages
    amazon-ebs: pip freeze
    amazon-ebs: pip uninstall --disable-pip-version-check -y edxval
    amazon-ebs: pip uninstall --disable-pip-version-check -y django-oauth2-provider
    amazon-ebs:

[...]

    amazon-ebs: Build failed running pavelib.bok_choy.test_bokchoy: Subprocess return code: 1
    amazon-ebs: Cannot uninstall requirement django-oauth2-provider, not installed

@clintonb clintonb mentioned this pull request Feb 17, 2016
Merged
@anonim-01 anonim-01 mentioned this pull request May 13, 2024
Open
@sumodgeorge sumodgeorge mentioned this pull request May 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@clintonb @feanil @nedbat @nasthagiri @fredsmith @jzoldak