Skip to content

Using edx-django-oauth2-provider from PyPI - #10734

Closed
clintonb wants to merge 1 commit into
masterfrom
clintonb/update-oauth2-provider
Closed

Using edx-django-oauth2-provider from PyPI#10734
clintonb wants to merge 1 commit into
masterfrom
clintonb/update-oauth2-provider

Conversation

@clintonb

Copy link
Copy Markdown
Contributor
  • Uninstalling old package
  • Using package from PyPI

Related to edx/django-oauth2-provider#21.

@clintonb

Copy link
Copy Markdown
Contributor Author

jenkins run all

@clintonb

Copy link
Copy Markdown
Contributor Author

@nedbat @edx/devops please review

FYI @justinabrahms @nasthagiri @edx/ecommerce

Comment thread pavelib/prereqs.py Outdated

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.

Given you're only using frozen to check for contents, it seems easier to not do splitlines, but get it back as a full string. That way you can do "{}==".format(package) in frozen which seems easier in a big string, rather than iterating over a list of strings.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@clintonb
clintonb force-pushed the clintonb/update-oauth2-provider branch from d6cc85b to a23bcb8 Compare November 25, 2015 17:21
@jimabramson

Copy link
Copy Markdown

@clintonb note that edx-oauth2-provider has this package as a direct dependency. I guess that is a separate problem but we need the dependencies to match.

@justinabrahms

Copy link
Copy Markdown
Contributor

Looks pretty good from my perspective. :)

@clintonb

Copy link
Copy Markdown
Contributor Author

Thanks for the heads-up @jimabramson. See edx/edx-oauth2-provider#33. I will update this code shortly to only use edx-oauth2-provider. I'm not sure why we are installing PyJWT or django-oauth2-provider separately.

@jimabramson

Copy link
Copy Markdown

@clintonb I think, unfortunately, that edx-platform accesses some of that stuff directly.

@clintonb
clintonb force-pushed the clintonb/update-oauth2-provider branch from a23bcb8 to eb96122 Compare November 25, 2015 19:13
@clintonb

Copy link
Copy Markdown
Contributor Author

@jimabramson I have confirmed that all direct uses are test-related. We don't seem to be using either package outside of the intended scope (in conjunction with edx-oauth2-provider), so this is a fairly safe change.

@jimabramson

Copy link
Copy Markdown

👍 but the accessibility failures look real.

Comment thread pavelib/prereqs.py

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 doesn't do the same thing as the original code, because edxval was installed with -e, and so wouldn't be found by this new code. It's probably a moot point now, since people have had to make new devstacks for 1.8 anyway, but we should get a more robust technique here...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Based on your statements, it sounds like this code is no longer needed. If I updated it to simply remove django-oauth2-provider, would that be okay?

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.

In theory we don't need to uninstall the other packages any more. I'm not a fan of getting rid of them just because we borked the code, though.

Also, you need to change the expected_version from 2 to 3 so that the uninstall will run at all.

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.

After I wrote the last comment, there's a developer in the Dev hipchat channel with a South failure. Not sure why, but these installations persist...

@rlucioni

Copy link
Copy Markdown
Contributor

@clintonb @jimabramson jumping in to note that https://github.com/edx/edx-platform/pull/10701 introduces direct use of PyJWT, separate from edx-oauth2-provider.

@feanil

feanil commented Nov 30, 2015

Copy link
Copy Markdown
Contributor

@clintonb currently we don't run this as a part of building AMIs so either AMIs will need to be built from scratch or there will need to be a corresponding configuration change to run this command as a part of building the edxapp AMIs.

@clintonb

clintonb commented Dec 2, 2015

Copy link
Copy Markdown
Contributor Author

@feanil is there any cost to building from scratch aside from time?

* Uninstalling old package
* Using package from PyPI
@clintonb
clintonb force-pushed the clintonb/update-oauth2-provider branch from eb96122 to a1668c2 Compare December 2, 2015 04:25
@clintonb clintonb closed this Feb 22, 2016
@clintonb
clintonb deleted the clintonb/update-oauth2-provider branch February 22, 2016 23:52
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