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 django-rest-swagger #17

Open
Jawayria opened this issue Jul 26, 2021 · 32 comments
Open

Update or replace django-rest-swagger #17

Jawayria opened this issue Jul 26, 2021 · 32 comments
Labels
edx/ecommerce outdated dependency A dependency needs to be updated or replaced to support an upgrade initiative

Comments

@Jawayria
Copy link

We use the package django-rest-swagger in edx/course-discovery, edx/credentials, edx/demographics, edx/django-user-tasks, edx/ecommerce, edx/edx-analytics-data-api, edx/edx-notes-api, edx/enterprise-catalog, edx/license-manager, edx/portal-designer, edx/video-encode-manager. It hasn't yet added support for Django3.2. 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.

@Jawayria Jawayria added the outdated dependency A dependency needs to be updated or replaced to support an upgrade initiative label Jul 26, 2021
@NeOneSoft
Copy link

Hi @natabene, I would like to work in this issue if it's already available!! Thank you so much !!

@jmbowman
Copy link

I think she's still out for another day or two, but I can go ahead and assign it to you. Thanks for looking into this!

@NeOneSoft
Copy link

Hi @jmbowman !!!, Thank you so much for your support and I'll be working on!!!

@NeOneSoft
Copy link

Hi @jmbowman, django rest swagger is deprecated so as they mentioned in the repo, we can use drf-yasg, in fact we already using it and I ran the tests for Django3.2 successfully. I already created the PR for https://github.com/axnsan12/drf-yasg and waiting to be merged.

@natabene
Copy link

natabene commented Sep 7, 2021

Thanks for an update, @NeOneSoft

@NeOneSoft
Copy link

Hi @natabene I suggest to keep using https://github.com/axnsan12/drf-yasg since tests were ran successfully for Django 3.2 even tough this is not mentioned in the repo and I could not contact with the maintainer yet>

@natabene
Copy link

@Jawayria Please let me know what you think.

@jmbowman
Copy link

It looks like there was a maintainer transition last year, although it sounds like it's still hard to get maintainer attention. Maybe create a new issue asking if more help is wanted to get the next release out with Django 3.2 support and/or if a move to Jazzband would be appropriate? In the meantime, it sounds like the only blocker for our Django 3.2 upgrade is finishing the switch from django-rest-swagger to drf-yasg. It sounds like we need to do this in multiple repos; @NeOneSoft , are you up for creating some PRs to kick off that effort?

@natabene natabene reopened this Sep 17, 2021
@NeOneSoft
Copy link

Hi @jmbowman thank you for your response, I'm agree with you to switch django-rest-swagger to drf yasg so I'll be working with eduNEXT team to kick-off this effort and share updates or ask for help if we need it!!

@NeOneSoft
Copy link

Hi @jmbowman!!, we are wondering if its necessary to use https://github.com/edx/api-doc-tools in order to switch the repos to
drf-ysg or it could be done by simply do it directly in each repo. Thank you so much!!

@natabene
Copy link

@nedbat Could you help us out here? @jmbowman said you would have a better idea about this.

@NeOneSoft
Copy link

Hi @natabene @nedbat !!!,regarding to my last comment, do we have any update or suggestion to continue with this effort. Thank you so much!!! = )

@nedbat
Copy link
Contributor

nedbat commented Sep 30, 2021

I'm looking at this now. Here is @NeOneSoft 's PR for drf-yasg adding Django 3.2 support: axnsan12/drf-yasg#735

@nedbat
Copy link
Contributor

nedbat commented Sep 30, 2021

@NeOneSoft You don't have to switch the repos to using api-doc-tools. It would be nice to have all the repos using the same docs foundation, but api-doc-tools only generates docs for "/api" by default, so it might be more work than we want to take on right now to also move to api-doc-tools.

@NeOneSoft
Copy link

Thank you so much @nedbat !!!Yes the upgrade was done on axnsan12/drf-yasg#735 so we will start to work on each repo for this issue. Thanks for your support!!!

@NeOneSoft
Copy link

Hi @jmbowman I couldn't find edx/demographics repo. Do you know if it was moved to some other place? Thanks in advance!!

@NeOneSoft
Copy link

@natabene @awais786 I think that edx/video-encode-manager is also private

@ericfab179
Copy link

Quick update here:

@NeOneSoft
Copy link

Reviewing @ericfab179 update I found also this:

@NeOneSoft
Copy link

Hi @natabene @awais786 @jmbowman !! I think that this issue is ready for review. Thank you so much for all your help and support!!

@awais786
Copy link

We are reviewing prs.

@natabene
Copy link

@awais786 What is still left here?

@awais786
Copy link

Repos still using django-rest-swagger
credentials, demographics, django-user-tasks, video-encode-manager, commerce-coordinator.
Few PRs already in review and few need PRs. Once all merged I will notify you.

@natabene
Copy link

@awais786 Just checking if those PRs have merged.

@Jawayria
Copy link
Author

@natabene @NeOneSoft these 2 PRs are still not merged
openedx/credentials#1408
openedx/django-user-tasks#176

@Jawayria
Copy link
Author

@NeOneSoft can you please create a PR to replace Django-rest-swagger in commerce-coordinator as well.

@NeOneSoft
Copy link

@Jawayria sorry about my delay to answer, I have been a little bit busy and I would like to know if you can help me with this repo please, commerce-coordinator

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

8 participants