-
Notifications
You must be signed in to change notification settings - Fork 71
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
docs: [FC-0002] plugin extensions ADR #1797
Conversation
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:
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. |
@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. | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this 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.
@cdeery @GlugovGrGlib cannot merge this, would you do the honors? |
@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. |
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