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

Update or replace rest-condition dependency #2

Open
jmbowman opened this issue Jul 15, 2021 · 17 comments
Open

Update or replace rest-condition dependency #2

jmbowman opened this issue Jul 15, 2021 · 17 comments
Assignees
Labels
edx/ecommerce outdated dependency A dependency needs to be updated or replaced to support an upgrade initiative

Comments

@jmbowman
Copy link

We use the rest-condition package in edx/completion, edx/course-discovery, edx/credentials, edx/demographics, edx/ecommerce, edx/edx-analytics-dashboard, edx/edx-analytics-data-api, edx/edx-drf-extensions, edx/edx-enterprise, edx/edx-enterprise-data, edx/edx-notes-api, edx/edx-organizations, edx/edx-platform, edx/edx-proctoring, edx/edx-rbac, edx/edx-val, edx/edx-when, edx/enterprise-catalog, edx/license-manager, edx/portal-designer, edx/registrar, and edx/video-encode-manager (in some cases, only via use of another package in that list of repos). But it hasn't yet added support for Django 3.2, and in fact hasn't been updated since January 2019. Please follow the guidance in https://openedx.atlassian.net/wiki/spaces/AC/pages/3036972032/Handling+Outdated+Dependencies to resolve the problem this poses for the Open edX Django 3.2 upgrade.

@jmbowman jmbowman added the outdated dependency A dependency needs to be updated or replaced to support an upgrade initiative label Jul 15, 2021
@xitij2000
Copy link

@jmbowman I've run a quick search across the codebase and I see that while rest-condition is listed as a requirement, it no longer seems to be imported anywhere other than in edx-platform.

Additionally, I see that composing permissions has been supported in DRF since v3.9 : https://www.django-rest-framework.org/community/3.9-announcement/#composable-permission-classes

I can take this on, remove the requirement across all usages and switch edx-platform to using DRF-based composition directly.

@xitij2000
Copy link

edx-platform PR: https://github.com/edx/edx-platform/pull/28663
edx-drf-extensions PR: openedx/edx-drf-extensions#202

@natabene
Copy link

natabene commented Sep 7, 2021

@xitij2000 Thanks so much for an update.

@xitij2000
Copy link

The base PRs have been idle for a while. If they are not going to be worked on I can pick them up.

@regisb
Copy link

regisb commented Sep 15, 2021

I guess this issue should be assigned to @xitij2000 right @natabene?

@natabene
Copy link

@regisb Absolutely, thanks for letting me know.

@xitij2000
Copy link

xitij2000 commented Oct 5, 2021

completion: openedx/completion#160
course-discovery: openedx/course-discovery#3169

@xitij2000
Copy link

@regisb I've run upgrade on a few more repos and added links here. At this point for most repos automated dependency updates should fix things, and those will be part of any django 3.2 upgrade anyway.

I am on leave for a few days so won't be able to look into this for a bit. My quick scan of the repos I have checked out locally shows that they all use rest-condition via edx-drf-extensions. So there may be nothing more to do here.

@natabene
Copy link

natabene commented Oct 7, 2021

@xitij2000 Thanks so much for an update!

@xitij2000
Copy link

xitij2000 commented Oct 25, 2021

Completed:

@natabene
Copy link

@xitij2000 Thank you for an update, this is beautiful! @awais786 We can use the list @xitij2000 made to see what is still outstanding for our team.

@Jawayria
Copy link

@natabene @xitij2000 I've checked in edx/demographics and edx/video-encode-manager. We've edx-drf-extensions 8.0.0 in master branches for both of them.

@xitij2000
Copy link

I might not have time to follow through fully with openedx/edx-enterprise#1393 which is the PR to upgrade edx-enterprise. Upgrading dependencies causes a lot of linting errors due to f-strings etc, and running a normal pyupgrade isn't sufficient.

@xitij2000
Copy link

I think that PR is mostly ready. However, it seems pretty large after running pyupgrade and performing other cleanup.
Not sure what the process is for a PR like that.

@natabene
Copy link

@xitij2000 Thanks for your comment, we will review.

@mikix
Copy link

mikix commented Apr 7, 2022

I hope you don't mind, but I took the liberty of editing the tracking comment above to mark that edx-notes-api is now upgraded.

@xitij2000
Copy link

I hope you don't mind, but I took the liberty of editing the tracking comment above to mark that edx-notes-api is now upgraded.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
edx/ecommerce outdated dependency A dependency needs to be updated or replaced to support an upgrade initiative
Projects
None yet
Development

No branches or pull requests

7 participants