Add CLI tool for listing patches available from installed plugins

[kdmccormick]

Context

Originally proposed in https://github.com/overhangio/2u-tutor-adoption/issues/5 was an in-code documentation system for patches accompanied by a CLI tool like this:

$ 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

We decided that the implementation and maintenance of this would not justify its worth.

Instead, we will document patches manually (as described in https://github.com/overhangio/2u-tutor-adoption/issues/5). Then, we could make a command-line tool just to list the patches and the locations that they will render (relative to the Tutor root), like this:

$ tutor config patches list
PATCH                           LOCATIONS
openedx-common-settings         env/apps/openedx/settings/cms/production.py
                                env/apps/openedx/settings/lms/production.py
local-docker-compose-services   env/dev/docker-compose.yml
credentials-certificate-footer  env/plugins/credentials/apps/credentials/html/certs/footer.html
...etc

Blockers

Plugin API v1: #32

Acceptance

• Propose an CLI syntax and output format on the Tutor Forums.
• Implement it.

[MaferMazu]

I can take this issue if you agree, @kdmccormick :D

[kdmccormick]

All yours!

Heads up @regisb that @MaferMazu will be working on this.

[MaferMazu]

Hi, I want to ask about the implementation of this feature.

At first moment I thought it interesting to extract the info for this command from the patches.rst, and I found two approaches:
First, the more accessible, but I don't think it is elegant, read the doc line by line and extract the patches and the associated files.
Second, try to parse the RST file, and with that structure, try to extract the info. For this approach, I need to create a Directive for patch and access to the information with a tree or something like an XML structure.

Another approach I had was like have another doc with a simple structure that provides the patch names and files, but we would maintain two docs of patch documentation.

What do you think @kdmccormick @rgraber @Carlos-Muniz ? Which is the best approach?

Another question I have is about the column output. Do you know a better way to do it than ljust() or {: >20}.format() ?

I would appreciate any feedback 😊

[regisb]

If I were to implement this feature I would not try to parse the patches.rst file at all. Instead, I would examine the values in hooks.Filters.ENV_PATCHES.callbacks. In particular, the callback.context attribute will tell you the name of the app it comes from.

Does that make sense? This introspection is fairly low-level in the hooks API, and it's not something that is commonly done by most developers (well I hope not!). Start by implementing the command, have a look at the values above and it should work 🤞

EDIT: My answer only allows you to list the existing patches, by name, but not to document them or to figure out in which templates they occur. I need to think about this a little more.

[kdmccormick]

From Slack:

@MaferMazu :
The issue #50 is to list all patches used by all installed plugins, right?
For example, if I have two plugins, one using openedx-common-settings and caddyfile, and the other using openedx-common-settings and openedx-common-i18n-settings, the output of the command would be only the three patches and the location of each one.
I understand that the form to do it is analyzing the callbacks.context of ENV_PATCHES.
Am I on the same page? Please let me know if I am not.

[kdmccormick]

What do you think @regisb ? Personally, I cannot think of any way to do this other than recursively scanning each directory in ENV_TEMPLATE_ROOTS and looking for instances of patch("...").

[MaferMazu]

I was trying to iterate over ENV_PATCHES, and I need to populate the Filter. So I was thinking about a pre-load, like reading the config with config = tutor_config.load_minimal(context.root) and then loading the plugins like this to obtain the patches somehow.

The other way, I think, is the idea of Kyle, is doing something with the ENV_TEMPLATE_ROOTS, maybe using this.

Am I on the right path?

@kdmccormick @regisb

[regisb]

What do you think @regisb ? Personally, I cannot think of any way to do this other than recursively scanning each directory in ENV_TEMPLATE_ROOTS and looking for instances of patch("...").

For a start, I've just realized something: we will not be able to list the file associated to {{ patch(...) }} statements which are themselves included in a patch. That's because plugin patches are not templates: they are just regular strings.

For instance, if a plugin implements the "caddyfile" patch, and in this patch there is a {{ patch("custompatch") }} statement, then we will not be able to associate the "custompatch" to any given template.

This is a relatively minor detail, but let's keep it in mind.

Now, to our initial problem. We want to print a list of <patch name> <template path>. In theory, the Renderer.patch method already sees the name of each patch that goes through it. When we render the full environment, this method will be called at least once for every {{ patch(...) }} statement. We can verify that by adding print(name) to the Renderer.patch method.

Thus, all we need to do is to find in which template the patch function was called.

Let's create a quick-and-dirty proof of concept. There are different ways to make this work, here is one. First, we create a custom Renderer class. Something along the lines of:

from tutor.env import Renderer
class PatchRenderer(Renderer):
    def render_template(self, template_name: str) -> t.Union[str, bytes]:
        self.current_template = template_name
        return super().render_template(template_name)

    def patch(self, name: str, separator: str = "\n", suffix: str = "") -> str:
        print(name, self.current_template)
        return super().patch(name, separator=separator, suffix=suffix)

Then, we render the full environment with our custom renderer:

renderer = PatchRenderer(config)
renderer.render_all_to("some-dummy-dir")

This should print all patch statements, unsorted. They should be de-duplicated, sorted and nicely formatted. And of course the code should be nicer -- maybe with custom filters?

I was trying to iterate over ENV_PATCHES, and I need to populate the Filter.

That was my initial proposal, but I was completely confused. It would help us find all the patches introduced by plugins, but not the {{ patch(...) }} statements. Does this make sense?

[kdmccormick]

@regisb Your approach sounds great to me.

Zooming in on the patches-within-patches minor detail for a bit:

For instance, if a plugin implements the "caddyfile" patch, and in this patch there is a {{ patch("custompatch") }} statement, then we will not be able to associate the "custompatch" to any given template.

The inner {{ patch ("custompatch") }} call will still be rendered, resulting in a call to Renderer.patch, right? So we should still be able to print it out via something like:

    def patch(self, name: str, separator: str = "\n", suffix: str = "") -> str:
        print(name, self.current_template)

        # Store the template's name, and replace it with the name of this patch.
        # This handles the case where patches themselves include patches.
        original_template = self.current_template
        self.current_template = f"within patch: {name}"

        rendered_patch = super().patch(name, separator=separator, suffix=suffix)
        self.current_template = original_template  # Restore the template's name from before.
        return rendered_patch

which would yield an output like:

PATCH           TEMPLATE
caddyfile       <template path>
custompatch     within patch: caddyfile

If you were to make the patch implementation smarter, you could probably even include the plugin that the patch is coming from.

[regisb]

self.current_template = f"within patch: {name}"

Damn, I wish I had thought of that myself. Looks rad!

[MaferMazu]

I was thinking of adding self.current_template = template_name in the render template function, and in the patch function creates a file to store the patch name and the template that patches edit: but only store the patches that are modified by a plugin. Then when we execute the tutor config patches list command only read that file created. What do you think? Do you have a better approach? @kdmccormick @regisb @Carlos-Muniz @rgraber

About the @kdmccormick comment, I haven't used patches-within-patches; if you have an example, I appreciate it for better understanding. ✨

[kdmccormick]

Sorry for the slow response @MaferMazu.

Saving the list to a file is an interesting idea, but I do not prefer it because it would persist state between commands. When we rely on state, there is extra room for errors. For example, if someone installed a plugin and then ran tutor config patches list before running tutor config save, then the file would be missing or outdated.

Instead, I recommend following Régis's advice above, which is to subclass the Renderer class and override its render_template and patch functions so that they print(...) each patch occurrence. Then, this new subclass could be used to implement the tutor config patches list command. Does that approach make sense?

[MaferMazu]

Yes, it makes sense. So I'm going to start working on that. I only have a question, and it's about where patches include patches; if you have an example, that would be helpful.

[kdmccormick]

where patches include patches

@MaferMazu , I could have sworn that some plugins had patches that included other patches, but to be honest I cannot find any examples. Sorry about that! When we extract LMS & CMS into a plugin, I expect that we will have plenty of examples, but for now I'll just have to invent one:

Consider tutor-mfe's implement of the caddyfile patch, which adds routing for the .apps. subdomain for MFEs (learning.apps.example.com, profile.apps.example.com, etc.). Now, imagine that we wanted to allow other plugins to add custom routes to .apps.. We might change the patch to this:

{{ MFE_HOST }}{$default_site_port} {
    {{ patch("caddyfile-mfe") | indent(2) }}  # <------------------------- This line was added

    respond / 204
    request_body {
        max_size 2MB
    }

    reverse_proxy /api/mfe_config/v1* lms:8000 {
        # We need to specify the host header, otherwise it will be rejected with 400
        # from the lms.
        header_up Host {{ LMS_HOST }}
    }
    import proxy "mfe:8002"
}

Now, imagine we create a tutor-mfe-custom plugin. That plugin could implement our new caddyfile-mfe patch as this:

respond /custom-path 200 {
  body "This is a custom response!"
  close
}

then http://apps.example.com/custom-path would return the custom response.

To summarize:

• The caddyfile template in Tutor core exposes a caddyfile patch.
• tutor-mfe implements the caddyfile patch, and exposes its own caddyfile-mfe patch.
• tutor-mfe-custom implements the caddyfile-mfe patch ("a patch within a patch").

At the moment, this is an edge case. If handling it is causing problems in your implementation, I would say go ahead and ignore the patch-within-a-patch edge case. If the edge case becomes common in the future, we can always come back and add support for it 😄

[MaferMazu]

Kyle, thanks for your answer.
I started a draft PR here overhangio/tutor#798 for this issue; I will refactor it and then open the PR for your review.
I think I forgot to list only the patches we use with our plugins. We want to list only the patches we are using, right? cc @kdmccormick

[kdmccormick]

@MaferMazu Great!

We want to list all patches, whether they are used or not, and whether they are from the core or from plugins. That way, when somebody is a developing a new plugin, they can use this tool in order to see all patches that are available for them to use.

[MaferMazu]

@kdmccormick, the PR for this issue is ready for review. Maybe it's a good idea to link this issue with that PR and set this card in review status.
PR: overhangio/tutor#798

[kdmccormick]

I don't have permission to link the PR to this issue unfortunately, but I've marked it as In Review.

[MaferMazu]

@kdmccormick I think we can close this issue; the PR overhangio/tutor#798 was merged.

[kdmccormick]

🎉