Improve clarity of No New Apps ADR

[robrap]

A reader of https://github.com/openedx/edx-platform/blob/master/docs/decisions/0014-no-new-apps.rst misinterpreted this as meaning that the service shouldn't be added to at all. The various sections of the ADR could be re-worded to make it abundantly clear that this is not about eliminating the ability to add new apps to the LMS, but only to keep that new code outside of the edx-platform repository.

Other related notes:

• The 2U code for detecting new apps (or directories) in the ownership spreadsheet is not working correctly. So, new apps only get detected once they are observed in production without a code owner. This may be good enough, but new apps with no active views will go undetected.
• We should have a README for each new app, but I don't think that is documented anywhere. Do we need another ADR, or could we add that request for documentation to this ADR?

Acceptance Criteria

• Update OEP-49 in 2 PRs or commits: docs: update oep-49 for clarity around new apps open-edx-proposals#455
  • 1) Make it a best practice OEP instead, and
  • 2) Add section for adding an ADR to explain the decision behind adding the app.
• Update the No New Apps ADR in at least the following ways:
  • Describe it as extending the requirements of the OEP-49 by providing a special section if the app is being added directly to the edx-platform repo.
  • Clarify that the ADR, including the title, is all about not adding new apps directly to the edx-platform repo.
  • Consider adding a plugin example, maybe to the plugin docs, or maybe to the ADR?
  • Improve the ADR and/or plugin docs based on comments below. Note: I think we should not use plugins where we can use an installed app, but there may be further discussion required.
    • Add tasks here based on the recommendations below.
• Finally, unblock the Recommendations ADR that is under review by providing these new docs. Maybe we can also support them in moving that code if it should be moved, which it seems like it should.

[kdmccormick]

The README recommendation is a good idea, not just for edx-platform. If you have the capacity to, consider adding that to the Django App Patterns OEP.

[kdmccormick]

For new-app-observability, I wonder if you could leverage CODEOWNERS to trigger an Arch review request.

[robrap]

For new-app-observability, I wonder if you could leverage CODEOWNERS to trigger an Arch review request.

@kdmccormick: I'm not sure exactly what this means. Will this make more sense in the future when-and-if we are using CODEOWNERS consistently over our legacy ownership spreadsheet?

[robrap]

I'm wondering if this ADR should be separate from a how-to on adding a new app? New apps could have an ADR explaining why they are needed at all. The no-new-apps ADR is specifically looking for details in an ADR about why the app must live in edx-platform, rather than in an external repository. The creator of a new app may want to write a single ADR, but we need to ensure that both these requirements are met (or at least the requirement document in this ADR).

See #31665 for an attempt to follow this ADR, and some possible clues on how we could make this all more clear.

[kdmccormick]

@robrap Putting aside both the legacy ownership and new maintainership systems... Arch-BOM could set up CODEOWNERS expressly for the purpose of enforcing this ADR, something like this:

# Arch-BOM "owns" each Djangoapp root.
# (this is not "owns" in the sense of OEP-55 maintainership nor operator-ownership)
# This means that they will get pinged on any PR that modifies these directories.
# edx-platform settings could also be modified in order to restrict those PRs from
# merging without their review.
# This would apply to subdirectories (the actual Django apps) too, but we exempt those below.
lms/djangoapps @openedx/arch-bom 
cms/djangoapps @openedx/arch-bom 
common/djangoapps @openedx/arch-bom 
openedx/features @openedx/arch-bom 
openedx/core/djangoapps  @openedx/arch-bom 

# Existing Djangoapps are exempted (assigned to no owner).
lms/djangoapps/badges
lms/djangoapps/courseware
lms/djangoapps/course_api
...
cms/djangoapps/contentstore
...
etc. # remaining existing djangoapps

Of course, if it seemed valuable in the future, the "existing djangoapps" section could be mapped to maintainership or operator-ownership assignments without disrupting this setup.

I don't feel especially strongly about whether you folks go with this system, but I wanted to suggest it while you were considering solutions to:

new apps only get detected once they are observed in production without a code owner. This may be good enough, but new apps with no active views will go undetected.

[robrap]

The README recommendation is a good idea, not just for edx-platform. If you have the capacity to, consider adding that to the Django App Patterns OEP.

@kdmccormick: This is a good idea. We also may want an ADR that explains why we decided to create the app to begin with. This ADR could then note the specific new requirements for the app ADR of any new app going directly into the edx-platform repository.

Unfortunately, OEP-49 was added as type "architecture". I added notes around these two possible changes to OEP-49 in this new issue: openedx/open-edx-proposals#439.

[robrap]

@robrap Putting aside both the legacy ownership and new maintainership systems... Arch-BOM could set up CODEOWNERS expressly for the purpose of enforcing this ADR, something like this:

@kdmccormick: Understood. Managing ownership attribution in edx-platform is an odd job that Arch-BOM on-call has to take care of, and I wouldn't want any additional manual overhead. That doesn't mean we should do something like this, but I wouldn't do it until we can automate moving or duplicating the data.

In the meantime, our current automation does serve as a linting mechanism, which is how I noticed a new app to begin with.

[hurtstotouchfire]

@robrap I anticipate that edx-platform will be the very last repo to come under Maintainership in the new sense, and I think we need to be planning for the interim.

For new-app-observability, I wonder if you could leverage CODEOWNERS to trigger an Arch review request.

For this piece, I think we should solve for edx-platform specifically and not worry about it generalizing to other django apps.

Are you saying though that the linting is sufficient and we don't need another mechanism?

[kdmccormick]

Yeah, I took this:

The 2U code for detecting new apps (or directories) in the ownership spreadsheet is not working correctly.

to mean that you were looking for an alternative new-app-detection solution.

If you're all set on that front, though, then nevermind the CODEOWNERS thing.

[robrap]

Yeah, I took this:

The 2U code for detecting new apps (or directories) in the ownership spreadsheet is not working correctly.

to mean that you were looking for an alternative new-app-detection solution.

@kdmccormick

1. It is working for any apps that create traffic in Production.
2. It is broken for generally detecting new directories, but fixing should be relatively simply if and when we prioritize. Switching to an entirely new system would be a large undertaking, and probably would use this same broken code that investigates the file system.

[robrap]

Here are additional questions that may affect this ADR, or related docs, like the plugins README.

1. If we add a dependency into the requirements of a service, can anyone think of a reason to make a plugin? Do we all agree that if we are updating dependencies, we can also update INSTALLED_APPS, and make the whole dependency explicit?
2. I imagine that the more useful use of plugins, or discoverable apps, is when using privately installed dependencies. Do we all agree?
3. Where and how should we document guidance on when an app should be a plugin versus a hard dependency. With a hard dependency, we could still clearly toggle on/off functionality. Do we have some gut threshold around the percent of community deployments that would use the functionality? 50%? 80%? Some other way to document this? Or, do we simply want to clarify that if it is clear that few if any other deployments will use the functionality, make it a plugin, and leave it at that?

Also note that the doc https://edx.readthedocs.io/projects/edx-developer-guide/en/latest/extending_platform/index.html is completely out-of-sync with the docs referenced in this ADR, which seems like a big documentation issue.

[robrap]

Also, does anyone know of a good reference implementation for a plugin? I'm not sure if that is needed, but it was requested by @zainab-amir.

[kdmccormick]

I'm not sure if you're looking for input now, but here are my opinions anyway, in case they might be useful to the ADR author...

If we add a dependency into the requirements of a service, can anyone think of a reason to make a plugin? Do we all agree that if we are updating dependencies, we can also update INSTALLED_APPS, and make the whole dependency explicit?

I will try to steel-man (give my most generous view on) the idea of adding direct dependencies as plugins:

1. It may be easier to extract a dependency later if it is implemented using the plugin framework.
2. Implementing built-in apps via the plugins system forces upstream developers to "dogfood" the plugin system. Breakages to the plugin system are more likely to break upstream CI. Annoyances in the plugin system are more likely to be noticed by upstream developers.
3. Central settings files (including the INSTALLED_APPS list) do not need to be modified (but setup.py still does). Those central files could then be write-protected (for example, by an architecture team) without getting in the way of teams that are developing new Django apps.

But I also have counterarguments for all of these:

1. The hardest part of extracting an app is NOT the mechanical bits (settings location, plugin config in apps.py). The hardest part is untangling it from other apps which have begun to depend on its code or its behavior. Making something a Django app plugin does not guard against that.
2. Rather than improving the plugin system, making everything a plugin muddies the waters of which apps are critical parts of edx-platform and which ones can unplugged. Does it really make sense to call authn_provider a "plugin", given that extracting it would be a multi-month effort? Our system would be more comprehensible if all only optional components were "plugins", and everything else was just called an "app" or a "library".
3. Write-protecting central settings files does not really protect edx-platform from architectural rot. Undesirable dependencies between apps can be created without touching the central files.

Personally, I lean towards my counterarguments: dependencies from base.in should NOT be plugins, nor should in-repo apps. An interesting corollary of this opinion is that we should remove all xblocks from base.in, except those that edx-platform has dependencies into.

I imagine that the more useful use of plugins, or discoverable apps, is when using privately installed dependencies. Do we all agree?

If, by "privately installed", you mean "not installed by base requirements" rather than strictly private, than I agree 👍🏻

To expand on what I mean, I could see there being public edx-platform plugins for generally-used features that we want run by default in named releases, yet we want implemented only using official edx-platform plugin APIs in order to keep them decoupled from edx-platform's core. Based on OEP-57, these would be a type of bundled extension. For example, if grading was reimplemented as a proper plugin, then we would keep it public and install it by-default into named releases.

Where and how should we document guidance on when an app should be a plugin versus a hard dependency. With a hard dependency, we could still clearly toggle on/off functionality. Do we have some gut threshold around the percent of community deployments that would use the functionality? 50%? 80%? Some other way to document this? Or, do we simply want to clarify that if it is clear that few if any other deployments will use the functionality, make it a plugin, and leave it at that?

I want to take a very hard-line stance there: All new features should be implemented as plugins and use official plugin APIs, like the XBlock framework or the Hooks API. If the plugin APIs are not sufficient, then they should be augmented to support the feature. Exceptions would only be made for features that really belong in the "core", although the only such features I can think of would be replacements for existing components, such as a replacement for the LMS courseware unit renderer.

This lets us throw away the gut-threshold-of-usage calculation. Just make a plugin, and then let operators decide whether or not they want to use it

(Calling back to the "dogfood" argument from the top of my comment: this is how we would really dogfood a plugin system. This policy would be a pain in the neck at first... but then it would lead us to make an awesome set of plugin APIs!)

[robrap]

Thanks @kdmccormick. Great thoughts and I will be coming back to this soon.

Note to self: There have been several requests for an example plugin in the plugin documentation.

[robrap]

@davidjoy: [inform/question] Not sure if you want to participate in helping with any of these docs, or simply want to be aware of where all this lands, in relation to the extensibility question. Thanks.

[davidjoy]

I'll happily read along and improve my mental model, but think that y'all have way more subject matter expertise here than I ever will, and that I implicitly trust your choices. I mean, if you want an opinion on something I'll give it, but otherwise I'd just lurk. 😛

[robrap]

I'm going to document things I think we need to do, and then unassign and move back to groomed (or TODO):

• Update OEP-49 in 2 PRs or commits: 1) Make it a best practice OEP instead, and 2) Add section for adding an ADR to explain the decision behind adding the app.
• Update the No New Apps ADR in at least the following ways:
  • Describe it as extending the requirements of the OEP-49 by providing a special section if the app is being added directly to the edx-platform repo.
  • Clarify that the ADR, including the title, is all about not adding new apps directly to the edx-platform repo.
  • Consider adding a plugin example, maybe to the plugin docs, or maybe to the ADR?
  • Improve the ADR and/or plugin docs based on comments above. Note: I think we should not use plugins where we can use an installed app, but there may be further discussion required.
• Finally, unblock the Recommendations ADR that is under review by providing these new docs. Maybe we can also support them in moving that code if it should be moved, which it seems like it should.

[kdmccormick]

Thanks @robrap .

[robrap]

@rgraber: You may want to take this other ticket into account: edx/edx-arch-experiments#206

[rgraber]

Going to take a stab at summarizing the guidance we want to give, for my own benefit as well as posterity:

1. There should be a conscious effort to make all new features work as plugins, ie things that do not need to be called directly elsewhere in edx-platform. Plugins are installed into private dependencies.

2. If it's necessary for edx-platform to call your app code directly, it should NOT be a plugin. That is to say, it should be added to base.in and INSTALLED_APPS.

3. For both cases, use a new repo unless you have a really good reason (and describe that reason in an ADR)

Note that here I'm not talking about where we will put all this guidance, just that this is the guidance we want to give. Is this more or less correct?

[kdmccormick]

I think that's a great summary 👍🏻

[robrap]

Great summary. The first point I'd add a little nuance.

1. There should be a conscious effort to make all new features work as plugins, ie things that do not need to be called directly elsewhere in edx-platform. Plugins are installed into private dependencies.

The "ie" is where this gets tricky. Depending on what you are adding, and whether or not it will be core to the platform, it might require an interface that the edx-platform calls without knowing about your specific implementation. Part of these docs are about helping uncover these opportunities, and documenting them, and maybe even fixing them and making the appropriate API that didn't before exist.

[rgraber]

I'm not sure where to put the plugin-vs-not information (possibly https://docs.openedx.org/en/latest/developers/concepts/index.html ), but I definitely would not put it in the ADR for not adding new apps to the edx-platform repository. We could specifically say "this ADR does not address what kind of app you should create, for that, see :some doc:"

[robrap]

I'm not sure where to put the plugin-vs-not information (possibly https://docs.openedx.org/en/latest/developers/concepts/index.html )

Interesting that that doc points to an ADR in plugins, and not the README (or a how-to). I think the README should provide this context, and if you wanted to try to summarize anything in the concepts doc, you could try, but otherwise maybe we just point to the README instead (once updated). See https://github.com/openedx/edx-django-utils/blob/master/edx_django_utils/plugins/README.rst

[bradenmacdonald]

Whoops, I wasn't aware of this thread so I posted a long related idea on the forum. But the key points are:

1. Personally I think focusing on whether the code is in the edx-platform repo or in its own repo is not very important.
2. What is important is whether or not there's a clear boundary and interface separating the app from the core. What is importing what? When everything is installed and importable in the same python virtual environment, boundaries get eroded over time without people even noticing.
3. The best way to ensure that the boundaries are clear and respected is to make sure that there is a build+test suite that runs without your app being installed at all (in the python venv).
4. If you add something new to the platform that's not "core" functionality for other things to build on, make sure that the platform can be installed and run without your app, and that there's an automated check to ensure that stays the case going forward.
5. A good example of what not to do is edx-enterprise. It is non-core and should be a separable plugin, but you cannot currently run the platform without having it installed. This is the case even though it was built in a separate repo and generally follows this ADR.

Number 3 and 4 are not easily possible today, but I think we can easily make them possible with what I suggested in the thread (separating base.in into two files).

[robrap]

Thanks @bradenmacdonald.

But the key points are:

1. Personally I think focusing on whether the code is in the `edx-platform` repo or in its own repo is not very important.

I agree that pluggability is more important than the separate repo. I know there are advantages and disadvantages to the separate repos, so I'll leave that for now.

2. What is important is whether or not there's a clear boundary and interface separating the app from the core. What is importing what? When everything is installed and importable in the same python virtual environment, boundaries get eroded over time without people even noticing.

I think @kdmccormick introduced some linting capabilities around boundaries that would be an additional or alternate approach.

3. The best way to ensure that the boundaries are clear and respected is to make sure that there is a build+test suite that runs without your app being installed at all (in the python venv).

Note that sometimes the boundaries can't be respected as-is, and the platform needs to provide better interfaces. Part of this ADR was about helping explain and capture some of that information.

4. If you add something new to the platform that's not "core" functionality for other things to build on, make sure that the platform can be installed and run without your app, and that there's an automated check to ensure that stays the case going forward.

If the dependency isn't included, we get this for free, right? Are you just talking about the exceptions which we are trying to avoid? Could we use linting to help us avoid new exceptions, and then deal with the old ones at a later point in time?

5. A good example of what not to do is `edx-enterprise`. It is non-core and should be a separable plugin, but you cannot currently run the platform without having it installed. This is the case even though it was built in a separate repo and generally follows this ADR.

I think the ADR should better explain when and why to use a plugin Django app, which is more than just putting code in a separate repo.

In general I agree with your sentiments (except maybe the monorepo), and I'm simply just trying to add in other possibilities.

[bradenmacdonald]

@robrap

If the dependency isn't included, we get this for free, right? Are you just talking about the exceptions which we are trying to avoid? Could we use linting to help us avoid new exceptions, and then deal with the old ones at a later point in time?

Yeah that's true, as long as it's not installed by default that covers #4. But then your code won't be covered by edx-platform tests.

Could we use linting to help us avoid new exceptions, and then deal with the old ones at a later point in time?

Yes, I think that's a good approach. I think it also works hand in hand with my base.in/default.in idea. For example, you could write a lint rule that marks any import from a package that's not listed in base.in as an error. And then things like edx-enterprise can be moved to default.in, and we'll start getting lint errors for every place in the codebase that has inappropriate imports of non-core code. The existing ones can be grandfathered in as exceptions.

[kdmccormick]

I think @kdmccormick introduced some linting capabilities around boundaries that would be an additional or alternate approach.

Yup, importlinter is running on edx-platform PRs. The rules are pretty basic right now but I encourage folks to expand them & I am happy to help anyone who is interested but doesn't know where to start. One could certainly use importlinter rules to, say, add a library directly to edx-platform but enforce that the library doesn't rely on some/all other edx-platform apps.

Note that sometimes the boundaries can't be respected as-is, and the platform needs to provide better interfaces. Part of this ADR was about helping explain and capture some of that information.

I would gently push back on the idea that they can't be respected. I think that, historically, we have allowed those with direct edx-platform write access to disrespect the boundaries. Other community members were held to a higher standard: they were asked to use proper platform interfaces or create their own. That's why we now have the Hooks framework, Tutor plugins, etc.

I am not trying to be adversarial here... as someone who has been there, it is hard to tell your product manager that you need to build a plugin interface in order to implement the feature X when your entire team possesses direct upstream commit rights and could easily implement X as another djangoapp under openedx/core/djangoapps. I think it might be in everyone's best interest to put hard limits on what can be added to edx-platform and base.in.

[kdmccormick]

For example, you could write a lint rule that marks any import from a package that's not listed in base.in as an error. And then things like edx-enterprise can be moved to default.in, and we'll start getting lint errors for every place in the codebase that has inappropriate imports of non-core code. The existing ones can be grandfathered in as exceptions.

I don't think we could write a lint rule for this, since there's no easy mapping of distribution names (the entries in *.in files, eg edx-enterprise) to package names (the things you import, eg enterprise). What we could do, though, is run pylint a second time with only base.in installed, which would highlight all non-core code imports.

[ormsbee]

I'm in broad agreement with the direction you folks are taking this. I do want to mention a couple historical reasons for the push to split out into separate apps like:

• The edx-platform test suite used to take a long time to run. This is much better now due to various optimization and sharding efforts, but there was a very real fear that continuing to pile on code in edx-platform would make release cycle time prohibitively slow.
• As mentioned earlier here, repos are as much about teams as they are about functionality, and our encouragement of moving apps out predates GitHub's codeowners functionality. Even now, we talk about core contributors getting write access on a repo-level. Our choice of where apps go have as much to do with ownership, policy, and workflow as it does with architecture.

Right now, I think there's a kind of implicit assumption that there is a 1:1 relationship between an app/plugin and a repo, and I'd like to get away from that mindset. It's appropriate for certain repos like openedx-events, but I think it muddles other things. For instance, I don't think that "Enterprise" can ever be a coherent "plugin"–it's a feature that is probably a combination of a number of plugins to different plugin APIs in addition to a bunch of its own apps. But because it's thought of as "the place for enterprise things", it has an main enterprise app with a 3.5K line models.py that has everything from enrollment-related tables to catalogs, fulfillment, identity providers, branding, etc.

[rgraber]

Deleted my previous comments because I think they confused the issue more than helped.
Given that we're not going to be able to do everything everywhere all at once, I'm going to propose that we update the A/C on this ticket to be:

• Update the No New Apps ADR to make it clear that it doesn't mean we can't add any code to edx-platform at all or any new apps at all, just that when we add things they should be in a separate repo. I understand this last clause is controversial but that was the original point of the ADR, so this is a lateral change at worst and it makes the original purpose clearer.
• Update the New Plugin section of the docs site to point to the setup instructions in the plugins README. Also add a link to an example plugin if I can find one
• Clarify this recommendation, when we say we recommend extending rather than modifying the platform, to say we mean we recommend making a new plugin with no direct dependencies in edx-platform if at all possible

In the future, we can update that third thing with more guidance about whether something is in base, or default, or a plugin, or what to do if you really can't separate something because our plugin API is missing something.

[robrap]

I've got more thoughts and don't yet have time to document before a meeting. For now:

1. Please don't delete the old AC yet. I think there are nuggets in the bullets that you haven't captured yet.
2. Feel free to start on the first task you outlined here.
3. The second and third tasks you outlined I'd like to discuss a bit more.

[rgraber]

Here is the PR for that first bit: #31863

[rgraber]

Some additional clarification of the three A/C I proposed:

1. Was meant to include the first two sub-criteria in the original list (retitling and calling out OEP-49), which the PR did.
2. Was meant to include the third sub-criteria (including a link to an existing plugin)

[rgraber]

Is someone with write access able to merge openedx/docs.openedx.org#267 ?

[robrap]

This may be all set, but I still plan to review. Feel free to close when you think it is ready, and I can always create a new ticket.
I also created this ticket, instead of expanding scope on this one: openedx/edx-django-utils#276

[bradenmacdonald]

@kdmccormick @robrap @ormsbee @rgraber I wrote a new importlinter configuration that can enforce OEP-49 api.py boundaries for django apps / python packages - check it out and let me know what you think: #31903

[rgraber]

I think this can be closed. The original intent was to make it clearer that we just wanted certain information in the ADR that is required by OEP-49 and have a clearer starting point to people who want to begin developing a plugin, which I think we've done.