feat: replace rest_framework_swagger with drf_yasg #1502

NeOneSoft · 2021-12-15T19:42:23Z

This PR was created following edx/upgrades#17 to switch django rest swagger to drf-yasg

openedx-webhooks · 2021-12-15T19:42:28Z

Thanks for the pull request, @NeOneSoft! I've created OSPR-6284 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

justinhynes · 2021-12-16T12:45:38Z

credentials/urls.py

 urlpatterns = oauth2_urlpatterns + [
    re_path(r"^admin/", admin.site.urls),
    re_path(r"^api/", include(("credentials.apps.api.urls", "api"), namespace="api")),
    re_path(r"^api-auth/", include((oauth2_urlpatterns, "rest_framework"), namespace="rest_framework")),
-    re_path(r"^api-docs/", get_swagger_view(title="Credentials API"), name="api_docs"),
+    re_path(r"^api-docs/$", schema_view.with_ui('swagger', cache_timeout=0), name="api_docs"),,


@NeOneSoft Looks like we may have an extra comma here that is causing issues the config files.

@justinhynes Thank you so much!!!

justinhynes

@NeOneSoft I think we're really close getting this PR wrapped up. It looks like we're running into a few quality issues. We should be able to use our existing tools to automate the fixes. Locally you can try running:
make isort - this should fix the error with the imports in the wrong order, then
black . - (note the period) black is a code formatter used in this repo to keep syntax common across all files.

From there you can run make quality locally to have isort/black/pylint run to point out any other potential quality issues.

I think after this we should be good to squash and merge the changes. :)

NeOneSoft · 2021-12-16T15:56:48Z

@justinhynes !!!!Thank you so much for you instructions and support. I already apply the suggested commands and once our quality issues were checked I will be doing squash!!

NeOneSoft · 2021-12-16T18:30:34Z

Hey@justinhynes!! after apply isort, black and quality we're still having and import issue on copy_catalog.py file but I think that the sort is correct.

justinhynes · 2021-12-16T20:09:43Z

@NeOneSoft I'm sorry this is such a pain in the neck! I definitely didn't expect 126 files to be updated. I'm curious as to what is going on here. Going to take some extra time to think this through... thanks for your patience.

NeOneSoft · 2021-12-17T08:04:30Z

No problem @justinhynes!!!!I'll be attending your comments, thanks again for all your support!!

justinhynes · 2021-12-17T20:36:18Z

@NeOneSoft Hey there! Apologies that I haven't been able to return and triage your issues any further. I am wrapping up my day here soon and actually am off until January. If it's okay with you, we can try and get this sorted out shortly after the new year. Thanks for understanding and talk soon!

NeOneSoft · 2021-12-21T17:38:37Z

Thank you so much @justinhynes, enjoy and happy holidays!!!!

natabene · 2022-01-11T19:55:51Z

@NeOneSoft Thanks so much for your contribution. @justinhynes Thank you for reviewing!

hurtstotouchfire · 2022-03-18T20:38:24Z

Hey @NeOneSoft, I'd like to get this merged but I think we will need to reduce the number of touched files. Are you able to rebase and do some commit cleanup? I would like to see 5f2d5ca fixed up to the commit before it (just in case we end up dividing up this PR, I don't want that fixup to be separated from the commit it fixes) and then pull out 93db679 for the time being. You could stash it in another branch for now, and then we can determine if we want to pull some of it in piecemeal. Does that sound doable?

NeOneSoft · 2022-03-22T13:04:46Z

Hi @hurtstotouchfire , thanks for your review I think that in the last fix I touched many files so I'll do you the cleanup for this as you mentioned to move forward.

natabene · 2022-04-01T19:12:23Z

@NeOneSoft Please let us know once you are ready for this to be reviewed again.

NeOneSoft · 2022-04-05T22:32:11Z

Hi @natabene of course, I'll be working on it this week!

NeOneSoft · 2022-04-18T15:21:58Z

Hi @hurtstotouchfire!!,Are you agree if I store the changes of 94db679in another branch as you suggested and then I reset and squash to 5f25ca?Thanks in advance

feat: modify settings base.py and url.py files feat: remove an extra comma from urls.py file feat: resolve conflicted files feat: resolve remaining conflicted files feat: resolve conflicted files and requirment versions

hurtstotouchfire

As long as @justinhynes has no blocking concerns I think this is good to go from our perspective. I think you may need to resolve conflicts though.

justinhynes · 2022-05-31T18:03:06Z

Hey @NeOneSoft,

I was taking another look at this and I think I found one final complaint. It looks like you did the right steps to add drf_yasg to the base.in file, and you added that package to the installed_apps list in base.py. However, it looks like the package dependency is missing from the requirements base.txt file.

Please run make upgrade locally. This make target will regenerate the requirements text files and will pull in the new drf_yasg package. Afterwards, please push these updated files to this PR.

Otherwise, if we were to merge this, the service won't come up successfully as the drf_yasg requirement won't be installed (but will be listed as a required app, which will cause an error).

So, to summarize:

Run make upgrade locally
Confirm that drf_yasg==<some_version> appears in the base.txt requirements file
Push those changes to this PR

Afterwards, as long as all the tests pass, we should be good to go!

justinhynes

The drf_yasg package is missing from the base.txt file and will cause an issue if not included. Please see my latest comment on your PR to fix this issue. Thanks!

NeOneSoft · 2022-06-15T19:50:55Z

Hi @justinhynes !!Thanks a lot for your review. I already run the commands are you suggested. I'll be glad to attend your comments and thanks again

justinhynes · 2022-06-15T20:06:50Z

@NeOneSoft Great, thank you for those changes. I took a look and it looks good to go. I'm going to approve the PR.

tuchfarber · 2022-06-15T20:14:08Z

🎉

hurtstotouchfire · 2022-06-16T13:19:25Z

Hmmmm we have merge conflicts again. Sorry @NeOneSoft this tends to happen when there are dependency updates because we are also merging update PRs so they tend to conflict.

NeOneSoft · 2022-06-16T13:57:24Z

No worries @hurtstotouchfire !!!, should we wait to get updated all our dependencies before apply make upgrade again?

hurtstotouchfire · 2022-06-16T18:26:47Z

@NeOneSoft I had one more update PR to merge which may or may not conflict for you but after this you should be good to go.

This is the PR: #1689

Let us know when you're de-conflicted!

NeOneSoft · 2022-06-16T22:07:06Z

Hi @hurtstotouchfire!!, I had errors rebasing this branch so I created a new one on PR with exactly the same changes but starting from updated master. I think that this should be solve the conflicts. For last, could you please review it to be sure that everything it's ok?. For know titled as "draft" but I will change it once this is ready for merge. Thanks a lot !!

NeOneSoft · 2022-06-29T16:00:59Z

This PR can be closed on behalf of PR where the required changes were done and some issues regarding python 3.8 dependencies where solved. Thanks everyone for your all your support!!!!

openedx-webhooks · 2022-06-29T16:01:21Z

@NeOneSoft Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

NeOneSoft requested a review from a team as a code owner December 15, 2021 19:42

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Dec 15, 2021

NeOneSoft mentioned this pull request Dec 15, 2021

feat: replace rest_framework_swagger with drf_yasg #1408

Closed

justinhynes reviewed Dec 16, 2021

View reviewed changes

NeOneSoft force-pushed the ednx/add_drf_yasg branch from 2792b87 to 5f2d5ca Compare December 16, 2021 13:38

justinhynes requested changes Dec 16, 2021

View reviewed changes

openedx-webhooks added changes requested and removed needs triage labels Dec 16, 2021

hurtstotouchfire self-requested a review March 18, 2022 20:26

openedx-webhooks added engineering review and removed changes requested labels Apr 27, 2022

NeOneSoft force-pushed the ednx/add_drf_yasg branch from d15c1f0 to 5f2d5ca Compare April 29, 2022 15:31

feat: replace rest_framework_swagger with drf_yasg library

49c8f0d

feat: modify settings base.py and url.py files feat: remove an extra comma from urls.py file feat: resolve conflicted files feat: resolve remaining conflicted files feat: resolve conflicted files and requirment versions

NeOneSoft force-pushed the ednx/add_drf_yasg branch from 4fc504a to 49c8f0d Compare April 29, 2022 17:16

NeOneSoft requested a review from justinhynes April 29, 2022 17:25

hurtstotouchfire approved these changes May 31, 2022

View reviewed changes

hurtstotouchfire self-assigned this May 31, 2022

justinhynes requested changes May 31, 2022

View reviewed changes

feat: make upgrade files

5a3344c

openedx-webhooks removed the engineering review label Jun 15, 2022

justinhynes approved these changes Jun 15, 2022

View reviewed changes

NeOneSoft mentioned this pull request Jun 16, 2022

feat: update drf_yasg library for python 38 #1690

Merged

NeOneSoft closed this Jun 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace rest_framework_swagger with drf_yasg #1502

feat: replace rest_framework_swagger with drf_yasg #1502

NeOneSoft commented Dec 15, 2021

openedx-webhooks commented Dec 15, 2021 •

edited

Loading

justinhynes Dec 16, 2021

NeOneSoft Dec 16, 2021

justinhynes left a comment

NeOneSoft commented Dec 16, 2021

NeOneSoft commented Dec 16, 2021

justinhynes commented Dec 16, 2021

NeOneSoft commented Dec 17, 2021

justinhynes commented Dec 17, 2021

NeOneSoft commented Dec 21, 2021

natabene commented Jan 11, 2022

hurtstotouchfire commented Mar 18, 2022

NeOneSoft commented Mar 22, 2022 •

edited

Loading

natabene commented Apr 1, 2022

NeOneSoft commented Apr 5, 2022

NeOneSoft commented Apr 18, 2022

hurtstotouchfire left a comment

justinhynes commented May 31, 2022 •

edited

Loading

justinhynes left a comment

NeOneSoft commented Jun 15, 2022

justinhynes commented Jun 15, 2022

tuchfarber commented Jun 15, 2022

hurtstotouchfire commented Jun 16, 2022

NeOneSoft commented Jun 16, 2022

hurtstotouchfire commented Jun 16, 2022

NeOneSoft commented Jun 16, 2022

NeOneSoft commented Jun 29, 2022

openedx-webhooks commented Jun 29, 2022

feat: replace rest_framework_swagger with drf_yasg #1502

feat: replace rest_framework_swagger with drf_yasg #1502

Conversation

NeOneSoft commented Dec 15, 2021

openedx-webhooks commented Dec 15, 2021 • edited Loading

justinhynes Dec 16, 2021

Choose a reason for hiding this comment

NeOneSoft Dec 16, 2021

Choose a reason for hiding this comment

justinhynes left a comment

Choose a reason for hiding this comment

NeOneSoft commented Dec 16, 2021

NeOneSoft commented Dec 16, 2021

justinhynes commented Dec 16, 2021

NeOneSoft commented Dec 17, 2021

justinhynes commented Dec 17, 2021

NeOneSoft commented Dec 21, 2021

natabene commented Jan 11, 2022

hurtstotouchfire commented Mar 18, 2022

NeOneSoft commented Mar 22, 2022 • edited Loading

natabene commented Apr 1, 2022

NeOneSoft commented Apr 5, 2022

NeOneSoft commented Apr 18, 2022

hurtstotouchfire left a comment

Choose a reason for hiding this comment

justinhynes commented May 31, 2022 • edited Loading

justinhynes left a comment

Choose a reason for hiding this comment

NeOneSoft commented Jun 15, 2022

justinhynes commented Jun 15, 2022

tuchfarber commented Jun 15, 2022

hurtstotouchfire commented Jun 16, 2022

NeOneSoft commented Jun 16, 2022

hurtstotouchfire commented Jun 16, 2022

NeOneSoft commented Jun 16, 2022

NeOneSoft commented Jun 29, 2022

openedx-webhooks commented Jun 29, 2022

openedx-webhooks commented Dec 15, 2021 •

edited

Loading

NeOneSoft commented Mar 22, 2022 •

edited

Loading

justinhynes commented May 31, 2022 •

edited

Loading