search

openedx / wg-devops

Issue repository for the DevOps Working Group
1 stars 2 forks source link

Document available Tutor patches and their uses #28

Open kdmccormick opened 2 years ago

kdmccormick commented 2 years ago

Context

~The current guidance for plugin developers looking for patches they can hook into is to grep for the string {{ patch and then infer what the implications of using the patch would be. Could official patches be enumerated and described somewhere? Even better, could this documentation be written inline and then generated, and could it include patches defined by other plugins?~

After discussing below, we resolved that patches should be documented by hand. This scaffolding for this is in place, but many patches remain undocumented: https://github.com/overhangio/2u-tutor-adoption/issues/50

In a separate issue we will propose a CLI addition for enumerating patches: https://github.com/overhangio/2u-tutor-adoption/issues/50.

Blockers

We should let openedx/wg-developer-experience#32 solidify first in case it majorly changes the Patches plugin point.

Acceptance

Note: Doing all of this would take a long time. Contributors, I recommend documenting a handful of patches at a time. We can split this issue into subtasks if that would help.

regisb commented 2 years ago

Yes, I agree that the detection and documentation of patch statements should be improved.

Could official patches be enumerated and described somewhere?

Yes, we can certainly generate a yaml file of all the patches that we go through whenever we run tutor config save. We could generate this list for any plugin with, for instance: tutor config patches dump --limit=myplugin --output=./myplugin/patches.yml. Then, we could generate some reference documentation that would be included in the tutor docs.

I'm not too sure where the patches should be documented: in the template themselves, where we would add {% patch_doc('my-name) %}...{% end_patch_doc %} blocks, or in the destination yaml file? Both have their pros and cons.

I'm even less sure about how to visualize patch statements from other plugins. I think that the documentation for these plugin patches should reside in the plugin repo. But I guess that the underlying question is rather: how should plugins be documented? Currently, they don't have a documentation website of their own. And as long as a README is good enough, then I'd rather not confuse users with extra documentation websites. Maybe that a yaml file is good enough for inclusion in the plugin docs?

kdmccormick commented 2 years ago

Starting with the high-level question,

But I guess that the underlying question is rather: how should plugins be documented? Currently, they don't have a documentation website of their own. And as long as a README is good enough, then I'd rather not confuse users with extra documentation websites.

I agree. I bet the vast majority of plugins are going to be simple enough that a documentation site for each one would be overkill.

That being said, I have a fondness for tools that include documentation in the tool itself. I like that I can explore the Tutor command line interface by running tutor --help, tutor dev --help, tutor dev start --help, etc. I think it would be slick if the CLI could programmatically tell the user available patches, with something like:

$ tutor config patches list
PATCH                           PLUGIN        DESCRIPTION
openedx-common-settings         core          Python to be added both LMS and
                                              CMS common settings modules
local-docker-compose-services   core          Additional services to run in local
                                              and dev mode, using docker-compose.yml syntax
credentials-certificate-footer  credentials   HTML to add to footer of course
                                              certificate (hypothetical)
...etc

(as an aside, I also think it cool if there were a corresponding tutor config list to show all configuration variables, with documentation)

With that in place, would be reasonable for a plugin's README to just direct users to run tutor config patches list --limit=myplugin to see all available patches.

Yes, we can certainly generate a yaml file of all the patches that we go through whenever we run tutor config save. We could generate this list for any plugin with, for instance: tutor config patches dump --limit=myplugin --output=./myplugin/patches.yml. Then, we could generate some reference documentation that would be included in the tutor docs.

The yaml generation sounds great to me (if you do like my CLI idea, then both the yaml and the CLI docs could be generated by the same subsystem). And including the generated reference docs in Tutor's docs would be awesome.

I'm not too sure where the patches should be documented: in the template themselves, where we would add {% patch_doc('my-name) %}...{% end_patch_doc %} blocks, or in the destination yaml file? Both have their pros and cons.

I like the idea of in-template documentation for the same reason I like pydoc and the code annotations in openedx-toggles: it puts the reference documentation right next the source of truth (the code) which makes it easier for developers to write and more obvious when it has fallen out-of-date.

I also like the patch_doc block syntax you propose. We would need to handle the case where multiple patch docs are defined for the same patch (choose the first one? fail the command?) but that shouldn't be too hard.

regisb commented 2 years ago

After reading this other backlog entry I must admit that I'm having a change of heart about automated doc generation. While more "interesting" from a purely technical perspective, I believe that it would also cause a ton of technical problems (similar to what we experience with code annotations) and not resolve any actually important one. I'll clarify by replying to your comments.

I think it would be slick if the CLI could programmatically tell the user available patches, with something like

Admittedly this is cool, but do we really want/need this? What problem would it resolve? If any, can we just grep the documentation?

When I am writing Tutor plugins I frequently check the Tutor docs myself. And when the answer is not in the docs I grep the code, which is always going to be "more true" than the docs.

Alternatively, we can get the same output with little effort by dropping the "Description" column. The output of tutor config patches list would then only include the patch names and file path. Would that be sufficient?

The yaml generation sounds great to me (if you do like my CLI idea, then both the yaml and the CLI docs could be generated by the same subsystem). And including the generated reference docs in Tutor's docs would be awesome.

By generating an additional yaml file, we solve one problem but end up with another one, which is that we need to maintain and keep up-to-date that yaml file.

The more I think about it, the more I believe that we should adopt the Django approach and be more disciplined during PR reviews, to make sure that all patches and settings are correctly documented.

Note that I still believe that this issue is very much valid. Patches must be documented. Maybe we can start with a more manual approach and automate later if necessary.

kdmccormick commented 2 years ago

Fair, I may be over-engineering here. When something seems "cool" it can be tough to put that aside and determine whether and it's "cool and useful" versus, well, "just cool" :)

Alternatively, we can get the same output with little effort by dropping the "Description" column. The output of tutor config patches list would then only include the patch names and file path. Would that be sufficient?

I still think there would be value in seeing all available patches in one place, but if we resolve to also document all patches manually, then the CLI diminishes in importance from "need to have" down to "nice to have".

The more I think about it, the more I believe that we should adopt the Django approach and be more disciplined during PR reviews, to make sure that all patches and settings are correctly documented. Note that I still believe that this issue is very much valid. Patches must be documented. Maybe we can start with a more manual approach and automate later if necessary.

When I was at edX there was a strong desire to use tooling as much as possible to help people follow standards (like writing docs) and then automatically check that they did. With 200+ committers to edx-platform and varying levels of review stringency between teams, you can see how that might be important, even though the tooling consumed significant dev resources.

But, Tutor is a much smaller codebase with a much smaller set of core committers. So, I am in favor of manual documentation and review 👍🏻

With that said, we do have Plugins all the way Down as a potential roadmap item. Executing that task would mean many settings and patches currently in the core would move outward to the LMS/CMS plugin(s). I'm not sure if docs for those would stay in the core docs or move out along with the plugin code. If we assume for a second that we are extracting LMS & CMS, does that change your feeling on patch and config docs at all?

regisb commented 2 years ago

Well, when we move the LMS and the CMS out of the core, I expect that the current tutor documentation is still very much going to remain the same. It's actually the "core" that will be moved away from tutor and to a different project, with its own documentation. So if we end up with two new documentation pages where we have an exhaustive list of patches and settings, then this list will remain there.

Oh but you mean that it would be convenient for any "tutor core" user to list patches and settings, even when they don't use tutor? This is a good point. This "tutor core" tool could indeed provide the following CLI:

tutorcore patches list --limit=myplugin # list all patches by name and file path
tutorcore patches show some-patch # print the rendered content of the patch

I'm still not too sure about how to best document those patches and settings... My intuition tells me that manually generated docs is the best choice. Can we maybe go down this road and reconsider this choice if "tutor core" ever becomes a runaway success?

kdmccormick commented 2 years ago

Yes, let us do that 👍🏻

I now have a lot of curiosity about tutor vs tutorcore but I will put those over on https://github.com/overhangio/2u-tutor-adoption/issues/10 :)