Skip to content

Updated django-oauth2-provider - #11432

Merged
clintonb merged 1 commit into
masterfrom
clintonb/oauth-client-credentials
Feb 10, 2016
Merged

Updated django-oauth2-provider#11432
clintonb merged 1 commit into
masterfrom
clintonb/oauth-client-credentials

Conversation

@clintonb

@clintonb clintonb commented Feb 4, 2016

Copy link
Copy Markdown
Contributor

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

clintonb commented Feb 8, 2016

Copy link
Copy Markdown
Contributor Author

@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

clintonb commented Feb 9, 2016

Copy link
Copy Markdown
Contributor Author

jenkins run all

@clintonb
clintonb force-pushed the clintonb/oauth-client-credentials branch from 9f2bd9c to 83da1e8 Compare February 9, 2016 02:03
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

feanil commented Feb 9, 2016

Copy link
Copy Markdown
Contributor

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

clintonb commented Feb 9, 2016

Copy link
Copy Markdown
Contributor Author

@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

feanil commented Feb 9, 2016

Copy link
Copy Markdown
Contributor

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

clintonb commented Feb 9, 2016

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

@feanil @nedbat thumbs?

@feanil

feanil commented Feb 10, 2016

Copy link
Copy Markdown
Contributor

@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.

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

nedbat commented Feb 10, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@clintonb 👍

@fredsmith

Copy link
Copy Markdown
Contributor

👍 from devops

clintonb added a commit that referenced this pull request Feb 10, 2016
@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

nedbat commented Feb 10, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Follow up to #11432. This value needs to be incremented to ensure the old package is uninstalled.

ECOM-3647
@jzoldak

jzoldak commented Feb 11, 2016

Copy link
Copy Markdown
Contributor

@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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants