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

docs: [FC-0002] plugin extensions ADR #1797

Merged

Conversation

GlugovGrGlib
Copy link
Member

In this ADR you will find a proposal to adopt plugin extensions system, initially implemented in the Open edX platform, but latter was extracted from it and now is ready for using by IDAs, ref. https://github.com/openedx/edx-django-utils/blob/master/docs/decisions/0002-extract-plugins-infrastructure-from-edx-platform.rst.

This ADR is proposed as a part of FC-0002 General Design for Sharing Open edX Credentials

Ticket: #1735

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 20, 2022
@openedx-webhooks
Copy link

Thanks for the pull request, @GlugovGrGlib! 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.

@e0d
Copy link

e0d commented Oct 21, 2022

@GlugovGrGlib Thanks for this ADR, the references are very helpful, the decision seems well aligned with our other document architectural decisions. Broadly I approve of this direction, though I haven't officially approved the PR as I think it's critical that that is done by the maintaining team.

* Use plugin applications for future extension points and optional functionality for the Credentials service.

* Create a version of `cookiecutter-django-app`_ for the Credentials, to easily create new plugin applications.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to create a new cookiecutter specifically for Credentials? I'm not entirely familiar with how this works. Is it not extendable enough? Is this a common pattern?

Copy link
Member Author

@GlugovGrGlib GlugovGrGlib Nov 21, 2022

Choose a reason for hiding this comment

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

You're right, we probably don't need to create a specific cookiecutter for credentials, right now cookiecutter-django-app is completely universal.

My intention was to ease plugins creation for credentials and have apps config auto generated, app config examples for edx-platform:
https://github.com/openedx/edx-django-utils/tree/master/edx_django_utils/plugins#manual-setup
https://github.com/openedx/edx-platform/blob/master/openedx/core/djangoapps/ace_common/apps.py

But I think it will be much better to just include some examples in the documentation for extending credentials.

What do you think? Is it better to change this line with documentation decision or remove at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just remove it. Once there are some actual plugins, it may be easier to just point to them as examples, or document in the readme what needs to be done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

@cdeery cdeery left a comment

Choose a reason for hiding this comment

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

This looks good. Thank you.

@e0d
Copy link

e0d commented Dec 12, 2022

@cdeery @GlugovGrGlib cannot merge this, would you do the honors?

@cdeery cdeery merged commit f1f3b0d into openedx:master Dec 12, 2022
@openedx-webhooks
Copy link

@GlugovGrGlib 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@GlugovGrGlib GlugovGrGlib deleted the fc-0002/g-glugovskiy/plugin-ext-adr branch December 13, 2022 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants