docs: [FC-0074] add comparison table between the framework and forking openedx #617

mariajgrimaldi · 2024-11-21T18:09:28Z

Description

This PR adds a comparison table between extending the platform using events/filters and modifying it directly in a fork.

openedx-webhooks · 2024-11-21T18:09:32Z

Thanks for the pull request, @mariajgrimaldi!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/docs-openedx-org-maintainers. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

source/developers/concepts/hooks_extension_framework.rst

sarina · 2024-11-25T15:40:09Z

source/developers/concepts/hooks_extension_framework.rst

+| **Disadvantages**                                                  | - Can increase complexity in debugging and maintenance because it adds more processing layers.                | - Separation from the core makes long-term maintenance challenging.                                                                |
+|                                                                    | - Limited by existing events and filters available in the Open edX ecosystem.                                 | - High maintenance burden for upgrades and compatibility.                                                                          |
+|                                                                    | - Might increase complexity in plugins for highly customized features.                                        | - Requires deep understanding of the platform to modify it correctly.                                                              |
+|                                                                    | - Might introduce an edx-platform dependency into plugins, mainly when using filters.                         |                                                                                                                                    |


Might introduce an edx-platform dependency into plugins, mainly when using filters.

What does this mean?

When implementing new features in plugins, even though you could be using an extension point which provides enough data about what's being extended, e.g., the enrollment filter provides the user, course_key, and enrollment mode, we might want to perform operations with the data like getting the course overview object that inevitably results in importing edx-platform code into the plugin. This is an antipattern, and it's strongly not recommended, but there's no current alternative for doing that kind of operation (this might be solved by learning core in the long term). Is this too specific to add here?

How does this sound as a summary / a little bit more detail & clarity?

Suggested change

| | - Might introduce an edx-platform dependency into plugins, mainly when using filters. | |

| | - Although it is not recommended to do so, a plugin might introduce an edx-platform dependency, mainly when using filters, when there is no alternative for getting certain types of data. | |

Yes! Thank you. I implemented the suggestion here: d4266da

sarina · 2024-11-25T15:41:24Z

source/developers/concepts/hooks_extension_framework.rst

+|                                                                    | - Easier to maintain long-term due to backward compatibility and versioning policies.                         |                                                                                                                                    |
+--------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
+| **Disadvantages**                                                  | - Can increase complexity in debugging and maintenance because it adds more processing layers.                | - Separation from the core makes long-term maintenance challenging.                                                                |
+|                                                                    | - Limited by existing events and filters available in the Open edX ecosystem.                                 | - High maintenance burden for upgrades and compatibility.                                                                          |


Maybe add a note that it is possible to add additional events/filters into the upstream?

sarina · 2024-11-25T15:42:32Z

source/developers/concepts/hooks_extension_framework.rst

+|                                                                    | - Might increase complexity in plugins for highly customized features.                                        | - Requires deep understanding of the platform to modify it correctly.                                                              |
+|                                                                    | - Might introduce an edx-platform dependency into plugins, mainly when using filters.                         |                                                                                                                                    |
+--------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
+| **Performance Impact**                                             | Medium to high, as events and filters execute within the caller process. Although events are designed so      | High, as changes directly impact the Open edX platform (edx-platform or other IDAs) and may require additional testing, monitoring,|


I'm not sure what "high" impact means - does this mean that performance will be worse no matter which you choose?

Mmmm. I think both the framework and forking could behave similarly under this category cause this mainly depends on how performant the solution is, which could be directly embedded into the code or extended using a plugin. I do think we could get more performant code by implementing more contained solutions through extension points, but that doesn't necessarily mean that choosing this approach improves performance.

I will remove this row and move the it may require additional testing... from the fork column to the implementation effort row.

sarina · 2024-11-25T15:49:38Z

source/developers/concepts/hooks_extension_framework.rst

+| **Performance Impact**                                             | Medium to high, as events and filters execute within the caller process. Although events are designed so      | High, as changes directly impact the Open edX platform (edx-platform or other IDAs) and may require additional testing, monitoring,|
+|                                                                    | receivers can trigger asynchronous tasks for improved performance.                                            | and performance considerations of new added code and for the integration itself.                                                   |
+--------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
+| **Community Compatibility**                                        | High, as the community encourage reusable implementations and the contribution of new extension points.       | Low, as changes in forks are usually highly customized not designed for others to reuse.                                           |


Suggested change

| **Community Compatibility** | High, as the community encourage reusable implementations and the contribution of new extension points. | Low, as changes in forks are usually highly customized not designed for others to reuse. |

| **Community Compatibility** | High, as the community encourages reusable implementations and the contribution of new extension points. | Low, as changes in forks are usually highly customized and not designed for others to reuse. |

sarina · 2024-11-25T15:49:57Z

source/developers/concepts/hooks_extension_framework.rst

+--------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
+| **Community Compatibility**                                        | High, as the community encourage reusable implementations and the contribution of new extension points.       | Low, as changes in forks are usually highly customized not designed for others to reuse.                                           |
+--------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
+| **Implementation Effort**                                          | Low to medium, depending on the complexity of the receiver logic and filter pipelines.                        | High, as modifications require expertise in the platform and increase technical debt.                                              |


Suggested change

| **Implementation Effort** | Low to medium, depending on the complexity of the receiver logic and filter pipelines. | High, as modifications require expertise in the platform and increase technical debt. |

| **Implementation Effort** | Low to medium, depending on the complexity of the receiver logic and filter pipelines. | High, as modifications require expertise in the platform and increases technical debt. |

felipemontoya

Having a table of pros and cons is difficult as it is very opinionated. I do think the information within is very accurate. And I commend the work put into this.

Left some comments that reflect my opinion in specific lines. None of them are blocking.

felipemontoya · 2024-12-09T22:30:45Z

source/developers/concepts/hooks_extension_framework.rst

+The Framework vs Forking the Open edX Platform
+**********************************************
+
+The Hooks Extension Framework is designed to provide a stable and maintainable way to extend the Open edX platform without modifying its core. This approach is in contrast to forking the platform, which involves creating a separate version that diverges from the core codebase. Here is a comparison table of the two approaches:


This approach is in contrast to forking -> sounds a bit weird to me.

How about: This approach stands in opposition to the practice of forking ... ?

Sounds better, yes. Thank you!

felipemontoya · 2024-12-09T22:32:26Z

source/developers/concepts/hooks_extension_framework.rst

+|                                                                    | **Open edX Events and Filters**                                                                               | **Forking**                                                                                                                        |
+====================================================================+===============================================================================================================+====================================================================================================================================+
+| **Purpose**                                                        | Allow extending functionality of the Open edX platform without modifying the                                  | Modify the code directly to meet specific needs.                                                                                   |
+|                                                                    | core codebase but by leveraging plugins and configurations.                                                   |                                                                                                                                    |


Suggested change

| | core codebase but by leveraging plugins and configurations. | |

| | core codebase by leveraging plugins and configurations. | |

Thanks. I completely missed that.

felipemontoya · 2024-12-09T22:35:13Z

source/developers/concepts/hooks_extension_framework.rst

+--------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
+| **Advantages**                                                     | - More decoupled implementations.                                                                             | - Full control of the platform behavior.                                                                                           |
+|                                                                    | - Decreased maintenance burden of the platform, maintenance efforts are centered on plugins.                  | - No restrictions from what can be changed.                                                                                        |
+|                                                                    | - Enables reusable and configurable behavior across the platform.                                             | - Suitable for highly customized use cases.                                                                                        |


Suggested change

| | - Enables reusable and configurable behavior across the platform. | - Suitable for highly customized use cases. |

| | - Enables reusable and configurable behavior across the platform. | - Suitable for extremely customized use cases. |

I think that hooks framework already allows for high customization. IMHO the modification must be extreme to warrant forking where hooks can do it. The more regular reason is when the platform does not support hooks in the place you want to customize.

Another point in this line would be:

Enables reusable and configurable behavior across the platform

I think is worth splitting in two:

Enables reusable behavior across the ecosystem and community.

Enables configurable behavior across the platform

I think that hooks framework already allows for high customization. IMHO the modification must be extreme to warrant forking where hooks can do it. The more regular reason is when the platform does not support hooks in the place you want to customize.

I agree. I added a note suggesting to look into how the framework can help before forking: 67bd6f7#diff-01d0bcd5d9113aaa8a7f3fde94c7e5ca100265eebebbad867dbf4de21123d451R193

I also split the sentence you mentioned into two for more clarity: 67bd6f7#diff-01d0bcd5d9113aaa8a7f3fde94c7e5ca100265eebebbad867dbf4de21123d451R172-R173

felipemontoya · 2024-12-09T22:49:09Z

source/developers/concepts/hooks_extension_framework.rst

+|                                                                    | - Enables reusable and configurable behavior across the platform.                                             | - Suitable for highly customized use cases.                                                                                        |
+|                                                                    | - Easier to maintain long-term due to backward compatibility and versioning policies.                         |                                                                                                                                    |
+--------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
+| **Disadvantages**                                                  | - Can increase complexity in debugging and maintenance because it adds more processing layers.                | - Separation from the core makes long-term maintenance challenging.                                                                |


I think we should call it "separation from the upstream project" because core has a different meaning here.

That's more clear. Thank you.

67bd6f7#diff-01d0bcd5d9113aaa8a7f3fde94c7e5ca100265eebebbad867dbf4de21123d451R177

felipemontoya · 2024-12-09T22:53:06Z

source/developers/concepts/hooks_extension_framework.rst

+| **Disadvantages**                                                  | - Can increase complexity in debugging and maintenance because it adds more processing layers.                | - Separation from the core makes long-term maintenance challenging.                                                                |
+|                                                                    | - Limited by existing events and filters available in the Open edX ecosystem, although new events and         | - High maintenance burden for upgrades and compatibility.                                                                          |
+|                                                                    |   filters can be proposed and added to the framework.                                                         | - Requires manual integration with the Open edX platform for each new release.                                                     |
+|                                                                    | - Might increase complexity in plugins for highly customized features.                                        | - Requires deep understanding of the platform to modify it correctly.                                                              |


Requires deep understanding of the platform to modify it correctly

I would argue that modifying correctly means using the open-closed principle which is not being used if the road was a fork.

I would list however the "not being able to reuse by the community" as part of the disadvantages.

Aso a disadvantage of fork:

difficult to test in isolation

I would argue that modifying correctly means using the open-closed principle which is not being used if the road was a fork.

I removed the "correctly" from the sentence. I do think that to modify the platform, you need a deep understanding of the components you'd be modifying, mainly because of the implications of the changes. But one can argue that you'd need the same to add new extension points, so I'll drop the line altogether.

I would list however the "not being able to reuse by the community" as part of the disadvantages.

Added! Thanks 67bd6f7#diff-01d0bcd5d9113aaa8a7f3fde94c7e5ca100265eebebbad867dbf4de21123d451R181

felipemontoya · 2024-12-09T22:54:06Z

source/developers/concepts/hooks_extension_framework.rst

+|                                                                    | - Although it is not recommended to do so, a plugin developer might introduce an edx-platform dependency,     |                                                                                                                                    |
+|                                                                    |   mainly when using filters, when there is no alternative for getting certain types of data.                  |                                                                                                                                    |
+--------------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
+| **Community Compatibility**                                        | High, as the community encourages reusable implementations and the contribution of new extension points.      | Low, as changes in forks are usually highly customized not designed for others to reuse.                                           |


Low, as changes in forks are usually highly customized not designed for others to reuse.

And discoverability is low. Most probably no one will ever know "you did that".

I added a note about discoverability here: 67bd6f7#diff-01d0bcd5d9113aaa8a7f3fde94c7e5ca100265eebebbad867dbf4de21123d451R185

* Address wording suggestion "in contrast of forking" * Remove extra but * Change high -> extreme to suggest that only extreme cases should warrant a fork for new features / behaviors * Split sentence reusable / configurable into two * Remove "correctly" from deep understanding sentence * Add test disadvantage for fork * Add test advantage for fork * Add discoverability note * Suggest looking into the framework before forking

mariajgrimaldi · 2024-12-10T11:31:41Z

@felipemontoya @sarina: thank you for the review!

Felipe, I think I addressed all of your comments with these two commits:
67bd6f7
1251605

felipemontoya

Thanks for addressing all the comments. Lgtm

mariajgrimaldi · 2024-12-11T19:59:58Z

Thank you all!

@sarina: merge when you're ready :)

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 21, 2024

mariajgrimaldi added the FC Relates to an Axim Funded Contribution project label Nov 21, 2024

mariajgrimaldi marked this pull request as ready for review November 21, 2024 18:18

mariajgrimaldi requested review from sarina, felipemontoya and Ian2012 November 21, 2024 18:19

sarina reviewed Nov 25, 2024

View reviewed changes

mariajgrimaldi added 3 commits November 26, 2024 12:54

docs: add comparison table between the framework and forking openedx

65f1156

docs: add reference to contribute features

bb6e9ad

refactor: address PR reviews

b8d7719

mariajgrimaldi force-pushed the MJG/hooks-vs-forking branch from 9942b58 to b8d7719 Compare November 26, 2024 11:55

mariajgrimaldi requested a review from sarina November 26, 2024 11:56

docs: clarify edx-platform dependency in plugins

d4266da

Ian2012 approved these changes Dec 9, 2024

View reviewed changes

sarina approved these changes Dec 9, 2024

View reviewed changes

felipemontoya reviewed Dec 9, 2024

View reviewed changes

mariajgrimaldi added 2 commits December 10, 2024 12:18

docs: remove deep understanding of the platform disadvantage

1251605

felipemontoya approved these changes Dec 11, 2024

View reviewed changes

sarina merged commit 61cc5aa into openedx:main Dec 11, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: [FC-0074] add comparison table between the framework and forking openedx #617

docs: [FC-0074] add comparison table between the framework and forking openedx #617

mariajgrimaldi commented Nov 21, 2024 •

edited

Loading

openedx-webhooks commented Nov 21, 2024 •

edited

Loading

sarina Nov 25, 2024

mariajgrimaldi Nov 26, 2024 •

edited

Loading

sarina Dec 5, 2024

mariajgrimaldi Dec 9, 2024

sarina Nov 25, 2024

sarina Nov 25, 2024

mariajgrimaldi Nov 26, 2024 •

edited

Loading

sarina Nov 25, 2024

sarina Nov 25, 2024

felipemontoya left a comment

felipemontoya Dec 9, 2024

mariajgrimaldi Dec 10, 2024

felipemontoya Dec 9, 2024

mariajgrimaldi Dec 10, 2024

felipemontoya Dec 9, 2024

felipemontoya Dec 9, 2024

mariajgrimaldi Dec 10, 2024

felipemontoya Dec 9, 2024

mariajgrimaldi Dec 10, 2024

felipemontoya Dec 9, 2024

felipemontoya Dec 9, 2024

mariajgrimaldi Dec 10, 2024

felipemontoya Dec 9, 2024

mariajgrimaldi Dec 10, 2024

mariajgrimaldi commented Dec 10, 2024

felipemontoya left a comment

mariajgrimaldi commented Dec 11, 2024

	\| \| - Might introduce an edx-platform dependency into plugins, mainly when using filters. \| \|
	\| \| - Although it is not recommended to do so, a plugin might introduce an edx-platform dependency, mainly when using filters, when there is no alternative for getting certain types of data. \| \|

	\| Community Compatibility \| High, as the community encourage reusable implementations and the contribution of new extension points. \| Low, as changes in forks are usually highly customized not designed for others to reuse. \|
	\| Community Compatibility \| High, as the community encourages reusable implementations and the contribution of new extension points. \| Low, as changes in forks are usually highly customized and not designed for others to reuse. \|

	\| Implementation Effort \| Low to medium, depending on the complexity of the receiver logic and filter pipelines. \| High, as modifications require expertise in the platform and increase technical debt. \|
	\| Implementation Effort \| Low to medium, depending on the complexity of the receiver logic and filter pipelines. \| High, as modifications require expertise in the platform and increases technical debt. \|

	\| \| core codebase but by leveraging plugins and configurations. \| \|
	\| \| core codebase by leveraging plugins and configurations. \| \|

	\| \| - Enables reusable and configurable behavior across the platform. \| - Suitable for highly customized use cases. \|
	\| \| - Enables reusable and configurable behavior across the platform. \| - Suitable for extremely customized use cases. \|

docs: [FC-0074] add comparison table between the framework and forking openedx #617

docs: [FC-0074] add comparison table between the framework and forking openedx #617

Conversation

mariajgrimaldi commented Nov 21, 2024 • edited Loading

Description

openedx-webhooks commented Nov 21, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

Choose a reason for hiding this comment

mariajgrimaldi Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariajgrimaldi Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipemontoya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariajgrimaldi commented Dec 10, 2024

felipemontoya left a comment

Choose a reason for hiding this comment

mariajgrimaldi commented Dec 11, 2024

mariajgrimaldi commented Nov 21, 2024 •

edited

Loading

openedx-webhooks commented Nov 21, 2024 •

edited

Loading

mariajgrimaldi Nov 26, 2024 •

edited

Loading

mariajgrimaldi Nov 26, 2024 •

edited

Loading